Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional configuration for Rails/InverseOf #632

Closed
composerinteralia opened this issue Jan 20, 2022 · 3 comments
Closed

Additional configuration for Rails/InverseOf #632

composerinteralia opened this issue Jan 20, 2022 · 3 comments

Comments

@composerinteralia
Copy link
Contributor

composerinteralia commented Jan 20, 2022

Is your feature request related to a problem? Please describe.

The Rails/InverseOf cop reports violations for any belongs_to associations with options like :foreign_key that prevent automatic inverse detection. This is great for has_one/belongs_to association pairs, and for belongs_to associations on join models in the middle of a has_many :through, but I don't think it's that helpful for the common has_many/belongs_to pairs.

Consider the following example:

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

Rails will automatically infer the inverse of the :comments association, but it does NOT infer the inverse of the :post association (Rails can only automatically detect singular inverses, so we'd get the automatic inverse if the other side was has_one :comment, but not for the collection has_many :comments).

In general I don't think having an inverse on the belongs_to side of a has_many/belongs_to is helpful. For example doing comment.post.comments will still require a load of all the comments (whereas going the other way a missing inverse on the has_many could cause N+1 queries with something like post.comments.each(&:post)).

If we add the :foreign_key option to the belongs_to:

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post, foreign_key: :post_id
end

Rails no longer automatically infers the inverse for either association. The Rails/InverseOf cop tells us to add an :inverse_of to the belongs_to association, but I don't think that's actually helpful. What we really want is to add :inverse_of to has_many :comments, but I'm not sure there's a good way for this cop to tell us that (and most of the time the :foreign_key option would be on both associations anyway).

I'm trying to introduce the Rails/InverseOf cop at GitHub, but I'd rather not force people to add inverses in places where it's not helpful. That would seem to add friction without much benefit.

Describe the solution you'd like

I'd like an option to disable linting belongs_to associations and focus only on has_one and has_many (and I'd be happy to submit a PR for this if it is acceptable). This does mean we'll miss some cases where having inverse_of on the belongs_to might have been helpful (i.e. when it's the other side of a has_one or it's in a join model), but those cases are less common than has_many/belongs_to pairs and at least in my case that tradeoff is worth it.

Describe alternatives you've considered

I've been working to get Rails to automatically infer inverses in more places, essentially to make this cop unnecessary. I managed to get Rails to infer inverses when there are scopes, but trying to get it to infer inverses more broadly has stalled because the solution is too complex can impact performance.

@MattFenelon
Copy link

Was this closed by #614?

@composerinteralia
Copy link
Contributor Author

No, that one had to do with a new scope option added in Rails. This one has to do with disabling the cop for belongs_to associations entirely.

But given that this issue hasn't seen any movement in a year, I think it's fine to close it for now (we can always reopen if there is renewed interest).

@thestelz
Copy link
Contributor

thestelz commented Jun 1, 2023

Hey @composerinteralia, we are currently running into this issue. I would love to see this added into rubocop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants