Finding inverse associations automatically

[wangjohn]

If you don't define inverse_of in an association, then you don't get the performance benefits of the inverse associations. However, these inverses can often times be guessed automatically. I'm adding functionality that will do some simple guesses if the inverse_of has not been set in an association.

For example, let's say we have two models Post and Comment:

class Post < ActiveRecord::Base
  has_one :comment
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

The code I'm adding will automatically find that the post and comments are inverses of each other, so that the result is that the above code is equivalent to this:

class Post < ActiveRecord::Base
  has_one :comment, :inverse_of => :post
end

class Comment < ActiveRecord::Base
  belongs_to :post, :inverse_of => :comment
end

Note that I've removed some tests which no longer make any sense (they were asserting that inverses were nil, but the new functionality means that they actually do find an inverse automatically).

[guilleiguaran]

/cc @tenderlove

[egilburg]

I support the idea as I would really like to see such behaviour be automatic (I never understood why parent.children.build, a very commo pattern, doesn't set parent on children without :inverse_of which is a somewhat less known option). That being said, I have some questions about implementation.

Would this work with :class_name option? I'm also concerned that there seems to be no caching, which can cause performance issues with those to_sym and rescue on every association call.

Also, if parent has_many children and child belongs_to parent, then when using syntax parent.children.build, parent.children is an association object and should know the parent's ruby class and association type. Would it be possible to use this information to deterministically find the parent rather than heuristically? (e.g. try to symbolize keys and rescue if not found)

[jeremy]

We'll need to consider that the inverse association may exclude this record via :conditions or :class_name - any others? Default scope on the associated class as well.

This gets hairy quickly. Any way to limit the complexity & scope? Perhaps ignoring associations with any exclusionary conditions at all, rather than trying to check whether they match.

[wangjohn]

@egilburg Your right, it seems like I should be excluding a bunch of cases when the association, as @jeremy said, has particular options on it. I'll also go ahead and implement some caching because it seems to have a small memory penalty in comparison with running this method multiple times.

I'm going to make sure that this method isn't called when there are conditions on the association.

[al2o3cr]

I've been poking at a similar patch, but have never gotten it working quite right. Some thoughts:

• it may be better to check :foreign_key, :primary_key and :class_name instead of relying on the name of the inverse association.
• there needs to be some way to explicitly opt-out of the guessing. A fairly common Rails idiom is to make passing false explicitly turn off default behavior.
• definitely want to avoid picking an association with :conditions or a lambda attached.

[wangjohn]

I've done a bunch of things to improve this PR. First, I've added caching as @egilburg suggested. The results for each reflection are computed once and stored in the @automatic_inverse_of (the user can also clear this cache using remove_automatic_inverse_of!).

Second, I've added the ability for a user to opt out of having the reflection automatically find an inverse, as @al2o3cr suggested. You can now use the following syntax to opt out of the automatic inverse finding:

has_many :posts, :automatic_inverse_of => false

The option only works when you are given an explicit false value.

Third, I've limited the method so that that when particular options are specified on an association, the automatic inverse finding will fail. The association options that cause a failure in the automatic inverse finder are specified in:

INVALID_AUTOMATIC_INVERSE_OPTIONS = [:class_name, :conditions, :through, :polymorphic, :foreign_key]

These options are checked for both the current reflection and the potential inverse.

As a side note: a couple of the models in the ActiveRecord tests had to be changed to :automatic_inverse_of => false because they relied on the models not have inverses.

[jeremy]

Looking good 👍

/cc @jonleighton @tenderlove

[al2o3cr]

Wouldn't :inverse_of => false be equally descriptive to turn off guessing? Also avoids adding another config option...

[wangjohn]

@al2o3cr That's a good point. I don't really see any downside with that approach. I'll go ahead and change it.

[azuby]

Does this work on both sides of a has_many relationship? Refer to issue: #8125

[wangjohn]

@Retistic No it doesn't, and in fact, it shouldn't because the belongs_to association isn't supposed to be invertible into a has_many. For example, see here in the documentation: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations.rb#L933.

Even more clear is inside the belongs_to_association file where invertible_for? is defined: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/belongs_to_association.rb#L72-77. The code clearly only works for has_one associations, not has_many.

I'm not adding any extra functionality to inverse_of, I'm simply making sure that if an inverse can be found easily, then Rails will automatically call that inverse instead of waiting for the user to add it.

[azuby]

That makes sense and this is a great idea.

+1 for this pull request

[jonleighton]

• I'm in favour of this feature in general, but I think it's too late to go in 4.0
• For me inverse_of: nil reads a bit better than inverse_of: false. We're trying to express "this association is not the inverse of anything", i.e. "the association is the inverse of nothing (nil)".
• I think we'll probably need a global config to turn this off, as people upgrading existing apps are likely to encounter problems and will need an easy escape route.

I'm up for working on getting this merged, but let's wait til 4-0-stable has been branched.

[al2o3cr]

@jonleighton - the biggest reason I can see to prefer inverse_of: false is that it's trickier to distinguish an option explicitly set to nil from an option that wasn't passed at all. Using false also follows the pattern established by autosave, where omitting the option / setting it to nil gives the default behavior (for autosave, only autosaving on new records) while false completely disables the behavior.

[jonleighton]

@al2o3cr probably we should just check for falsey values, which both nil and false are, so this probably doesn't need to affect the code. but perhaps the documentation. I still think inverse_of: nil makes more sense, and I don't think it needs to mirror autosave: false (which makes sense in the context of autosave).

[schneems]

Letting people know this is the current state of this PR (had to hunt for it in comments)

wait til 4-0-stable has been branched.

After 4.0 has been branched we will re-visit this feature.

[h-lame]

This is really exciting, my original intention for :inverse_of was that it be automatic. Indeed in the parental_control plugin the feature was extracted from, it always was automatic.

There's some work in there that might be useful to look at, particularly https://github.com/h-lame/parental_control/blob/master/lib/parental_control.rb#L17 onwards. It looks like you've got everything covered, but I notice that you explicitly discard associations with the class_name option set. It's been a while since I used parental_control (it's a plugin after all) but I'm not sure that it has that constraint, so if you can stomach comparing rails 2.3 plugin code to rails 4.0-ish internals it might be worth comparing the approaches.

[wangjohn]

@h-lame Thanks for the tip! I looked at the parental_control code and I think it can indeed be useful. I'm now checking the primary key, and making sure that klass.primary_key matches the reflection's primary key.

I think this makes it possible to keep associations with the class_name option set.

[guilleiguaran]

/cc @jonleighton 4-0-stable is live now, this can be reviewed now 😄

[jonleighton]

I don't think this was ready to be merged. It still has the :automatic_inverse_of option, which is unnecessary as discussed above. @wangjohn would you be up for removing that option please?

Also it really needs some documentation (changelog entry, edits to docs in associations.rb, edits to the associations guide).

[wangjohn]

@jonleighton Yep, I'll put this on my bucket list.

[deependersingla]

@jonleighton Is this done?