-
Notifications
You must be signed in to change notification settings - Fork 798
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[MODEL] Ensure that specified ActiveRecord order is not overwritten by Elasticsearch search results order #835
Conversation
…y Elasticsearch query order
c267c3b
to
0d236c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think we could even remove the code which redefines the to_a
method to the original one?
elasticsearch-rails/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb
Lines 57 to 77 in 0d236c8
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 |
@jazzytomato Good catch, I'll test it and see if we can remove that. Thanks for your review! |
…by checking order_values in #to_a
@jazzytomato I've pushed another commit that removes the interception of |
…y Elasticsearch search results order (#835) * [MODEL] Ensure that specified ActiveRecord order is not overwritten by Elasticsearch query order * [MODEL] Remove interception of #order method, as ordering is handled by checking order_values in #to_a
This pull request resolves the following issues
And is related to this pull request, which partially resolved the issue.
#831