diff --git a/CHANGELOG.md b/CHANGELOG.md index 62ac2fa3e1..05e6421e19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * [#211](https://github.com/rubocop-hq/rubocop-rails/issues/211): Add autocorrect to `Rails/RakeEnvironment` cop. ([@tejasbubane][]) * [#242](https://github.com/rubocop-hq/rubocop-rails/pull/242): Add `Rails/ContentTag` cop. ([@tabuchi0919][]) * [#249](https://github.com/rubocop-hq/rubocop-rails/pull/249): Add new `Rails/Pick` cop. ([@eugeneius][]) +* [#257](https://github.com/rubocop-hq/rubocop-rails/pull/257): Add new `Rails/RedundantForeignKey` cop. ([@eugeneius][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 375bb0f42a..89e597fdeb 100644 --- a/config/default.yml +++ b/config/default.yml @@ -408,6 +408,11 @@ Rails/RedundantAllowNil: Include: - app/models/**/*.rb +Rails/RedundantForeignKey: + Description: 'Checks for associations where the `:foreign_key` option is redundant.' + Enabled: true + VersionAdded: '2.6' + Rails/RedundantReceiverInWithOptions: Description: 'Checks for redundant receiver in `with_options`.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index b6b80dd2c6..01c3a80912 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -48,6 +48,7 @@ * xref:cops_rails.adoc#railsrakeenvironment[Rails/RakeEnvironment] * xref:cops_rails.adoc#railsreadwriteattribute[Rails/ReadWriteAttribute] * xref:cops_rails.adoc#railsredundantallownil[Rails/RedundantAllowNil] +* xref:cops_rails.adoc#railsredundantforeignkey[Rails/RedundantForeignKey] * xref:cops_rails.adoc#railsredundantreceiverinwithoptions[Rails/RedundantReceiverInWithOptions] * xref:cops_rails.adoc#railsreflectionclassname[Rails/ReflectionClassName] * xref:cops_rails.adoc#railsrefutemethods[Rails/RefuteMethods] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index bb78232950..a7c933f57a 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -2391,6 +2391,44 @@ validates :x, length: { is: 5 }, allow_nil: true, allow_blank: false | Array |=== +== Rails/RedundantForeignKey + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Enabled +| Yes +| Yes +| 2.6 +| - +|=== + +This cop detects cases where the `:foreign_key` option on associations +is redundant. + +=== Examples + +[source,ruby] +---- +# bad +class Post + has_many :comments, foreign_key: 'post_id' +end + +class Comment + belongs_to :post, foreign_key: 'post_id' +end + +# good +class Post + has_many :comments +end + +class Comment + belongs_to :author, foreign_key: 'user_id' +end +---- + == Rails/RedundantReceiverInWithOptions |=== diff --git a/lib/rubocop/cop/rails/redundant_foreign_key.rb b/lib/rubocop/cop/rails/redundant_foreign_key.rb new file mode 100644 index 0000000000..ebba1c9730 --- /dev/null +++ b/lib/rubocop/cop/rails/redundant_foreign_key.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop detects cases where the `:foreign_key` option on associations + # is redundant. + # + # @example + # # bad + # class Post + # has_many :comments, foreign_key: 'post_id' + # end + # + # class Comment + # belongs_to :post, foreign_key: 'post_id' + # end + # + # # good + # class Post + # has_many :comments + # end + # + # class Comment + # belongs_to :author, foreign_key: 'user_id' + # end + class RedundantForeignKey < Cop + include RangeHelp + + MSG = 'Specifying the default value for `foreign_key` is redundant.' + + def_node_matcher :association_with_foreign_key, <<~PATTERN + (send nil? ${:belongs_to :has_one :has_many :has_and_belongs_to_many} ({sym str} $_) + $(hash <$(pair (sym :foreign_key) ({sym str} $_)) ...>) + ) + PATTERN + + def on_send(node) + association_with_foreign_key(node) do |type, name, options, foreign_key_pair, foreign_key| + if redundant?(node, type, name, options, foreign_key) + add_offense(node, location: foreign_key_pair.loc.expression) + end + end + end + + def autocorrect(node) + _type, _name, _options, foreign_key_pair, _foreign_key = association_with_foreign_key(node) + range = range_with_surrounding_space(range: foreign_key_pair.source_range, side: :left) + range = range_with_surrounding_comma(range, :left) + + lambda do |corrector| + corrector.remove(range) + end + end + + private + + def redundant?(node, association_type, association_name, options, foreign_key) + foreign_key.to_s == default_foreign_key(node, association_type, association_name, options) + end + + def default_foreign_key(node, association_type, association_name, options) + if association_type == :belongs_to + "#{association_name}_id" + elsif (as = find_as_option(options)) + "#{as}_id" + else + node.parent_module_name&.foreign_key + end + end + + def find_as_option(options) + options.pairs.find do |pair| + pair.key.sym_type? && pair.key.value == :as + end&.value&.value + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 8e12c90da5..ec291ddc5b 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -50,6 +50,7 @@ require_relative 'rails/rake_environment' require_relative 'rails/read_write_attribute' require_relative 'rails/redundant_allow_nil' +require_relative 'rails/redundant_foreign_key' require_relative 'rails/redundant_receiver_in_with_options' require_relative 'rails/reflection_class_name' require_relative 'rails/refute_methods' diff --git a/spec/rubocop/cop/rails/redundant_foreign_key_spec.rb b/spec/rubocop/cop/rails/redundant_foreign_key_spec.rb new file mode 100644 index 0000000000..1ddcbb110a --- /dev/null +++ b/spec/rubocop/cop/rails/redundant_foreign_key_spec.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RedundantForeignKey do + subject(:cop) { described_class.new } + + context '`belongs_to` associations' do + it 'registers an offense when the `:foreign_key` option is redundant' do + expect_offense(<<~RUBY) + class Comment + belongs_to :post, foreign_key: 'post_id' + ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Comment + belongs_to :post + end + RUBY + end + + it 'does not register an offense when the `:foreign_key` option is not redundant' do + expect_no_offenses(<<~RUBY) + class Comment + belongs_to :author, foreign_key: 'user_id' + end + RUBY + end + + it 'does not register an offense when the `:foreign_key` option is absent' do + expect_no_offenses(<<~RUBY) + class Comment + belongs_to :author + end + RUBY + end + + it 'registers an offense even when other options are used' do + expect_offense(<<~RUBY) + class Comment + belongs_to :post, class_name: 'SpecialPost', foreign_key: 'post_id', dependent: :destroy + ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Comment + belongs_to :post, class_name: 'SpecialPost', dependent: :destroy + end + RUBY + end + + it 'registers an offense even when defined in a block' do + expect_offense(<<~RUBY) + class_methods do + belongs_to :post, foreign_key: 'post_id' + ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class_methods do + belongs_to :post + end + RUBY + end + end + + %w[has_one has_many has_and_belongs_to_many].each do |association_type| + context "`#{association_type}` associations" do + it 'registers an offense when the `:foreign_key` option is redundant' do + expect_offense(<<~RUBY, association_type: association_type) + class Book + %{association_type} :chapter, foreign_key: 'book_id' + _{association_type} ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Book + #{association_type} :chapter + end + RUBY + end + + it 'does not register an offense when the `:foreign_key` option is not redundant' do + expect_no_offenses(<<~RUBY) + class Book + #{association_type} :chapter, foreign_key: 'publication_id' + end + RUBY + end + + it 'does not register an offense when the `:foreign_key` option is absent' do + expect_no_offenses(<<~RUBY) + class Book + #{association_type} :chapter + end + RUBY + end + + it 'registers an offense even when other options are used' do + expect_offense(<<~RUBY, association_type: association_type) + class Book + %{association_type} :chapter, class_name: 'SpecialChapter', foreign_key: 'book_id', dependent: :destroy + _{association_type} ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Book + #{association_type} :chapter, class_name: 'SpecialChapter', dependent: :destroy + end + RUBY + end + + it 'registers an offense even when multiple associations are defined' do + expect_offense(<<~RUBY, association_type: association_type) + class Book + belongs_to :series + + %{association_type} :chapter, foreign_key: 'book_id' + _{association_type} ^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Book + belongs_to :series + + #{association_type} :chapter + end + RUBY + end + + it 'does not register an offense when defined in a block' do + expect_no_offenses(<<~RUBY) + class_methods do + #{association_type} :chapter, foreign_key: 'book_id' + end + RUBY + end + + it 'registers an offense when the `:foreign_key` options is redundant with the `:as` option' do + expect_offense(<<~RUBY, association_type: association_type) + class Book + %{association_type} :chapter, as: :publishable, foreign_key: 'publishable_id' + _{association_type} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specifying the default value for `foreign_key` is redundant. + end + RUBY + + expect_correction(<<~RUBY) + class Book + #{association_type} :chapter, as: :publishable + end + RUBY + end + + it 'does not register an offense when the `:foreign_key` option is not redundant with the `:as` option' do + expect_no_offenses(<<~RUBY) + class Book + #{association_type} :chapter, as: :publishable, foreign_key: 'book_id' + end + RUBY + end + + it 'does not register an offense when the class cannot be determined' do + expect_no_offenses(<<~RUBY) + #{association_type} :chapter, foreign_key: 'book_id' + RUBY + end + end + end +end