Looking for feedback and review of changes to AR predicate builder

[Peter Brown]

Hi,

Guillermo asked me to post this here for feedback: https://github.com/rails/rails/pull/7273

The pull request has some examples and rationale for the change, but basically it would let you specify the model name in queries when you'd normally use a foreign key. 

These queries would then be equivalent:

Post.where(:author_id => author)

Post.where(:author => author)

and so would these:

Author.where(:posts => {:author_id => author}).joins(:posts)

Author.where(:posts => {:author => author}).joins(:posts)

TL;DR

• API is more consistent with other parts of ActiveRecord
• Works with polymorphic relationships
• Better for legacy schemas that don't follow Rails' foreign key conventions (you don't need to remember column names)

[Josh Susser]

I think this makes a lot of sense. I tried to do this for the old find_by_x dynamic finders a few years ago but was thwarted by the amount of refactoring it would require. Using association names instead of the underlying foreign key is more natural and keeps things at the right level of abstraction, and is especially helpful for polymorphic associations. If the new ARel-based query API enables this to be done without changing a bunch of other things, I'm all for it.

--

Josh Susser

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To view this discussion on the web visit https://groups.google.com/d/msg/rubyonrails-core/-/lGc5FokBYLoJ.
To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

[thiagocifani]

Great!

But is it still returning a Arel Relation or a array like in dynamic finds?

2012/8/7 Josh Susser <jo...@hasmanythrough.com>

--
thiagocifani

http://about.me/thiagocifani

[Peter Brown]

Somebody pointed out in the pull request that it doesn't set the polymorphic type column, so it only solves half that problem. I'm a little curious what it would take to get it to fully work with polymorphic relationships, but maybe that's outside the scope of this change. 

[Peter Brown]

Josh,

I appreciate the feedback. The consensus was that adding polymorphic support would have the most value so I went ahead and added support for the cases I could think of. I was hoping you could take a look at the test cases I wrote and see if they're what you had in mind.

Thanks!

- Pete

[Peter Brown]

I added a few more test cases to account for polymorphic STI associations, so it should be ready for re-review.

- Pete