From cfcb90d540d30eeff92914fb08706e77a613dc89 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sat, 4 Dec 2021 01:03:43 +0300 Subject: [PATCH] [Fix #587] Add Rails/RedundantPresenceValidationOnBelongsTo cop --- ...for_required_belongs_to_association_cop.md | 1 + config/default.yml | 5 + ...ndant_presence_validation_on_belongs_to.rb | 106 +++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + ..._presence_validation_on_belongs_to_spec.rb | 179 ++++++++++++++++++ 5 files changed, 292 insertions(+) create mode 100644 changelog/new_redundant_presence_validation_for_required_belongs_to_association_cop.md create mode 100644 lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb create mode 100644 spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb diff --git a/changelog/new_redundant_presence_validation_for_required_belongs_to_association_cop.md b/changelog/new_redundant_presence_validation_for_required_belongs_to_association_cop.md new file mode 100644 index 0000000000..5d91ea3519 --- /dev/null +++ b/changelog/new_redundant_presence_validation_for_required_belongs_to_association_cop.md @@ -0,0 +1 @@ +* [#594](https://github.com/rubocop/rubocop-rails/pull/594): Add `Rails/RedundantPresenceValidationOnBelongsTo` cop. ([@pirj][]) diff --git a/config/default.yml b/config/default.yml index d916e83268..48483f97a1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -618,6 +618,11 @@ Rails/RedundantForeignKey: Enabled: true VersionAdded: '2.6' +Rails/RedundantPresenceValidationOnBelongsTo: + Description: 'Checks for redundant presence validation on belongs_to association.' + Enabled: pending + VersionAdded: '<>' + Rails/RedundantReceiverInWithOptions: Description: 'Checks for redundant receiver in `with_options`.' Enabled: true diff --git a/lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb b/lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb new file mode 100644 index 0000000000..9d39a31e8d --- /dev/null +++ b/lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Since Rails 5.0 the default for `belongs_to` is `optional: false` + # unless `config.active_record.belongs_to_required_by_default` is + # explicitly set to `false`. The presence validator is added + # automatically, and explicit presence validation is redundant. + # + # @example + # # bad + # belongs_to :user + # validates :user, presence: true + # + # # bad + # belongs_to :user + # validates :user_id, presence: true + # + # # bad + # belongs_to :author, foreign_key: :user_id + # validates :user_id, presence: true + # + # # good + # belongs_to :user + # + # # good + # belongs_to :author, foreign_key: :user_id + # + class RedundantPresenceValidationOnBelongsTo < Base + include RangeHelp + extend AutoCorrector + extend TargetRailsVersion + + MSG = 'Remove explicit presence validation for `%s`.' + RESTRICT_ON_SEND = %i[validates].freeze + + minimum_target_rails_version 5.0 + + def_node_matcher :presence_validation?, <<~PATTERN + $( + send nil? :validates + (sym $_) + ... + $(hash <$(pair (sym :presence) {true hash}) ...>) + ) + PATTERN + + def_node_matcher :optional_option?, <<~PATTERN + { + (hash <(pair (sym :optional) true) ...>) + (hash <(pair (sym :required) false) ...>) + } + PATTERN + + def_node_matcher :optional?, <<~PATTERN + (send nil? :belongs_to _ ... #optional_option?) + PATTERN + + def_node_matcher :class_with_required_belongs_to?, <<~PATTERN + ( + begin + < + ${ + (send nil? :belongs_to (sym %key) ...) + (send nil? :belongs_to ... (hash <(pair (sym :foreign_key) (sym %fk)) ...>)) + } + ... + > + ) + PATTERN + + def on_send(node) + validation, key, options, presence = presence_validation?(node) + return unless validation + + normalized_key = key.to_s.sub(/_id\Z/, '').to_sym + default_foreign_key = :"#{normalized_key}_id" + belongs_to = class_with_required_belongs_to?(node.parent, key: normalized_key, fk: default_foreign_key) + return unless belongs_to + return if optional?(belongs_to) + + message = format(MSG, association: key.to_s) + + add_offense(presence, message: message) do |corrector| + remove_presence_validation(corrector, node, options, presence) + end + end + + private + + def remove_presence_validation(corrector, node, options, presence) + if options.children.one? + corrector.remove(range_by_whole_lines(node.source_range, include_final_newline: true)) + else + range = range_with_surrounding_comma( + range_with_surrounding_space(range: presence.source_range, side: :left), + :left + ) + corrector.remove(range) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index a642abd347..a238afaa4b 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -73,6 +73,7 @@ require_relative 'rails/read_write_attribute' require_relative 'rails/redundant_allow_nil' require_relative 'rails/redundant_foreign_key' +require_relative 'rails/redundant_presence_validation_on_belongs_to' require_relative 'rails/redundant_receiver_in_with_options' require_relative 'rails/redundant_travel_back' require_relative 'rails/reflection_class_name' diff --git a/spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb b/spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb new file mode 100644 index 0000000000..383bddc25e --- /dev/null +++ b/spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb @@ -0,0 +1,179 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RedundantPresenceValidationOnBelongsTo, :config do + context 'with an explicit redundant presence validation for a required belongs_to association' do + it 'registers an offense' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + RUBY + end + + it 'registers an offense for the default foreign key' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user_id, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user_id`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + RUBY + end + + it 'registers an offense when belongs_to has an explicit foreign key' do + expect_offense(<<~RUBY) + belongs_to :author, foreign_key: :user_id + validates :user_id, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user_id`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :author, foreign_key: :user_id + RUBY + end + + it 'registers an offense when belongs_to has an explicit foreign key with other options' do + expect_offense(<<~RUBY) + belongs_to :author, class_name: 'User', foreign_key: :user_id + validates :user_id, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user_id`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :author, class_name: 'User', foreign_key: :user_id + RUBY + end + + it 'registers an offense even when belongs_to has an explicit foreign key' do + expect_offense(<<~RUBY) + belongs_to :author, foreign_key: :user_id + validates :author, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `author`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :author, foreign_key: :user_id + RUBY + end + + it 'registers an offense even when validates has other preceding options' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user, uniqueness: true, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + validates :user, uniqueness: true + RUBY + end + + it 'registers an offense even when validates has other trailing options' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user, presence: true, uniqueness: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + validates :user, uniqueness: true + RUBY + end + + it 'registers an offense for presence with a message' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user, presence: { message: 'Must be present' } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + RUBY + end + + pending 'registers an offense even when the presence option is factored out' do + expect_offense(<<~RUBY) + belongs_to :user + with_options presence: true do + ^^^^^^^^^^^^^^ Remove explicit presence validation for `:user`. + validates :user + end + RUBY + end + + pending 'registers an offense for non-absence option' do + expect_offense(<<~RUBY) + belongs_to :user + validates :user, absence: false + ^^^^^^^^^^^^^^ Remove explicit presence validation for `:user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + RUBY + end + + pending 'registers an offense when validation key is a string' do + expect_offense(<<~RUBY) + belongs_to :user + validates 'user', presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user + RUBY + end + end + + it 'does not register an offense with redundant presence' do + expect_no_offenses(<<~RUBY) + belongs_to :user + validates :user_count, presence: true + RUBY + end + + it 'does not register an offense with presence validation turned off' do + expect_no_offenses(<<~RUBY) + belongs_to :user + validates :user_id, presence: false + RUBY + end + + it 'does not register an offense with an optional belongs_to' do + expect_no_offenses(<<~RUBY) + belongs_to :user, optional: true + validates :user_id, presence: true + RUBY + end + + it 'does not register an offense with a non-required belongs_to' do + expect_no_offenses(<<~RUBY) + belongs_to :user, required: false + validates :user_id, presence: true + RUBY + end + + pending 'does not register an offense when foreign keys do not match' do + expect_no_offenses(<<~RUBY) + belongs_to :author, foreign_key: :user_id + validates :author_id, presence: true + RUBY + end + + it 'does not register an offense for optional association with explicit foreign key' do + expect_no_offenses(<<~RUBY) + belongs_to :author, foreign_key: :user_id, optional: true + validates :author, presence: true + RUBY + end +end