Fix MONGOID-5020 Condition lifting breaks with symbol operators on Ruby <= 2.6 (#4915)

* MONGOID-5020 test cases for operators with symbols

* MONGOID-5020 convert hashes to indifferent access prior to query manipulation

* MONGOID-5020 queries are now more uniform

* MONGOID-5020 combine with $eq when conditions are given in the same argument

* MONGOID-5020 test all matrix of string/symbol operator types

* fix straggler test

* document the situation

* document $eq lifting

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

Author: p-mongo

docs/tutorials/mongoid-upgrade.txt

@@ -18,10 +18,123 @@ please consult GitHub releases for detailed release notes and JIRA for
 the complete list of issues fixed in each release, including bug fixes.
 
 
+Upgrading to Mongoid 7.3
+========================
+
+The following sections describe significant changes in Mongoid 7.3.
+
+Field Operator Stringification
+------------------------------
+
+Minor change: the ``and`` logical operator now stringifies field operators
+in its arguments. Mongoid 7.3 behavior:
+
+.. code-block:: ruby
+
+  Band.and(year: {'$in': [2020]})
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{"$in"=>[2020]}}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+Mongoid 7.2 behavior:
+
+.. code-block:: ruby
+
+  Band.and(year: {'$in': [2020]})
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{:$in=>[2020]}}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+Note that this stringification does not yet happen for all query construction
+paths. For example, ``where`` does not stringify operators in both Mongoid 7.3
+and Mongoid 7.2:
+
+.. code-block:: ruby
+
+  Band.where(year: {'$in': [2020]})
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{:$in=>[2020]}}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+It is expected that over time, all operators will stringify the keys in their
+arguments.
+
+
+Condition Combination Using ``$eq``
+-----------------------------------
+
+Minor change: when using the ``and`` method on ``Criteria`` objects and
+providing multiple conditions on the same field in the same argument to
+``and``, conditions may be combined using ``$eq`` instead of ``$and``.
+Mongoid 7.3 behavior:
+
+.. code-block:: ruby
+
+  Band.and(year: 2020, :year.gt => 1960)
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{"$eq"=>2020, "$gt"=>1960}}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+  Band.and(:year.gt => 1960, year: 2020)
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{"$gt"=>1960, "$eq"=>2020}}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+Mongoid 7.2 behavior:
+
+.. code-block:: ruby
+
+  Band.and(year: 2020, :year.gt => 1960)
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>2020, "$and"=>[{"year"=>{"$gt"=>1960}}]}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+  Band.and(:year.gt => 1960, year: 2020)
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{"$gt"=>1960}, "$and"=>[{"year"=>2020}]}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+This combination is not yet performed for ``where`` and other query methods:
+
+.. code-block:: ruby
+
+  Band.where(:year.gt => 1960, year: 2020)
+  # => 
+  # #<Mongoid::Criteria
+  #   selector: {"year"=>{"$gt"=>1960}, "$and"=>[{"year"=>2020}]}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+Tt is expected that in the future other query methods will also favor
+using ``$eq`` over ``$and``.
+
+
 Upgrading to Mongoid 7.2
 ========================
 
-The following sections describe major changes in Mongoid 7.2.
+The following sections describe significant changes in Mongoid 7.2.
 
 Embedded Document Matching
 --------------------------
@@ -416,7 +529,7 @@ of the legacy query cache, see :ref:`the query cache documentation <query-cache>
 Upgrading to Mongoid 7.1
 ========================
 
-The following sections describe major changes in Mongoid 7.1.
+The following sections describe significant changes in Mongoid 7.1.
 
 Condition Combination
 ---------------------
@@ -448,6 +561,45 @@ Corresponding Mongoid 7.0 behavior:
     #   class:    Band
     #   embedded: false>
 
+**Known issue:** When using Ruby 2.6 and lower, when adding multiple conditions
+on the same field using the same operator, the operator must be given as a
+string, not as a symbol. The following invocations fail:
+
+.. code-block:: ruby
+
+  Band.and({year: {'$in': [2020]}}, {year: {'$in': [2020]}})
+  # Traceback (most recent call last):
+  #         2: from (irb):10
+  #         1: from (irb):10:in `rescue in irb_binding'
+  # NoMethodError (undefined method `start_with?' for :$in:Symbol)
+
+  Band.and(year: {'$in': [2020]}).and(year: {'$in': [2020]})
+  # Traceback (most recent call last):
+  #         2: from (irb):11
+  #         1: from (irb):11:in `rescue in irb_binding'
+  # NoMethodError (undefined method `start_with?' for :$in:Symbol)
+
+Use string keys instead:
+
+.. code-block:: ruby
+
+  # Band.and({year: {'$in' => [2020]}}, {year: {'$in' => [2020]}})
+  # => #<Mongoid::Criteria
+  #   selector: {"year"=>{"$in"=>[2020]}, "$and"=>[{"year"=>{"$in"=>[2020]}}]}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+  Band.and(year: {'$in' => [2020]}).and(year: {'$in' => [2020]})
+  # => #<Mongoid::Criteria
+  #   selector: {"year"=>{"$in"=>[2020]}, "$and"=>[{"year"=>{"$in"=>[2020]}}]}
+  #   options:  {}
+  #   class:    Band
+  #   embedded: false>
+
+This issue is rectified in Mongoid 7.3.
+
+
 Logical Operations
 ------------------
 

lib/mongoid/criteria/queryable/mergeable.rb

@@ -227,7 +227,8 @@ def __multi__(criteria, operator)
         end
 
         # Takes a criteria hash and expands Key objects into hashes containing
-        # MQL corresponding to said key objects.
+        # MQL corresponding to said key objects. Also converts the input to
+        # BSON::Document to permit indifferent access.
         #
         # Ruby does not permit multiple symbol operators. For example,
         # {:foo.gt => 1, :foo.gt => 2} is collapsed to {:foo.gt => 2} by the
@@ -237,15 +238,19 @@ def __multi__(criteria, operator)
         # Similarly, this method should never need to expand a literal value
         # and an operator at the same time.
         #
+        # This method effectively converts symbol keys to string keys in
+        # the input +expr+, such that the downstream code can assume that
+        # conditions always contain string keys.
+        #
         # @param [ Hash ] expr Criteria including Key instances.
         #
-        # @return [ Hash ] The expanded criteria.
+        # @return [ BSON::Document ] The expanded criteria.
         private def _mongoid_expand_keys(expr)
           unless expr.is_a?(Hash)
             raise ArgumentError, 'Argument must be a Hash'
           end
 
-          result = {}
+          result = BSON::Document.new
           expr.each do |field, value|
             field.__expr_part__(value.__expand_complex__).each do |k, v|
               if result[k]

spec/integration/matcher_operator_data/in.yml

@@ -192,3 +192,19 @@
         end: 20
         excl: false
   error: true
+
+- name: scalar field - symbol operator - matches
+  document:
+    count: 10
+  query:
+    count:
+      :$in: [10, 11]
+  matches: true
+
+- name: scalar field - symbol operator - does not match
+  document:
+    count: 8
+  query:
+    count:
+      :$in: [10, 11]
+  matches: false

spec/mongoid/criteria/queryable/mergeable_spec.rb

@@ -44,7 +44,7 @@
 
   describe '#_mongoid_expand_keys' do
     it 'expands simple keys' do
-      query.send(:_mongoid_expand_keys, {a: 1}).should == {a: 1}
+      query.send(:_mongoid_expand_keys, {a: 1}).should == {'a' => 1}
     end
 
     let(:gt) do

spec/mongoid/criteria/queryable/selectable_logical_spec.rb

@@ -417,6 +417,40 @@
           it_behaves_like 'returns a cloned query'
         end
 
+        context 'when criteria use operators' do
+          shared_examples 'behave correctly' do
+            let(:selection) do
+              query.and(
+                { field: {first_operator => [ 1, 2 ] }},
+                { field: {second_operator => [ 3, 4 ] }},
+              )
+            end
+
+            it "combines via $and operator and stringifies all keys" do
+              expect(selection.selector).to eq({
+                "field" => {'$in' => [ 1, 2 ]},
+                "$and" => [
+                  { "field" => {'$in' => [ 3, 4 ] }}
+                ]
+              })
+            end
+          end
+
+          [
+            ['$in', '$in'],
+            [:$in, '$in'],
+            ['$in', :$in],
+            [:$in, :$in],
+          ].each do |first_operator, second_operator|
+            context "when first operator is #{first_operator.inspect} and second operator is #{second_operator.inspect}" do
+              let(:first_operator) { first_operator }
+              let(:second_operator) { second_operator }
+
+              include_examples 'behave correctly'
+            end
+          end
+        end
+
         context 'when criteria are handled via Key' do
           shared_examples_for 'adds the conditions to top level' do
 
@@ -467,12 +501,23 @@
             it_behaves_like 'returns a cloned query'
           end
 
+          shared_examples_for 'combines conditions with $eq' do
+
+            it "combines conditions with $eq" do
+              expect(selection.selector).to eq({
+                "field" => {'$eq' => 3, '$lt' => 5},
+              })
+            end
+
+            it_behaves_like 'returns a cloned query'
+          end
+
           context 'criteria are provided in the same hash' do
             let(:selection) do
               query.send(tested_method, :field => 3, :field.lt => 5)
             end
 
-            it_behaves_like 'combines conditions with $and'
+            it_behaves_like 'combines conditions with $eq'
           end
 
           context 'criteria are provided in separate hashes' do
@@ -505,12 +550,23 @@
             it_behaves_like 'returns a cloned query'
           end
 
+          shared_examples_for 'combines conditions with $eq' do
+
+            it "combines conditions with $eq" do
+              expect(selection.selector).to eq({
+                "field" => {'$gt' => 3, '$eq' => 5},
+              })
+            end
+
+            it_behaves_like 'returns a cloned query'
+          end
+
           context 'criteria are provided in the same hash' do
             let(:selection) do
               query.send(tested_method, :field.gt => 3, :field => 5)
             end
 
-            it_behaves_like 'combines conditions with $and'
+            it_behaves_like 'combines conditions with $eq'
           end
 
           context 'criteria are provided in separate hashes' do
@@ -1270,14 +1326,14 @@
           it_behaves_like 'returns a cloned query'
         end
 
-        shared_examples_for 'adds one condition' do
+        shared_examples_for 'combines conditions with $eq' do
 
-          it "adds one condition" do
+          it "combines conditions with $eq" do
             expect(selection.selector).to eq({
-              'field' => 3,
-              '$and' => [
-                {'field' => {'$lt' => 5}},
-              ],
+              'field' => {
+                '$eq' => 3,
+                '$lt' => 5,
+              },
             })
           end
 
@@ -1289,7 +1345,7 @@
             query.send(tested_method, :field => 3, :field.lt => 5)
           end
 
-          it_behaves_like 'adds one condition'
+          it_behaves_like 'combines conditions with $eq'
         end
 
         context 'criteria are provided in separate hashes' do
@@ -1324,13 +1380,12 @@
           it_behaves_like 'returns a cloned query'
         end
 
-        shared_examples_for 'adds one condition' do
+        shared_examples_for 'combines conditions with $eq' do
 
-          it "adds one condition" do
-            expect(selection.selector).to eq({
-              'field' => {'$gt' => 3},
-              '$and' => ['field' => 5],
-            })
+          it "combines conditions with $eq" do
+            expect(selection.selector).to eq(
+              'field' => {'$gt' => 3, '$eq' => 5},
+            )
           end
 
           it_behaves_like 'returns a cloned query'
@@ -1341,7 +1396,7 @@
             query.send(tested_method, :field.gt => 3, :field => 5)
           end
 
-          it_behaves_like 'adds one condition'
+          it_behaves_like 'combines conditions with $eq'
         end
 
         context 'criteria are provided in separate hashes' do