From 0d236c8fed106858bedba55f0fe1d9016a5b386c Mon Sep 17 00:00:00 2001 From: Emily Stolfo Date: Thu, 13 Sep 2018 15:20:44 +0200 Subject: [PATCH 1/2] [MODEL] Ensure that specified ActiveRecord order is not overwritten by Elasticsearch query order --- .../lib/elasticsearch/model/adapters/active_record.rb | 6 +++++- .../elasticsearch/model/adapters/active_record_spec.rb | 2 ++ .../test/integration/active_record_basic_test.rb | 7 +++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb b/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb index ae1b38f56..1edaa5003 100644 --- a/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb +++ b/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb @@ -35,7 +35,11 @@ def records else self.__send__(:exec_queries) end - @records.sort_by { |record| hits.index { |hit| hit['_id'].to_s == record.id.to_s } } + if !self.order_values.present? + @records.sort_by { |record| hits.index { |hit| hit['_id'].to_s == record.id.to_s } } + else + @records + end end if self end diff --git a/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb b/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb index 19ad6f9e9..2db294455 100644 --- a/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb +++ b/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb @@ -95,6 +95,7 @@ class DummyClassForActiveRecord; end before do records.instance_variable_set(:@records, records) + allow(records).to receive(:order_values).and_return([]) end it 'reorders the records based on hits order' do @@ -111,6 +112,7 @@ class DummyClassForActiveRecord; end before do records.instance_variable_set(:@records, records) expect(instance.records).to receive(:order).and_return(records) + allow(records).to receive(:order_values).and_return([]) end it 'reorders the records based on hits order' do diff --git a/elasticsearch-model/test/integration/active_record_basic_test.rb b/elasticsearch-model/test/integration/active_record_basic_test.rb index 90abeb47a..d7ecb0586 100644 --- a/elasticsearch-model/test/integration/active_record_basic_test.rb +++ b/elasticsearch-model/test/integration/active_record_basic_test.rb @@ -231,6 +231,13 @@ def as_indexed_json(options = {}) end end + should "allow ordering following any method chain in SQL" do + if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 4 + response = Article.search query: { match: { title: { query: 'test' } } } + assert_equal 'Testing Coding', response.records.distinct.order(id: :desc).first.title + end + end + should "allow dot access to response" do response = Article.search query: { match: { title: { query: 'test' } } }, aggregations: { From 03851d1234b081fbf742b313978c8b661fb1326f Mon Sep 17 00:00:00 2001 From: Emily Stolfo Date: Fri, 14 Sep 2018 13:17:52 +0200 Subject: [PATCH 2/2] [MODEL] Remove interception of #order method, as ordering is handled by checking order_values in #to_a --- .../model/adapters/active_record.rb | 24 ------------------- .../model/adapters/active_record_spec.rb | 8 +++---- 2 files changed, 3 insertions(+), 29 deletions(-) diff --git a/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb b/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb index 1edaa5003..3fb3f987e 100644 --- a/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb +++ b/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb @@ -51,30 +51,6 @@ def records def load records.__send__(:load) end - - # Intercept call to the `order` method, so we can ignore the order from Elasticsearch - # - def order(*args) - sql_records = records.__send__ :order, *args - - # Redefine the `to_a` method to the original one - # - sql_records.instance_exec do - ar_records_method_name = :to_a - ar_records_method_name = :records if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 5 - - define_singleton_method(ar_records_method_name) do - if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 4 - self.load - else - self.__send__(:exec_queries) - end - @records - end - end - - sql_records - end end module Callbacks diff --git a/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb b/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb index 2db294455..7d18f0b72 100644 --- a/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb +++ b/elasticsearch-model/spec/elasticsearch/model/adapters/active_record_spec.rb @@ -66,6 +66,7 @@ class DummyClassForActiveRecord; end let(:instance) do model.tap do |inst| allow(inst).to receive(:klass).and_return(double('class', primary_key: :some_key, where: records)).at_least(:once) + allow(inst).to receive(:order).and_return(double('class', primary_key: :some_key, where: records)).at_least(:once) end end @@ -110,15 +111,12 @@ class DummyClassForActiveRecord; end context 'when the records have a different order than the hits' do before do - records.instance_variable_set(:@records, records) - expect(instance.records).to receive(:order).and_return(records) - allow(records).to receive(:order_values).and_return([]) + records.instance_variable_set(:@records, [record_2, record_1]) + allow(records).to receive(:order_values).and_return([double('order_definition')]) end it 'reorders the records based on hits order' do - expect(records.collect(&:id)).to eq([1, 2]) expect(instance.records.to_a.collect(&:id)).to eq([2, 1]) - expect(instance.order(:foo).to_a.collect(&:id)).to eq([1, 2]) end end end