diff --git a/changelog/new_add_validations_grouping_cop.md b/changelog/new_add_validations_grouping_cop.md new file mode 100644 index 0000000000..61caa99f73 --- /dev/null +++ b/changelog/new_add_validations_grouping_cop.md @@ -0,0 +1 @@ +* [#555](https://github.com/rubocop/rubocop-rails/issues/555): Add new `Rails/ValidationsGrouping` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index 97b01498b0..32785ea591 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1192,6 +1192,16 @@ Rails/Validation: Include: - app/models/**/*.rb +Rails/ValidationsGrouping: + Description: 'Group together all attribute validations.' + StyleGuide: 'https://rails.rubystyle.guide/#single-attribute-validations' + Enabled: pending + VersionAdded: '<>' + # Enforces single attribute per validation. + SingleAttributeValidations: false + Include: + - app/models/**/*.rb + Rails/WhereEquals: Description: 'Pass conditions to `where` and `where.not` as a hash instead of manually constructing SQL.' StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions' diff --git a/lib/rubocop/cop/rails/validations_grouping.rb b/lib/rubocop/cop/rails/validations_grouping.rb new file mode 100644 index 0000000000..46cc065b7e --- /dev/null +++ b/lib/rubocop/cop/rails/validations_grouping.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Enforces the grouping of attribute validations. + # + # @example + # # bad + # validates :name, :email, :bio, presence: true + # validates :email, format: { with: /@/ } + # + # # good + # validates :name, :bio, presence: true + # validates :email, presence: true, format: { with: /@/ } + # + # # good + # validates :name, :email, :bio, presence: true, if: :some_condition? + # validates :email, format: { with: /@/ } + # + # @example SingleAttributeValidations: true + # # Enforces single attribute per validation. + # + # # bad + # validates :name, :bio, presence: true + # + # # good + # validates :name, presence: true + # validates :bio, presence: true + # + class ValidationsGrouping < Base + MSG = 'Group together all %s validations.' + SINGLE_ATTRIBUTE_MSG = 'Specify single attribute per validation.' + + VALIDATION_KEYS = %w[acceptance confirmation comparison exclusion format inclusion + length numericality presence absence uniqueness].freeze + + RESTRICT_ON_SEND = %i[validates].freeze + + def_node_matcher :validates?, <<~PATTERN + (send nil? :validates ${sym str} + hash) + PATTERN + + def on_new_investigation + # Hash is keyed by parent id and then by attribute name within it. + @validations = Hash.new do |outer, parent| + outer[parent] = Hash.new { |inner, attribute| inner[attribute] = [] } + end + @validations.compare_by_identity + end + + def on_send(node) + validates?(node) do |attribute_nodes| + if attribute_nodes.size > 1 && cop_config['SingleAttributeValidations'] + add_offense(node, message: SINGLE_ATTRIBUTE_MSG) + end + + check_for_same_attributes_validations(node, attribute_nodes) + end + end + + private + + def check_for_same_attributes_validations(validation_node, attribute_nodes) + attribute_nodes.each do |attribute_node| + parent_node = validation_node.parent + attribute_name = attribute_node.value.to_s + @validations[parent_node][attribute_name] << validation_node + + validation_nodes = @validations[parent_node][attribute_name] + next unless validation_nodes.size > 1 && same_options?(validation_nodes) + + message = format(MSG, attribute: attribute_name) + + validation_nodes.each do |node| + add_offense(attribute_node(node, attribute_name), message: message) + end + end + end + + def same_options?(validation_nodes) + options = validation_nodes.map do |node| + option_node = node.last_argument + option_node.pairs.to_h do |pair| + [pair.key.source, pair.value.source] + end + end + + options = options.map { |o| extra_options(o) } + options.all?(options.first) + end + + def extra_options(options) + options.except(*VALIDATION_KEYS) + end + + def attribute_node(validation_node, attribute_name) + validation_node.arguments[0..-2].find do |argument| + argument.value.to_s == attribute_name.to_s + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index b7eddc4fe4..c8d5f61e17 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -134,6 +134,7 @@ require_relative 'rails/unused_ignored_columns' require_relative 'rails/unused_render_content' require_relative 'rails/validation' +require_relative 'rails/validations_grouping' require_relative 'rails/where_equals' require_relative 'rails/where_exists' require_relative 'rails/where_missing' diff --git a/spec/rubocop/cop/rails/validations_grouping_spec.rb b/spec/rubocop/cop/rails/validations_grouping_spec.rb new file mode 100644 index 0000000000..67beda5813 --- /dev/null +++ b/spec/rubocop/cop/rails/validations_grouping_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::ValidationsGrouping, :config do + let(:cop_config) { { 'SingleAttributeValidations' => false } } + + it 'registers an offense when attribute validations are split' do + expect_offense(<<~RUBY) + validates :name, :email, presence: true + ^^^^^^ Group together all email validations. + validates :email, format: { with: /@/ } + ^^^^^^ Group together all email validations. + RUBY + end + + it 'does not register an offense when attribute validations are grouped' do + expect_no_offenses(<<~RUBY) + validates :email, presence: true, format: { with: /@/ } + RUBY + end + + it 'does not register an offense for not-literal attribute name validations' do + expect_no_offenses(<<~RUBY) + validates attribute, presence: true + validates attribute, format: { with: /@/ } + RUBY + end + + it 'does not register an offense when attribute validations have different configuration options' do + expect_no_offenses(<<~RUBY) + validates :email, presence: true, if: :some_condition? + validates :email, format: { with: /@/ } + RUBY + end + + it 'registers an offense when attribute validations have same configuration options' do + expect_offense(<<~RUBY) + validates :email, presence: true, if: :some_condition? + ^^^^^^ Group together all email validations. + validates :email, format: { with: /@/ }, if: :some_condition? + ^^^^^^ Group together all email validations. + RUBY + end + + it 'registers an offense when attribute validations have different validation options' do + expect_offense(<<~RUBY) + validates :email, format: { with: /foo/ } + ^^^^^^ Group together all email validations. + validates :email, format: { with: /bar/ } + ^^^^^^ Group together all email validations. + RUBY + end + + context 'with conditionals' do + it 'registers an offense when attribute validations within conditional are split' do + expect_offense(<<~RUBY) + validates :email, uniqueness: true + + if condition? + validates :name, :email, presence: true + ^^^^^^ Group together all email validations. + validates :email, format: { with: /@/ } + ^^^^^^ Group together all email validations. + end + RUBY + end + + it 'registers an offense when attribute validations outside conditional are split' do + expect_offense(<<~RUBY) + validates :name, :email, presence: true + ^^^^^^ Group together all email validations. + validates :email, format: { with: /@/ } + ^^^^^^ Group together all email validations. + + if condition? + validates :email, uniqueness: true + end + RUBY + end + end + + context 'SingleAttributeValidations is used' do + let(:cop_config) { { 'SingleAttributeValidations' => true } } + + it 'registers and offense for multiple attributes per validation' do + expect_offense(<<~RUBY) + validates :name, :email, presence: true + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specify single attribute per validation. + validates :name, :email, presence: true, if: :some_condition? + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Specify single attribute per validation. + RUBY + end + end +end