Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/RedundantPresenceValidationOnBelongsTo removes other validations on the same line #603

Closed
Narnach opened this issue Dec 27, 2021 · 4 comments · Fixed by #609
Closed
Labels
bug Something isn't working

Comments

@Narnach
Copy link

Narnach commented Dec 27, 2021

The Rails/RedundantPresenceValidationOnBelongsTo cop introduced in #594 deletes entire validates statements with multiple validated attributes, when only one of them is a belongs_to reference.

I've managed to expose the underlying issue in a spec, but I'm not familiar enough (yet) with Rubocop internals to easily come up with a PR that fixes it.


Expected behavior

Applying Rails/RedundantPresenceValidationOnBelongsTo to this code:

belongs_to :user
validates :user, :name, presence: true

Is expected to only remove the :user validation, but leave the :name validation.

belongs_to :user
validates :name, presence: true

Actual behavior

The entire validates line is removed, causing the validation on :name to be lost.

belongs_to :user

Steps to reproduce the problem

I've created a failing spec that exposes the auto-correction bug:

it 'registers an offense when unrelated validations are present' do
  # ignore the ^^^ being misplaced to expose the correction failure
  expect_offense(<<~RUBY)
          belongs_to :user
          validates :user, :name, presence: true
                                  ^^^^^^^^^^^^^^ Remove explicit presence validation for `user`.
  RUBY

  expect_correction(<<~RUBY)
          belongs_to :user
          validates :name, presence: true
  RUBY
end

It fails due to the entire validation line getting deleted.

expected: "belongs_to :user\nvalidates :name, presence: true\n"
     got: "belongs_to :user\n"

I think the offense should only point to :user, resulting in this:

it 'registers an offense when unrelated validations are present' do
  expect_offense(<<~RUBY)
    belongs_to :user
    validates :user, :name, presence: true
              ^^^^^^^ Remove explicit presence validation for `user`.
  RUBY

    expect_correction(<<~RUBY)
    belongs_to :user
    validates :name, presence: true
  RUBY
end

RuboCop version

I encountered this in our own app after upgrading to rubocop-rails 2.13.0.
In isolation I can replicate it with the latest rubocop-rails master, 1e9c257.

$ [bundle exec] rubocop -V
1.24.0 (using Parser 3.0.3.2, rubocop-ast 1.15.0, running on ruby 3.0.3 x86_64-darwin21)
  - rubocop-performance 1.12.0
  - rubocop-rails 2.13.0
  - rubocop-rspec 2.5.0
@koic koic added the bug Something isn't working label Dec 27, 2021
@pirj
Copy link
Member

pirj commented Dec 27, 2021

Thanks for reporting, I'm working on a fix.

@pirj
Copy link
Member

pirj commented Dec 30, 2021

@Narnach, @texpert, @pyromaniac I've fixed this in #609, does fix your cases?

@texpert
Copy link

texpert commented Dec 30, 2021

@Narnach, @texpert, @pyromaniac I've fixed this in #609, does fix your cases?

Yay, works like a charm!

Thanks, @pirj

pirj added a commit to pirj/rubocop-rails that referenced this issue Dec 31, 2021
pirj added a commit to pirj/rubocop-rails that referenced this issue Dec 31, 2021
@Narnach
Copy link
Author

Narnach commented Dec 31, 2021

@Narnach, @texpert, @pyromaniac I've fixed this in #609, does fix your cases?

The test cases cover the problems I encountered and even went further by decomposing a presence/uniqueness combined case. Excellent work! :shipit:

@koic koic closed this as completed in #609 Dec 31, 2021
koic added a commit that referenced this issue Dec 31, 2021
[Fix #603] Teach RedundantPresenceValidationOnBelongsTo to work with multi-attribute validations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants