rubocop-rails icon indicating copy to clipboard operation
rubocop-rails copied to clipboard

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

Open dfritsch opened this issue 2 years ago • 1 comments

The RedundantForeignKey cop was introduced here and calls out that it matches the logic from Rails: https://github.com/rubocop/rubocop-rails/pull/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: https://github.com/rails/rails/commit/4e37f288814feacdfe1e7724d2b33ad82575b85a#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 avatar Apr 15 '22 19:04 dfritsch

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!

dfritsch avatar Apr 15 '22 19:04 dfritsch

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

yskkin avatar Sep 28 '22 05:09 yskkin