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

Allow ignoring scopes for inverse_of cop #614

Merged
merged 1 commit into from Jan 11, 2022

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Jan 8, 2022

With rails/rails#43358, Rails can now infer
inverse_of for associations with a scope when
config.active_record.automatic_scope_inversing = true.

This commit adds an IgnoreScopes option to the inverse_of cop allowing
us to disable the cop for scopes when the new Rails option is set to
true.

I considered having the default for IgnoreScopes change based on the
Rails version since this is the default for new Rails >= 7.0
applications, but existing applications that upgrade to 7.0 wouldn't
necessarily have the option set and in that case changing the
rubocop-rails default behavior would be a breaking change.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@composerinteralia composerinteralia force-pushed the inverse-of-scope branch 2 times, most recently from 3815ef8 to 8f4fd31 Compare January 8, 2022 01:36
@pirj
Copy link
Member

pirj commented Jan 8, 2022

automatically detect inverse_of when there is a scope on the association, but not when there is a scope on the potential inverse association

How do we detect that the latter condition is met?

@composerinteralia
Copy link
Contributor Author

composerinteralia commented Jan 8, 2022

How do we detect that the latter condition is met?

That's not something this Cop currently detects, with or without this change (and I'm not sure there's a good way we could detect it without runtime info).

For example:

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post, -> { where("1=0") }
end

Rails is not able to automatically infer the inverse_of for has_many :comments because of the scope on the opposite association. The new config.active_record.automatic_scope_inversing option hasn't changed that behavior.

And regardless of the change in this PR the Rails/InverseOf cop will not report an offense on has_many :comments, even though it doesn't have an inverse_of.

I think this is actually a good thing, since setting inverse_of in cases like this can cause problems. In this contrived example, the post association should never return any records because of the where("1=0") scope, but if we add an inverse_of it could return a record:

class Post < ActiveRecord::Base
  has_many :comments, inverse_of: post
end

class Comment < ActiveRecord::Base
  belongs_to :post, -> { where("1=0") }
end

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

# This fails because we use the already loaded inverse record,
# and thus never run the scope.
assert_nil comment.post 

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've right, I've missed the point of Rails/InverseOf cop.
Now, when Rails provides this option, inverse_of becomes redundant.
Looks good to me.
Thanks for the contribution!

@composerinteralia
Copy link
Contributor Author

🎉 @pirj any chance you have access to approve the workflow so I can verify everything passes?

@pirj
Copy link
Member

pirj commented Jan 10, 2022

access to approve the workflow

Unfortunately not, I'm only volunteering here with preliminary assessment of tickets and preliminary code review.
@koic is the sole maintainer as far as I know.

@koic
Copy link
Member

koic commented Jan 11, 2022

Can you update the config/default.yml?

 Rails/InverseOf:
   Description: 'Checks for associations where the inverse cannot be determined automatically.'
   Enabled: true
   VersionAdded: '0.52'
+  IgnoreScopes: false
   Include:
     - app/models/**/*.rb

# class Post < ApplicationRecord
# belongs_to :blog
# end
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the contrast will be more noticeable whether only the differences are extracted as shown below.

      # @example IgnoreScopes: false (default)
      #   # bad
      #   class Blog < ApplicationRecord
      #     has_many :posts, -> { order(published_at: :desc) }
      #   end
      #
      # @example IgnoreScopes: true
      #   # good
      #   class Blog < ApplicationRecord
      #     has_many :posts, -> { order(published_at: :desc) }
      #   end

And can you move this as the bottom example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Thank you for the review, and hi 👋🏻!

With rails/rails#43358, Rails can now infer
inverse_of for associations with a scope when
`config.active_record.automatic_scope_inversing = true`.

This commit adds an `IgnoreScopes` option to the inverse_of cop allowing
us to disable the cop for scopes when the new Rails option is set to
`true`.

I considered having the default for `IgnoreScopes` change based on the
Rails version since this is the default for new Rails >= 7.0
applications, but existing applications that upgrade to 7.0 wouldn't
necessarily have the option set and in that case changing the
rubocop-rails default behavior would be a breaking change.
@koic koic merged commit f271b4e into rubocop:master Jan 11, 2022
@koic
Copy link
Member

koic commented Jan 11, 2022

Thank you, Daniel!

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

Successfully merging this pull request may close these issues.

None yet

3 participants