Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MODEL] Fix sort order on ActiveRecord >= 5 #831

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

jazzytomato
Copy link
Contributor

This is addressing an issue in #546

The monkey patching regarding sort order was not removed when calling order on ActiveRecord versions >= 5.

The code introduced in #618 did not change the order method, this patch fixes it.

I have added a test that fails without my new code. The problem with testing the order with .first is that it applies the sql LIMIT 1, which would return a collection of 1 item from the database. But we want to test the order of the collection returned. Therefore, I added a test for the first item of the collection with [0] which doesn't limit the results returned from the db.

@estolfo
Copy link
Contributor

estolfo commented Sep 4, 2018

Thanks @jazzytomato !

@estolfo estolfo merged commit 0143aa7 into elastic:master Sep 4, 2018
@estolfo
Copy link
Contributor

estolfo commented Sep 4, 2018

@jazzytomato If I understand your comment correctly, #546 is not fully fixed with this PR, right?

@jazzytomato
Copy link
Contributor Author

@estolfo Indeed, there is another issue which is a bit more involved.

The .order method is broken when sent to ActiveRecord ::Relation instead of Elasticsearch::Model::Response::Records, i.e. this will work

Model.search('abc').order('id desc')

but this will not work

Model.search('abc').distinct.order('id desc')

My comment here explains it in more details: #546 (comment)

@estolfo
Copy link
Contributor

estolfo commented Sep 13, 2018

@jazzytomato
I believe this pull request resolves the issue: #835
I had to test if there was an order defined on the ActiveRecord query before applying the order of ids returned by the Elasticsearch query. Please test it and let me know if it works for you. Thanks for all your help on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants