Looking for feedback and review of changes to AR predicate builder

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)

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.

Great!

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

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.

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

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

  • Pete