squeel
squeel copied to clipboard
Bug: Rails 4 order hashes fail inside a has_many.
In Rails 4, if you give a has_many an order using a hash to specify the sort direction, it's misinterpreted:
class Person < ActiveRecord::Base
has_many :articles_with_order, lambda { order :title => :desc },
:class_name => 'Article'
end
Person.first.articles_with_order.to_sql
# => SELECT "articles".* FROM "articles" WHERE "articles"."person_id" = ? ORDER BY "title"."desc"
A failing spec is attached. It demonstrates the issue, though it probably needs a little rewriting.
same problem here :(
Hey all. Just wanted to note that I see this issue and recognize it needs a fix. However, having started a new job this week and needing to prep my keynote for RubyConf, I am not exactly flush with time to spare looking into this. I know myself -- if I take a quick look at this and find out it's a huge rabbit hole (as is often the case with these things) I will not stop until it's fixed. That is good for users of Squeel, but bad for me, at this point.
I'd really, really welcome someone stepping in and taking a stab at this.
In the meantime, please just don't use that goofy new hash syntax for ordering on your associations.
No problem, @ernie. Congrats on the new job and the keynote!
For @marcusg and others hitting the same issue, the easy workaround is to use a Squeel keypath instead. So, where the failing spec in this PR says:
has_many :articles_with_order, lambda { order :title => :desc },
:class_name => 'Article'
Instead, use:
has_many :articles_with_order, lambda { order{title.desc} },
:class_name => 'Article'
Which means this bug isn't a showstopper for people who want to use Squeel, just a bump in the road to adding it to a project.
I've come up with a "fix" for this, but I'm treading in unknown waters, so I'd like to get some input.
What I've Learned
Orders included in scopes attached to associations are "visited" in order to setup the order_values
. The OrderVisitor
does nothing special with Hash
values so this line by way of this line cause the issue you were seeing. Before Rails 4, was there any legitimate reason to include a has in an order
scope?
The "fix"
It's simple, for orders given hashes I'm assuming that nothing special must be done. This may very well be Bad Things™, but alas your test passes.
At this point I must defer to the experts, but maybe this will make the real fix quicker! :+1:
Btw, I'm happy to work on this more for the final fix with some direction. Always down for pairing too :smile:
@Peeja: thank you for the tip, it works! @ernie: keep up the good work! :thumbsup:
I pulled d55eda3 to our project and it fixed the issue. I'd say it is good to merge with the tests in this PR.