Automatically infer inverse_of with scopes by composerinteralia · Pull Request #...

Background

I recently noticed we had a number of associations in GitHub that would
benefit from having inverse_of set, and so I began adding them. I
ended up adding them to virtually every association with a scope, at
which point I wondered whether Rails might be able to automatically find
these inverses for us.

For GitHub, the changes in this commit end up automatically adding
inverse_of to 171 of associations that were missing it.

My understanding is that we do not automatically detect inverse_of for
associations with scopes because the scopes could exclude the records we
are trying to inverse from. But I think that should only matter if there
is a scope on the inverse side, not on the association itself.

For example:

Scope on has_many

class Post < ActiveRecord::Base
  has_many :comments, -> { visible }
end

class Comment < ActiveRecord::Base
  belongs_to :post

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

post = Post.create!
comment = post.comments.hidden.create!
assert comment.post

This code leaves post.comments in sort of a weird state, since it
includes a comment that the association would filter out. But that's
true regardless of the changes in this commit.

Regardless of whether the comments association has an inverse,
comment.post will return the post. The difference is that when
inverse_of is set we use the existing post we have in memory, rather
than loading it again. If there is a downside to having the inverse_of
automatically set here I'm not seeing it.

Scope on belongs_to

class Post < ActiveRecord::Base
  has_many :comments

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

class Comment < ActiveRecord::Base
  belongs_to :post, -> { visible }
end

post = Post.hidden.create!
comment = post.comments.create!
assert_nil comment.post

This example is a different story. We don't want to automatically infer
the inverse here because that would change the behavior of
comment.post. It should return nil, since it's scoped to visible
posts while this one is hidden.

This behavior was not well tested, so this commit adds a test to
ensure we haven't changed it.

Changes

This commit changes can_find_inverse_of_automatically to allow us to
automatically detect inverse_of when there is a scope on the
association, but not when there is a scope on the potential inverse
association. (can_find_inverse_of_automatically gets called first with
the association's reflection, then if it returns true we attempt to find
the inverse reflection, and finally we call the method again with the
inverse reflection to ensure we can really use it.)

Since this is a breaking change—specifically in places where code may
have relied on a missing inverse_of causing fresh copies of a record
to be loaded—we've placed it behind the automatic_scope_inversing flag
(whose name was inspired by has_many_inversing). It is set to true for
new applications via framework defaults.

Testing

In addition to the inverse association tests, this commit also adds some
cases to a few tests related to preloading. They are basically
duplicates of existing tests, but with lower query counts.

Testing this change with GitHub, the bulk of the failing tests were
related to lower query counts. There were additionally 3 places (2 in
tests and one in application code) where we relied on missing
inverse_of causing fresh copies of a record to be loaded.

There's still one Rails test that wouldn't pass if we ran the whole
suite with automatic_scope_inversing = true. It's related to
TaggedPost, which changes the polymorphic type from the base class
Post to the subclass TaggedPost.

class TaggedPost < Post
  has_many :taggings, -> { rewhere(taggable_type: "TaggedPost") }, as: :taggable
end

Setting the inverse doesn't work because it ends up changing the type
back to Post, something like this:

post = TaggedPost.new
tagging = post.taggings.new
puts tagging.taggable_type
#=> TaggedPost

tagging.taggable = post
puts tagging.taggable_type
#=> Post

I think this is an acceptable change, given that it's a fairly specific
scenario, and is sort of at odds with the way polymorphic associations
are meant to work (they are meant to contain the base class, not the
subclass). If someone is relying on this specific behavior they can
still either keep automatic_scope_inversing set to false, or they can
add inverse_of: false to the association.

I haven't found any other cases where having the inverse_of would
cause problems like this.

cc @chrisbloom7