forked from rubocop/rubocop-rails
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Fix rubocop#587] Add Rails/RedundantPresenceValidationOnBelongsTo cop
- Loading branch information
Showing
5 changed files
with
292 additions
and
0 deletions.
There are no files selected for viewing
1 change: 1 addition & 0 deletions
1
...og/new_redundant_presence_validation_for_required_belongs_to_association_cop.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#594](https://github.com/rubocop/rubocop-rails/pull/594): Add `Rails/RedundantPresenceValidationOnBelongsTo` cop. ([@pirj][]) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
106 changes: 106 additions & 0 deletions
106
lib/rubocop/cop/rails/redundant_presence_validation_on_belongs_to.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 `%<association>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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
179 changes: 179 additions & 0 deletions
179
spec/rubocop/cop/rails/redundant_presence_validation_on_belongs_to_spec.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |