Convert model name to foreign key in queries

[beerlington]

This allows you to specify the model in a belongs_to relationship instead of the foreign key when querying. It does this by looking up the correct foreign key on the association, and changing the column to use that key when appropriate. It came out of a discussion in issue #1736 that seemed to have some interest.

The following queries are now equivalent:

Post.where(:author_id => Author.first)
Post.where(:author => Author.first)

I also think it makes it more consistent with has_many queries where the relationship and query key are both plural:

posts = Post.containing_the_letter_a.limit(5)
Author.where(:posts => posts)

[rafaelfranca]

Thank you for the pull request.

I don't know if is a good idea add more complexity changing a well know API. What are the benefits of this new format?

cc/ @jonleighton @tenderlove

[beerlington]

@rafaelfranca maybe it's not worth the added complexity, but I do feel like it's more consistent with other ActiveRecord APIs. When I'm working with models, I tend to be thinking in terms of relationships and objects as opposed to database columns. Take the following two examples:

# associations use models and hide foreign key details
belongs_to :author

# creating related objects uses models too
Post.new(:author => Author.first)

But then when you are querying, you sort of shift to an object/database hybrid mindset:

Post.where(:author_id => Author.first)

I am specifying the foreign key on one side, and the object on the other, and I think it can be hard to remember when you can use the associations and when you can't. Obviously a more practical approach would be to use Author.first.posts, but this gives flexibility in cases where you might not have the relationships fully setup.

Another useful thing about it is that it works with polymorphic belongs_to relationships as well:

class Image < ActiveRecord::Base
  belongs_to :imageable, :polymorphic => true
end

class Cat < ActiveRecord::Base
  has_many :images, :as => :imageable, :dependent => :destroy
end

# So you can do...
Image.where(:imageable => Cat.first)

Honestly though, I was just looking for an excuse to dig into the internals of ActiveRecord and it turned out to be much less hacky than I thought it would. I won't be offended if you guys give it a 👎 since it was a great learning experience. :)

[rafaelfranca]

I'm not giving 👎.

Make sense this consistence part and this polymorphic case seems a good benefit. I''ll wait for more feedback.

Also, thank you for this great explanation.

[guilleiguaran]

I really like this feature 👍 ( haven't checked the implementation yet), can you send a email to Rails-core mailing list to get more feedback?

[Antiarchitect]

👍 Looks naturally.

[beerlington]

@guilleiguaran thanks! Here's the mailing list discussion

[al2o3cr]

+1 for the idea, especially since I've been writing a lot of conditions against polymorphic belongs_to lately.

However, the current implementation doesn't generate a condition against the polymorphic type column (unless I've completely missed something). In your Imagable example, it generates a condition for imageable_id but not imageable_type.

[beerlington]

@al2o3cr good point about not adding the imageable type. I didn't really set out to do anything with polymorphic relationships, it was sort of a side effect. It probably wouldn't be hard to patch it to grab the type too, but maybe outside the scope of this PR (or not?)

[al2o3cr]

Personally, the polymorphic case is a huge win for this, since it avoids manually extracting the record's base class name all over the place. The corresponding column is available from the reflection as foreign_type.

[beerlington]

@al2o3cr agreed. I'll explore that this evening when I have some free time and see what it would take.

[dhh]

I tentatively like this, but only if it also does the polymorphic conversion. Otherwise its just a setup for inconsistency.

[jonleighton]

I like this too, but as other have said, please have a go at the polymorphic case. It need a changelog entry and doc updates also.

[beerlington]

@dhh and @jonleighton thanks for the feedback! I did some work on building in polymorphic support but wasn't sure exactly what use cases you were looking for.

There's a shallow query that's pretty trivial:

PriceEstimate.where(:estimate_of_type => 'Treasure', :estimate_of_id => treasure)
PriceEstimate.where(:estimate_of => treasure)

But then things start getting more complicated when you look at a nested query like:

Treasure.where(:price_estimates => {:estimate_of_type => 'Treasure', :estimate_of_id => treasure}).joins(:price_estimates)
Treasure.where(:price_estimates => {:estimate_of => treasure}).joins(:price_estimates)

I have both cases working, but the second is more complicated since it has to look at the polymorphic model's reflection and doesn't work on has_one associations. If the second use case is not something you envision people using, then it may not be worth the added complexity.

I pushed up my changes, but wanted to make sure I had all the test cases covered before updating the docs & changelog and rebasing.

[beerlington]

@jonleighton I went ahead and updated the docs & changelog, and squashed commits. Please take a look when you get a chance, thanks!

[jonleighton]

Probably we should make this work more generally? E.g. I might want to do Post.joins(:comments).where(comments: { author: david }). So I think there needs to be one level of recursion here.

[beerlington]

@jonleighton It doesn't appear that the elsif reflection.options[:as] case is even needed anymore. Maybe this was due to a change in ActiveRecord or ARel in the last month? Regardless, my tests now pass without it so I'm going to remove it. I think the example case you mentioned is already covered by some of the "nested" tests I wrote. See nested_where, polymorphic_nested_where, and sti_polymorphic_nested_where. If that's not what you had in mind, please let me know.

[jonleighton]

@beerlington Sorry for being slow. It's looking good but I added a couple of comments. Could you also update the querying guide? Thanks.

I'll try to be faster to reply this time - please @-mention me so I see you reply. Cheers

[beerlington]

@jonleighton I made some updates based on your comments. I wasn't totally sure on your comment about making it work more generally, so please take another look and see if this meets your expectations.

[jonleighton]

@beerlington yep, seems that my comment was not necessary - you have a test for the example I gave.

There are a few refactorings I'd like to make but I think it will be easier for me to just merge and perform them after that. Could you squash the commits please? Then I will merge it.

Thanks

[guilleiguaran]

Please move this to the top of file

[beerlington]

@jonleighton @guilleiguaran I moved the changelog entry to the top and squashed the commits.