[Fix #795] Change implementation of Order param

CHANGELOG.md

@@ -6,6 +6,13 @@
 
 ### Changes
 
+  * [#795](https://github.com/toptal/chewy/issues/795): **(Breaking)** Change the Chewy::Search::Parameters::Order implementation to use Array ([@jiajiawang][]):
+    * To allow multiple sorting options that may have the same key name. For example script based sorting whose key will always be `_script`.
+    * Behaviour change of chained `order` calls.
+      * e.g. `.order(_script: {a: 1}).order(_script: {b: 2})`
+        * Before `{:sort=>[{"_script"=>{:b=>2}}]}`
+        * After  `{:sort=>[{"_script"=>{:a=>1}},{"_script"=>{:b=>2}}]}`
+
   * [#654](https://github.com/toptal/chewy/issues/654): Add helpers and matchers for testing ([@Vitalina-Vakulchyk][]):
     * `mock_elasticsearch_response` helpers both Rspec and Minitest - to mock elasticsearch response
     * `mock_elasticsearch_response_sources` helpers both Rspec and Minitest - to mock elasticsearch response sources
@@ -637,6 +644,7 @@
 [@inbeom]: https://github.com/inbeom
 [@jesjos]: https://github.com/jesjos
 [@JF-Lalonde]: https://github.com/JF-Lalonde
+[@jiajiawang]: https://github.com/jiajiawang
 [@jimmybaker]: https://github.com/jimmybaker
 [@jirikolarik]: https://github.com/jirikolarik
 [@jirutka]: https://github.com/jirutka

lib/chewy/search/parameters/order.rb

@@ -17,7 +17,7 @@ class Order < Storage
         # @param other_value [Object] any acceptable storage value
         # @return [Object] updated value
         def update!(other_value)
-          value.merge!(normalize(other_value))
+          value.concat(normalize(other_value))
         end
 
         # Size requires specialized rendering logic, it should return
@@ -28,34 +28,21 @@ def update!(other_value)
         def render
           return if value.blank?
 
-          sort = value.map do |(field, options)|
-            options ? {field => options} : field
-          end
-          {sort: sort}
-        end
-
-        # Comparison also reqires additional logic. Hashes are compared
-        # orderlessly, but for `sort` parameter oder is important, so we
-        # compare hash key collections additionally.
-        #
-        # @see Chewy::Search::Parameters::Storage#==
-        # @return [true, false]
-        def ==(other)
-          super && value.keys == other.value.keys
+          {sort: value}
         end
 
       private
 
         def normalize(value)
           case value
           when Array
-            value.each_with_object({}) do |sv, res|
-              res.merge!(normalize(sv))
+            value.each_with_object([]) do |sv, res|
+              res.concat(normalize(sv))
             end
           when Hash
-            value.stringify_keys
+            [value.stringify_keys]
           else
-            value.present? ? {value.to_s => nil} : {}
+            value.present? ? [value.to_s] : []
           end
         end
       end

spec/chewy/search/parameters/order_spec.rb

@@ -4,34 +4,37 @@
   subject { described_class.new(%i[foo bar]) }
 
   describe '#initialize' do
-    specify { expect(described_class.new.value).to eq({}) }
-    specify { expect(described_class.new(nil).value).to eq({}) }
-    specify { expect(described_class.new('').value).to eq({}) }
-    specify { expect(described_class.new(42).value).to eq('42' => nil) }
-    specify { expect(described_class.new([42, 43]).value).to eq('42' => nil, '43' => nil) }
-    specify { expect(described_class.new(a: 1).value).to eq('a' => 1) }
-    specify { expect(described_class.new(['', 43, {a: 1}]).value).to eq('a' => 1, '43' => nil) }
+    specify { expect(described_class.new.value).to eq([]) }
+    specify { expect(described_class.new(nil).value).to eq([]) }
+    specify { expect(described_class.new('').value).to eq([]) }
+    specify { expect(described_class.new(42).value).to eq(['42']) }
+    specify { expect(described_class.new([42, 43]).value).to eq(%w[42 43]) }
+    specify { expect(described_class.new([42, 42]).value).to eq(%w[42 42]) }
+    specify { expect(described_class.new([42, [43, 44]]).value).to eq(%w[42 43 44]) }
+    specify { expect(described_class.new(a: 1).value).to eq([{'a' => 1}]) }
+    specify { expect(described_class.new(['a', {a: 1}, {a: 2}]).value).to eq(['a', {'a' => 1}, {'a' => 2}]) }
+    specify { expect(described_class.new(['', 43, {a: 1}]).value).to eq(['43', {'a' => 1}]) }
   end
 
   describe '#replace!' do
     specify do
       expect { subject.replace!(foo: {}) }
         .to change { subject.value }
-        .from('foo' => nil, 'bar' => nil).to('foo' => {})
+        .from(%w[foo bar]).to([{'foo' => {}}])
     end
 
     specify do
       expect { subject.replace!(nil) }
         .to change { subject.value }
-        .from('foo' => nil, 'bar' => nil).to({})
+        .from(%w[foo bar]).to([])
     end
   end
 
   describe '#update!' do
     specify do
       expect { subject.update!(foo: {}) }
         .to change { subject.value }
-        .from('foo' => nil, 'bar' => nil).to('foo' => {}, 'bar' => nil)
+        .from(%w[foo bar]).to(['foo', 'bar', {'foo' => {}}])
     end
 
     specify { expect { subject.update!(nil) }.not_to change { subject.value } }
@@ -41,7 +44,7 @@
     specify do
       expect { subject.merge!(described_class.new(foo: {})) }
         .to change { subject.value }
-        .from('foo' => nil, 'bar' => nil).to('foo' => {}, 'bar' => nil)
+        .from(%w[foo bar]).to(['foo', 'bar', {'foo' => {}}])
     end
 
     specify { expect { subject.merge!(described_class.new) }.not_to change { subject.value } }
@@ -51,6 +54,7 @@
     specify { expect(described_class.new.render).to be_nil }
     specify { expect(described_class.new(:foo).render).to eq(sort: ['foo']) }
     specify { expect(described_class.new([:foo, {bar: 42}, :baz]).render).to eq(sort: ['foo', {'bar' => 42}, 'baz']) }
+    specify { expect(described_class.new([:foo, {bar: 42}, {bar: 43}, :baz]).render).to eq(sort: ['foo', {'bar' => 42}, {'bar' => 43}, 'baz']) }
   end
 
   describe '#==' do
@@ -59,7 +63,10 @@
     specify { expect(described_class.new(:foo)).not_to eq(described_class.new(:bar)) }
     specify { expect(described_class.new(%i[foo bar])).to eq(described_class.new(%i[foo bar])) }
     specify { expect(described_class.new(%i[foo bar])).not_to eq(described_class.new(%i[bar foo])) }
+    specify { expect(described_class.new(%i[foo foo])).not_to eq(described_class.new(%i[foo])) }
     specify { expect(described_class.new(foo: {a: 42})).to eq(described_class.new(foo: {a: 42})) }
     specify { expect(described_class.new(foo: {a: 42})).not_to eq(described_class.new(foo: {b: 42})) }
+    specify { expect(described_class.new(['foo', {'foo' => 42}])).not_to eq(described_class.new([{'foo' => 42}, 'foo'])) }
+    specify { expect(described_class.new([{'foo' => 42}, {'foo' => 43}])).not_to eq(described_class.new([{'foo' => 43}, {'foo' => 42}])) }
   end
 end

spec/chewy/search/parameters_spec.rb

@@ -13,7 +13,7 @@
 
     specify { expect(subject.storages[:limit]).to equal(limit) }
     specify { expect(subject.storages[:limit].value).to eq(3) }
-    specify { expect(subject.storages[:order].value).to eq('foo' => nil) }
+    specify { expect(subject.storages[:order].value).to eq(['foo']) }
 
     specify { expect { described_class.new(offset: limit) }.to raise_error(TypeError) }
   end

spec/chewy/search/request_spec.rb

@@ -177,7 +177,7 @@
   describe '#order' do
     specify { expect(subject.order(:foo).render[:body]).to include(sort: ['foo']) }
     specify { expect(subject.order(foo: 42).order(nil).render[:body]).to include(sort: ['foo' => 42]) }
-    specify { expect(subject.order(foo: 42).order(foo: 43).render[:body]).to include(sort: ['foo' => 43]) }
+    specify { expect(subject.order(foo: 42).order(foo: 43).render[:body]).to include(sort: [{'foo' => 42}, {'foo' => 43}]) }
     specify { expect(subject.order(:foo).order(:bar, :baz).render[:body]).to include(sort: %w[foo bar baz]) }
     specify { expect(subject.order(nil).render[:body]).to be_blank }
     specify { expect { subject.order(:foo) }.not_to change { subject.render } }