MONGOID-4911 Range expansion must use $elemMatch to ensure both $gte and $lte match the same array element (#5188)

* MONGOID-4911 make range use elemMatch

* MONGOID-4911 tests evolve_multi

* MONGOID-4911 add range and matcher tests

* MONGOID-4911 add more range tests

* MONGOID-4911 refactor get_serializer

* MONGOID-4911 add matcher test

* MONGOID-4911 fix aliased associations and refactor distinct

* MONGOID-4911 test aliased fields

* MONGOID-4911 refactor using traverse association tree

* MONGOID-4911 update comments and update logic for traverse

* MONGOID-4911 fix belongs_to bug

* MONGOID-4911 update doc string for database_field_name

* MONGOID-4911 update tests and comments

* MONGOID-4911 add tests and update comments

* empty commit

Author: neilshweky

lib/mongoid/contextual/mongo.rb

@@ -712,15 +712,18 @@ def fetch_and_demongoize(d, meth, klass)
           end
         end
 
+        i = 1
+        num_meths = field_name.count('.') + 1
         k = klass
-        meths = field_name.split('.')
-        meths.each_with_index.inject(attrs) do |curr, (meth, i)|
+        curr = attrs.dup
+
+        klass.traverse_association_tree(field_name) do |meth, obj, is_field|
           is_translation = false
-          if !k.fields.key?(meth) && !k.relations.key?(meth)
-            if tr = meth.match(/(.*)_translations\z/)&.captures&.first
-              is_translation = true
-              meth = tr
-            end
+          # If no association or field was found, check if the meth is an
+          # _translations field.
+          if obj.nil? & tr = meth.match(/(.*)_translations\z/)&.captures&.first
+            is_translation = true
+            meth = tr
           end
 
           # 1. If curr is an array fetch from all elements in the array.
@@ -733,11 +736,11 @@ def fetch_and_demongoize(d, meth, klass)
           # 3. If the meth is an _translations field, do not demongoize the
           #    value so the full hash is returned.
           # 4. Otherwise, fetch and demongoize the value for the key meth.
-          if curr.is_a? Array
+          curr = if curr.is_a? Array
             res = curr.map { |x| fetch_and_demongoize(x, meth, k) }
             res.empty? ? nil : res
           elsif !is_translation && k.fields[meth]&.localized?
-            if i < meths.length-1
+            if i < num_meths
               curr.try(:fetch, meth, nil)
             else
               fetch_and_demongoize(curr, meth, k)
@@ -746,18 +749,15 @@ def fetch_and_demongoize(d, meth, klass)
             curr.try(:fetch, meth, nil)
           else
             fetch_and_demongoize(curr, meth, k)
-          end.tap do
-            if as = k.try(:aliased_associations)
-              if a = as.fetch(meth, nil)
-                meth = a
-              end
-            end
+          end
 
-            if relation = k.relations[meth]
-              k = relation.klass
-            end
+          # If it's a relation, update the current klass with the relation klass.
+          if !is_field && !obj.nil?
+            k = obj.klass
           end
+          i += 1
         end
+        curr
       end
 
       # Recursively demongoize the given value. This method recursively traverses
@@ -770,29 +770,20 @@ def fetch_and_demongoize(d, meth, klass)
       #
       # @return [ Object ] The demongoized value.
       def recursive_demongoize(field_name, value, is_translation)
-        k = klass
-        field_name.split('.').each do |meth|
-          if as = k.try(:aliased_associations)
-            if a = as.fetch(meth, nil)
-              meth = a.to_s
-            end
-          end
-
-          if relation = k.relations[meth]
-            k = relation.klass
-          elsif field = k.fields[meth]
-            # If it's a localized field that's not a hash, don't demongoize
-            # again, we already have the translation. If it's an _translation
-            # field, don't demongoize, we want the full hash not just a
-            # specific translation.
-            if field.localized? && (!value.is_a?(Hash) || is_translation)
-              return value.class.demongoize(value)
-            else
-              return field.demongoize(value)
-            end
+        field = klass.traverse_association_tree(field_name)
+
+        if field
+          # If it's a localized field that's not a hash, don't demongoize
+          # again, we already have the translation. If it's an _translations
+          # field, don't demongoize, we want the full hash not just a
+          # specific translation.
+          if field.localized? && (!value.is_a?(Hash) || is_translation)
+            value.class.demongoize(value)
           else
-            return value.class.demongoize(value)
+            field.demongoize(value)
           end
+        else
+          value.class.demongoize(value)
         end
       end
     end

lib/mongoid/criteria.rb

@@ -230,7 +230,7 @@ def initialize(klass)
       @klass = klass
       @embedded = nil
       @none = nil
-      klass ? super(klass.aliased_fields, klass.fields, klass.relations) : super({}, {}, {})
+      klass ? super(klass.aliased_fields, klass.fields, klass.relations, klass.aliased_associations) : super({}, {}, {}, {})
     end
 
     # Merges another object with this +Criteria+ and returns a new criteria.

lib/mongoid/criteria/queryable.rb

@@ -60,13 +60,14 @@ def ==(other)
       # @param [ Hash ] aliases The optional field aliases.
       # @param [ Hash ] serializers The optional field serializers.
       # @param [ Hash ] associations The optional associations.
+      # @param [ Hash ] aliased_associations The optional aliased associations.
       # @param [ Symbol ] driver The driver being used.
       #
       # @api private
-      def initialize(aliases = {}, serializers = {}, associations = {}, driver = :mongo)
+      def initialize(aliases = {}, serializers = {}, associations = {}, aliased_associations = {}, driver = :mongo)
         @aliases, @driver, @serializers = aliases, driver.to_sym, serializers
-        @options = Options.new(aliases, serializers)
-        @selector = Selector.new(aliases, serializers, associations)
+        @options = Options.new(aliases, serializers, associations, aliased_associations)
+        @selector = Selector.new(aliases, serializers, associations, aliased_associations)
         @pipeline = Pipeline.new(aliases)
         @aggregating = nil
         yield(self) if block_given?

lib/mongoid/criteria/queryable/options.rb

@@ -84,7 +84,7 @@ def to_pipeline
         #
         # @return [ Options ] The copied options.
         def __deep_copy__
-          self.class.new(aliases, serializers) do |copy|
+          self.class.new(aliases, serializers, associations, aliased_associations) do |copy|
             each_pair do |key, value|
               copy.merge!(key => value.__deep_copy__)
             end

lib/mongoid/criteria/queryable/selector.rb

@@ -53,8 +53,7 @@ def store(key, value)
             store_name = name
             store_value = evolve_multi(value)
           else
-            store_name = localized_key(name, serializer)
-            store_value = evolve(serializer, value)
+            store_name, store_value = store_creds(name, serializer, value)
           end
           super(store_name, store_value)
         end
@@ -74,6 +73,24 @@ def to_pipeline
 
         private
 
+        # Get the store name and store value. If the value is of type range,
+        # we need may need to change the store_name as well as the store_value,
+        # therefore, we cannot just use the evole method.
+        #
+        # @param [ String ] name The name of the field.
+        # @param [ Object ] serializer The optional serializer for the field.
+        # @param [ Object ] value The value to serialize.
+        #
+        # @return [ Array<String, String> ] The store name and store value.
+        def store_creds(name, serializer, value)
+          store_name = localized_key(name, serializer)
+          if Range === value
+            evolve_range(store_name, serializer, value)
+          else
+            [ store_name, evolve(serializer, value) ]
+          end
+        end
+
         # Evolves a multi-list selection, like an $and or $or criterion, and
         # performs the necessary serialization.
         #
@@ -103,11 +120,10 @@ def evolve_multi(specs)
               # some reason, although per its documentation Smash supposedly
               # owns both.
               name, serializer = storage_pair(key)
-              final_key = localized_key(name, serializer)
               # This performs type conversions on the value and transformations
               # that depend on the type of the field that the value is stored
               # in, but not transformations that have to do with query shape.
-              evolved_value = evolve(serializer, value)
+              final_key, evolved_value = store_creds(name, serializer, value)
 
               # This builds a query shape around the value, when the query
               # involves complex keys. For example, {:foo.lt => 5} produces
@@ -140,7 +156,7 @@ def evolve(serializer, value)
           when Array
             evolve_array(serializer, value)
           when Range
-            evolve_range(serializer, value)
+            value.__evolve_range__(serializer: serializer)
           else
             (serializer || value.class).evolve(value)
           end
@@ -184,16 +200,71 @@ def evolve_hash(serializer, value)
           end
         end
 
-        # Evolve a single key selection with range values.
+        # Evolve a single key selection with range values. This method traverses
+        # the association tree to build a query for the given value and
+        # serializer. There are three parts to the query here:
         #
-        # @param [ Object ] serializer The optional serializer for the field.
-        # @param [ Range ] value The Range to serialize.
+        # (1) "klass.child.gchild" => {
+        #       "$elemMatch" => {
+        #     (2) "ggchild.field" => (3) { "$gte" => 6, "$lte" => 10 }
+        #       }
+        #     }
+        # (1) The first n fields are dotted together until the last
+        #     embeds_many or field of type array. In the above case, gchild
+        #     would be an embeds_many or Array, and ggchild would be an
+        #     embeds_one or a hash.
+        # (2) The last fields are used inside the $elemMatch. This one is
+        #     actually optional, and will be ignored if the last field is an
+        #     array or embeds_many. If the last field is an array (1), (2) and
+        #     (3) will look like:
+        #
+        #       "klass.child.gchild.ggchild.field" => {
+        #         { "$elemMatch" => { "$gte" => 6, "$lte" => 10 } }
+        #       }
+        #
+        # (3) This is calculated by:
         #
-        # @return [ Range ] The serialized Range.
+        #       value.__evolve_range__(serializer: serializer).
         #
         # @api private
-        def evolve_range(serializer, value)
-          value.__evolve_range__(serializer: serializer)
+        #
+        # @param [ String ] key The to store the range for.
+        # @param [ Object ] serializer The optional serializer for the field.
+        # @param [ Range ] value The Range to serialize.
+        #
+        # @return [ Array<String, Hash> ] The store name and serialized Range.
+        def evolve_range(key, serializer, value)
+          v = value.__evolve_range__(serializer: serializer)
+          assocs = []
+          Fields.traverse_association_tree(key, serializers, associations, aliased_associations) do |meth, obj, is_field|
+            assocs.push([meth, obj, is_field])
+          end
+
+          # Iterate backwards until you get a field with type
+          # Array or an embeds_many association.
+          inner_key = ""
+          loop do
+            # If there are no arrays or embeds_many associations, just return
+            # the key and value without $elemMatch.
+            return [ key, v ] if assocs.empty?
+
+            meth, obj, is_field = assocs.last
+            break if (is_field && obj.type == Array) || (!is_field && obj.is_a?(Association::Embedded::EmbedsMany))
+
+            assocs.pop
+            inner_key = "#{meth}.#{inner_key}"
+          end
+
+          # If the last array or embeds_many association is the last field,
+          # the inner key (2) is ignored, and the outer key (1) is the original
+          # key.
+          if inner_key.blank?
+            [ key, { "$elemMatch" => v }]
+          else
+            store_key = assocs.map(&:first).join('.')
+            store_value = { "$elemMatch" => { inner_key.chop => v } }
+            [ store_key,  store_value ]
+          end
         end
 
         # Determines if the selection is a multi-select, like an $and or $or or $nor

lib/mongoid/criteria/queryable/smash.rb

@@ -16,14 +16,17 @@ class Smash < Hash
         # @attribute [r] associations The associations.
         attr_reader :associations
 
+        # @attribute [r] aliased_associations The aliased_associations.
+        attr_reader :aliased_associations
+
         # Perform a deep copy of the smash.
         #
         # @example Perform a deep copy.
         #   smash.__deep_copy__
         #
         # @return [ Smash ] The copied hash.
         def __deep_copy__
-          self.class.new(aliases, serializers, associations) do |copy|
+          self.class.new(aliases, serializers, associations, aliased_associations) do |copy|
             each_pair do |key, value|
               copy.store(key, value.__deep_copy__)
             end
@@ -41,8 +44,15 @@ def __deep_copy__
         #   responsible for serializing values. The keys of the hash must be
         #   strings that match the field name, and the values must respond to
         #   #localized? and #evolve(object).
-        def initialize(aliases = {}, serializers = {}, associations = {})
-          @aliases, @serializers, @associations = aliases, serializers, associations
+        # @param [ Hash ] associations An optional hash of names to association
+        #   objects.
+        # @param [ Hash ] aliased_associations An optional hash of mappings from
+        #   aliases for associations to their actual field names in the database.
+        def initialize(aliases = {}, serializers = {}, associations = {}, aliased_associations = {})
+          @aliases = aliases
+          @serializers = serializers
+          @associations = associations
+          @aliased_associations = aliased_associations
           yield(self) if block_given?
         end
 
@@ -91,7 +101,7 @@ def localized_key(name, serializer)
         #   serializer.
         def storage_pair(key)
           field = key.to_s
-          name = aliases[field] || field
+          name = Fields.database_field_name(field, associations, aliases, aliased_associations)
           [ name, get_serializer(name) ]
         end
 
@@ -109,21 +119,7 @@ def get_serializer(name)
           if s = serializers[name]
             s
           else
-            klass = nil
-            serializer = nil
-            name.split('.').each do |meth|
-              fs = klass ? klass.fields : serializers
-              if field = fs[meth]
-                serializer = field
-              else
-                serializer = nil
-                rs = klass ? klass.associations : associations
-                if rel = rs[meth]
-                  klass = rel.klass
-                end
-              end
-            end
-            serializer
+            Fields.traverse_association_tree(name, serializers, associations, aliased_associations)
           end
         end
       end