From e5a6494eb5c4ea09ab2b10a4a8b774448cbad8f9 Mon Sep 17 00:00:00 2001 From: Jiajia Wang Date: Thu, 20 May 2021 20:47:43 +1000 Subject: [PATCH] [Fix #795] Change implementation of Order param (#798) * [Fix #795] **(Breaking)** Change implementation of Order param Use Array to store the value of Chewy::Search::Parameters::Order. To allow multiple sorting options that may have the same key name. For example script based sorting whose key will always be `_script`. --- CHANGELOG.md | 8 ++++++ lib/chewy/search/parameters/order.rb | 25 +++++-------------- spec/chewy/search/parameters/order_spec.rb | 29 ++++++++++++++-------- spec/chewy/search/parameters_spec.rb | 2 +- spec/chewy/search/request_spec.rb | 2 +- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4bde0d85..be4f4421c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/chewy/search/parameters/order.rb b/lib/chewy/search/parameters/order.rb index f30a7d8f1..bae4d9872 100644 --- a/lib/chewy/search/parameters/order.rb +++ b/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,20 +28,7 @@ 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 @@ -49,13 +36,13 @@ def ==(other) 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 diff --git a/spec/chewy/search/parameters/order_spec.rb b/spec/chewy/search/parameters/order_spec.rb index fc556b352..9bc7a042a 100644 --- a/spec/chewy/search/parameters/order_spec.rb +++ b/spec/chewy/search/parameters/order_spec.rb @@ -4,26 +4,29 @@ 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 @@ -31,7 +34,7 @@ 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 diff --git a/spec/chewy/search/parameters_spec.rb b/spec/chewy/search/parameters_spec.rb index f14fcf9b2..eb1faca24 100644 --- a/spec/chewy/search/parameters_spec.rb +++ b/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 diff --git a/spec/chewy/search/request_spec.rb b/spec/chewy/search/request_spec.rb index e077716e4..2ac7790c0 100644 --- a/spec/chewy/search/request_spec.rb +++ b/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 } }