RUBY-2855 RUBY-2637 Fix dbref spec tests (#258)

* RUBY-2855 dont raise an error on implicit parsing

* RUBY-2855 raise error on explicit dbref parsing for invalid type

* RUBY-2855 don't accept dbref with invalid db type

* RUBY-2855 refactor, raise error on explicit invalid type for db in dbref

* RUBY-2855 remove unnecessary condition

* RUBY-2855 added fixed tests

* RUBY-2855 add trailing space

* RUBY-2855 add parse_hash to remainder of dbref hash

* RUBY-2855 remove failing tests

* RUBY-2855 update error messages

* RUBY-2855 make from_bson use implicit decoding

* RUBY-2855 fix MRI from_bson call

* RUBY-2855 add and update tests

* RUBY-2855 dbref nested inside dbref

* RUBY-2855 add from_bson validation tests for nested and top-level

* Ruby does not generally use is_ prefixes

* revert the bypass

* stop using renamed constants

* DBRef parsing in C

* do not convert DBRefs back to Documents

* remove unused var

* from_bson override is no longer needed

* pass validating_keys

* fix docstring

* instantiate DBRefs under jruby

Co-authored-by: Oleg Pudeyev <[email protected]>

ext/bson/bson-native.h

@@ -123,3 +123,5 @@ extern VALUE rb_bson_registry;
 extern VALUE rb_bson_illegal_key;
 
 extern const rb_data_type_t rb_byte_buffer_data_type;
+
+extern VALUE _ref_str, _id_str, _db_str;

ext/bson/init.c

@@ -30,13 +30,22 @@ const rb_data_type_t rb_byte_buffer_data_type = {
   { NULL, rb_bson_byte_buffer_free, rb_bson_byte_buffer_memsize }
 };
 
+VALUE _ref_str, _id_str, _db_str;
+
 /**
  * Initialize the bson_native extension.
  */
 void Init_bson_native()
 {
   char rb_bson_machine_id[256];
 
+  _ref_str = rb_str_new_cstr("$ref");
+  rb_gc_register_mark_object(_ref_str);
+  _id_str = rb_str_new_cstr("$id");
+  rb_gc_register_mark_object(_id_str);
+  _db_str = rb_str_new_cstr("$db");
+  rb_gc_register_mark_object(_db_str);
+
   VALUE rb_bson_module = rb_define_module("BSON");
 
   /* Document-class: BSON::ByteBuffer

ext/bson/read.c

@@ -356,6 +356,30 @@ VALUE rb_bson_byte_buffer_get_decimal128_bytes(VALUE self)
   return bytes;
 }
 
+/**
+ * This duplicates the DBRef validation code in DBRef constructor.
+ */
+static int pvt_is_dbref(VALUE doc) {
+  VALUE ref, id, db;
+
+  ref = rb_hash_aref(doc, _ref_str);
+  if (NIL_P(ref) || !RB_TYPE_P(ref, T_STRING)) {
+    return 0;
+  }
+
+  id = rb_hash_aref(doc, _id_str);
+  if (NIL_P(id)) {
+    return 0;
+  }
+
+  db = rb_hash_aref(doc, _db_str);
+  if (!NIL_P(db) && !RB_TYPE_P(db, T_STRING)) {
+    return 0;
+  }
+
+  return 1;
+}
+
 VALUE rb_bson_byte_buffer_get_hash(int argc, VALUE *argv, VALUE self){
   VALUE doc = Qnil;
   byte_buffer_t *b = NULL;
@@ -380,6 +404,11 @@ VALUE rb_bson_byte_buffer_get_hash(int argc, VALUE *argv, VALUE self){
   if (READ_PTR(b) - start_ptr != length) {
     pvt_raise_decode_error(rb_sprintf("Expected to read %d bytes for the hash but read %ld bytes", length, READ_PTR(b) - start_ptr));
   }
+
+  if (pvt_is_dbref(doc)) {
+    VALUE cDBRef = pvt_const_get_2("BSON", "DBRef");
+    doc = rb_funcall(cDBRef, rb_intern("new"), 1, doc);
+  }
 
   return doc;
 }

lib/bson/dbref.rb

@@ -22,27 +22,33 @@ class DBRef < Document
     include JSON
 
     # The constant for the collection reference field.
+    #
+    # @deprecated
     COLLECTION = '$ref'.freeze
 
     # The constant for the id field.
+    #
+    # @deprecated
     ID = '$id'.freeze
 
     # The constant for the database field.
+    #
+    # @deprecated
     DATABASE = '$db'.freeze
 
     # @return [ String ] collection The collection name.
     def collection
-      self[COLLECTION]
+      self['$ref']
     end
 
     # @return [ BSON::ObjectId ] id The referenced document id.
     def id
-      self[ID]
+      self['$id']
     end
 
     # @return [ String ] database The database name.
     def database
-      self[DATABASE]
+      self['$db']
     end
 
     # Get the DBRef as a JSON document
@@ -62,11 +68,22 @@ def as_json(*args)
     #
     # @param [ Hash ] hash the DBRef hash. It must contain $collection and $id.
     def initialize(hash)
-      [COLLECTION, ID].each do |key|
+      %w($ref $id).each do |key|
         unless hash[key]
-          raise ArgumentError, "DBRefs must have a #{key}"
+          raise ArgumentError, "DBRef must have #{key}: #{hash}"
+        end
+      end
+
+      unless hash['$ref'].is_a?(String)
+        raise ArgumentError, "The value for key $ref must be a string, got: #{hash['$ref']}"
+      end
+
+      if db = hash['$db']
+        unless db.is_a?(String)
+          raise ArgumentError, "The value for key $db must be a string, got: #{hash['$db']}"
         end
       end
+
       super(hash)
     end
 
@@ -78,29 +95,9 @@ def initialize(hash)
     # @param [ BSON::ByteBuffer ] buffer The encoded BSON buffer to append to.
     # @param [ true, false ] validating_keys Whether keys should be validated when serializing.
     #
-    # @return [ String ] The raw BSON.
+    # @return [ BSON::ByteBuffer ] The buffer with the encoded object.
     def to_bson(buffer = ByteBuffer.new, validating_keys = Config.validating_keys?)
-      as_json.to_bson(buffer)
-    end
-
-    module ClassMethods
-
-      # Deserialize the hash from BSON, converting to a DBRef if appropriate.
-      #
-      # @param [ String ] buffer The bson representing a hash.
-      #
-      # @return [ Hash, DBRef ] The decoded hash or DBRef.
-      #
-      # @see http://bsonspec.org/#/specification
-      def from_bson(buffer, **options)
-        decoded = super
-        if decoded[COLLECTION]
-          decoded = DBRef.new(decoded)
-        end
-        decoded
-      end
+      as_json.to_bson(buffer, validating_keys)
     end
   end
-
-  ::Hash.send(:extend, DBRef::ClassMethods)
 end

lib/bson/document.rb

@@ -320,6 +320,19 @@ def symbolize_keys!
       raise ArgumentError, 'symbolize_keys! is not supported on BSON::Document instances. Please convert the document to hash first (using #to_h), then call #symbolize_keys! on the Hash instance'
     end
 
+    # Override the Hash implementation of to_bson_normalized_value.
+    #
+    # BSON::Document is already of the correct type and already provides
+    # indifferent access to keys, hence no further conversions are necessary.
+    #
+    # Attempting to perform Hash's conversion on Document instances converts
+    # DBRefs to Documents which is wrong.
+    #
+    # @return [ BSON::Document ] The normalized hash.
+    def to_bson_normalized_value
+      self
+    end
+
     private
 
     def convert_key(key)

lib/bson/ext_json.rb

@@ -139,7 +139,7 @@ module ExtJSON
         return {}
       end
 
-      if hash.key?('$ref')
+      if dbref?(hash)
         # Legacy dbref handling.
         # Note that according to extended json spec, only hash values (but
         # not the top-level BSON document itself) may be of type "dbref".
@@ -148,10 +148,6 @@ module ExtJSON
         # logic to top level hashes doesn't cause harm.
         hash = hash.dup
         ref = hash.delete('$ref')
-        # $ref can be a string value or an ObjectId
-        unless ref.is_a?(String)
-          raise Error::ExtJSONParseError, "Invalid $ref value: #{ref}"
-        end
         # $id, if present, can be anything
         id = hash.delete('$id')
         if id.is_a?(Hash)
@@ -164,13 +160,9 @@ module ExtJSON
         out = {'$ref' => ref, '$id' => id}
         if hash.key?('$db')
           # $db must always be a string, if provided
-          db = hash.delete('$db')
-          unless db.is_a?(String)
-            raise Error::ExtJSONParseError, "Invalid $db value: #{db}"
-          end
-          out['$db'] = db
+          out['$db'] = hash.delete('$db')
         end
-        return out.update(hash)
+        return out.update(parse_hash(hash))
       end
 
       if hash.length == 1
@@ -383,5 +375,14 @@ module ExtJSON
     module_function def create_regexp(pattern, options)
       Regexp::Raw.new(pattern, options)
     end
+
+    module_function def dbref?(hash)
+      if db = hash.key?('$db')
+        unless db.is_a?(String)
+          return false
+        end
+      end
+      return hash['$ref']&.is_a?(String) && hash.key?('$id')
+    end
   end
 end

lib/bson/hash.rb

@@ -71,7 +71,7 @@ def to_bson(buffer = ByteBuffer.new, validating_keys = Config.validating_keys?)
     # @example Convert the hash to a normalized value.
     #   hash.to_bson_normalized_value
     #
-    # @return [ BSON::Document ] The normazlied hash.
+    # @return [ BSON::Document ] The normalized hash.
     #
     # @since 3.0.0
     def to_bson_normalized_value
@@ -128,6 +128,16 @@ def from_bson(buffer, **options)
           if actual_byte_size != expected_byte_size
             raise Error::BSONDecodeError, "Expected hash to take #{expected_byte_size} bytes but it took #{actual_byte_size} bytes"
           end
+
+          if hash['$ref'] && hash['$id']
+            # We're doing implicit decoding here. If the document is an invalid
+            # dbref, we should decode it as a BSON::Document.
+            begin
+              hash = DBRef.new(hash)
+            rescue ArgumentError
+            end
+          end
+
           hash
         end
       end