Skip to content

Commit

Permalink
Add new Rails/ValidationsGrouping cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima committed Sep 2, 2024
1 parent 5c8b114 commit 1b96385
Show file tree
Hide file tree
Showing 5 changed files with 210 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_validations_grouping_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#555](https://github.com/rubocop/rubocop-rails/issues/555): Add new `Rails/ValidationsGrouping` cop. ([@fatkodima][])
10 changes: 10 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<<next>>'
# 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'
Expand Down
105 changes: 105 additions & 0 deletions lib/rubocop/cop/rails/validations_grouping.rb
Original file line number Diff line number Diff line change
@@ -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 %<attribute>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.arguments.last
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
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
93 changes: 93 additions & 0 deletions spec/rubocop/cop/rails/validations_grouping_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 1b96385

Please sign in to comment.