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

Rails/RedundantForeignKey cop incorrectly flags foreign key as redundant in Rails 7.0 #690

Open
dfritsch opened this issue Apr 15, 2022 · 2 comments

Comments

@dfritsch
Copy link

The RedundantForeignKey cop was introduced here and calls out that it matches the logic from Rails: #257

However, as part of Rails 7.0, the logic for determining the name of the foreign key no longer falls back to the name of the class but instead now relies on class.model_name: rails/rails@4e37f28#diff-b1aed4985f41ad87dc1be30472b728a19f1a7354f25ce71bf6ce9a7744a9cd0dR668.

This means that you might have a model that defines a model_name but reverts a certain relationship back to its own name and this cop will flag this attribute as redundant when it is not.

Example models:

class Publication < ActiveRecord::Base
end

class Book < Publication
  has_many :chapters, inverse_of: :books, foreign_key: :book_id

  # Override so routes and other helpers use "publications" over "books"
  def self.model_name
    Publication.model_name
  end
end

class Chapter < ActiveRecord::Base
  belongs_to :book
end

Expected behavior

Expectation is that the cop would not flag a foreign_key attribute when it is not redundant

Actual behavior

The foreign_key attributes are flagged (and removed when using auto-correct) on these models.

Steps to reproduce the problem

Example models from the description is the know version of this problem.

I believe the keys are:

  1. Model has defined a class method for model_name which will change the default name used for the foreign_key relationship on a has_one/has_many relationship
  2. The has_one/has_many is defined but is specifically using the name for the foreign key that matches the model name

When those are true, Rails 7.0 will require that you specify the foreign_key attribute to work properly, and this cop will incorrectly flag this for removal.

RuboCop version

$ rubocop -V
1.25.1 (using Parser 3.1.2.0, rubocop-ast 1.17.0, running on ruby 3.0.3 arm64-darwin21)
  - rubocop-rails 2.13.0
  - rubocop-rspec 2.9.0
@dfritsch
Copy link
Author

I can also add that I'm happy to work through adding the tests for this and trying to fix it. If there are any recommended examples for needing to reference other nodes in the class to do this that would be appreciated though!

@yskkin
Copy link

yskkin commented Sep 28, 2022

Due to rails/rails#44594, belongs_to with class_name option must also have foreign_key option.

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

2 participants