Skip to content
This repository has been archived by the owner on Mar 30, 2022. It is now read-only.

Bug: Rails 4 order hashes fail inside a has_many. #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Peeja
Copy link

@Peeja Peeja commented Oct 21, 2013

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.

@marcusg
Copy link

marcusg commented Oct 24, 2013

same problem here :(

@ernie
Copy link
Contributor

ernie commented Oct 24, 2013

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.

@Peeja
Copy link
Author

Peeja commented Oct 24, 2013

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.

@iamvery
Copy link
Contributor

iamvery commented Oct 25, 2013

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! 👍

@iamvery
Copy link
Contributor

iamvery commented Oct 25, 2013

Btw, I'm happy to work on this more for the final fix with some direction. Always down for pairing too 😄

@marcusg
Copy link

marcusg commented Oct 25, 2013

@Peeja: thank you for the tip, it works!
@ernie: keep up the good work! 👍

@mikz
Copy link

mikz commented Sep 28, 2015

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.

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

Successfully merging this pull request may close these issues.

5 participants