From 301c7d46950d6ac3ea05e1621553e01658b3650f 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 | 192 +++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + ..._presence_validation_on_belongs_to_spec.rb | 202 ++++++++++++++++++ 5 files changed, 401 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..8ddbb73668 --- /dev/null +++ b/lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb @@ -0,0 +1,192 @@ +# 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 + + # @!method presence_validation?(node) + # Match a `validates` statement with a presence check + # + # @example source that matches - by association + # validates :user, presence: true + # + # @example source that matches - with presence options + # validates :user, presence: { message: 'duplicate' } + # + # @example source that matches - by a foreign key + # validates :user_id, presence: true + def_node_matcher :presence_validation?, <<~PATTERN + $( + send nil? :validates + (sym $_) + ... + $(hash <$(pair (sym :presence) {true hash}) ...>) + ) + PATTERN + + # @!method optional_option?(node) + # Match a `belongs_to` association with an optional option in a hash + def_node_matcher :optional?, <<~PATTERN + (send nil? :belongs_to _ ... #optional_option?) + PATTERN + + # @!method optional_option?(node) + # Match an optional option in a hash + def_node_matcher :optional_option?, <<~PATTERN + { + (hash <(pair (sym :optional) true) ...>) # optional: true + (hash <(pair (sym :required) false) ...>) # required: false + } + PATTERN + + # @!method any_belongs_to?(node, association:) + # Match a class with `belongs_to` with no regard to `foreign_key` option + # + # @example source that matches + # belongs_to :user + # + # @example source that matches - regardless of `foreign_key` + # belongs_to :author, foreign_key: :user_id + # + # @param node [RuboCop::AST::Node] + # @param association [Symbol] + # @return [Array, nil] matching node + def_node_matcher :any_belongs_to?, <<~PATTERN + (begin + < + $(send nil? :belongs_to (sym %association) ...) + ... + > + ) + PATTERN + + # @!method belongs_to?(node, key:, fk:) + # Match a class with a matching association, either by name or an explicit + # `foreign_key` option + # + # @example source that matches - fk matches `foreign_key` option + # belongs_to :author, foreign_key: :user_id + # + # @example source that matches - key matches association name + # belongs_to :user + # + # @example source that does not match - explicit `foreign_key` does not match + # belongs_to :user, foreign_key: :account_id + # + # @param node [RuboCop::AST::Node] + # @param key [Symbol] e.g. `:user` + # @param fk [Symbol] e.g. `:user_id` + # @return [Array] matching nodes + def_node_matcher :belongs_to?, <<~PATTERN + (begin + < + ${ + #belongs_to_without_fk?(%key) # belongs_to :user + #belongs_to_with_a_matching_fk?(%fk) # belongs_to :author, foreign_key: :user_id + } + ... + > + ) + PATTERN + + # @!method belongs_to_without_fk?(node, fk) + # Match a matching `belongs_to` association, without an explicit `foreign_key` option + # + # @param node [RuboCop::AST::Node] + # @param key [Symbol] e.g. `:user` + # @return [Array] matching nodes + def_node_matcher :belongs_to_without_fk?, <<~PATTERN + { + (send nil? :belongs_to (sym %1)) # belongs_to :user + (send nil? :belongs_to (sym %1) !hash) # belongs_to :user, -> { not_deleted } + (send nil? :belongs_to (sym %1) !(hash <(pair (sym :foreign_key) _) ...>)) + } + PATTERN + + # @!method belongs_to_with_a_matching_fk?(node, fk) + # Match a matching `belongs_to` association with a matching explicit `foreign_key` option + # + # @example source that matches + # belongs_to :author, foreign_key: :user_id + # + # @param node [RuboCop::AST::Node] + # @param fk [Symbol] e.g. `:user_id` + # @return [Array] matching nodes + def_node_matcher :belongs_to_with_a_matching_fk?, <<~PATTERN + (send nil? :belongs_to ... (hash <(pair (sym :foreign_key) (sym %1)) ...>)) + PATTERN + + def on_send(node) + validation, key, options, presence = presence_validation?(node) + return unless validation + + belongs_to = belongs_to_for(node.parent, 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 belongs_to_for(model_class_node, key) + if key.to_s.end_with?('_id') + normalized_key = key.to_s.delete_suffix('_id').to_sym + belongs_to?(model_class_node, key: normalized_key, fk: key) + else + any_belongs_to?(model_class_node, association: key) + end + end + + 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..d138e24817 --- /dev/null +++ b/spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb @@ -0,0 +1,202 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RedundantPresenceValidationOnBelongsTo, :config do + context 'Rails >= 5.0', :rails50 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 when association is defined with a scope' do + expect_offense(<<~RUBY) + belongs_to :user, -> { not_deleted } + validates :user, presence: true + ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`. + RUBY + + expect_correction(<<~RUBY) + belongs_to :user, -> { not_deleted } + 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 + + it '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 + + context 'Rails < 5.0', :rails42 do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + belongs_to :user # belongs_to is optional by default in Rails 4.2 + validates :user, presence: true + RUBY + end + end +end