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

Fix false positive for UniqueValidationWithoutIndex when using conditions #213

Merged

Conversation

sunny
Copy link
Contributor

@sunny sunny commented Mar 24, 2020

Hey everybody!

This fixes a false positive for Rails/UniqueValidationWithoutIndex when using conditions, such as:

class User < ApplicationRecord
  validates :email, uniqueness: true, unless: :guest?
end

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@sunny sunny changed the title No UniqueValidationWithoutIndex offense if validation has a condition Fix positive for UniqueValidationWithoutIndex when using conditions Mar 24, 2020
@sunny sunny changed the title Fix positive for UniqueValidationWithoutIndex when using conditions Fix false positive for UniqueValidationWithoutIndex when using conditions Mar 24, 2020
@koic
Copy link
Member

koic commented Mar 24, 2020

@pocke Can you take a look at this one?

@koic
Copy link
Member

koic commented Mar 25, 2020

Hm, I think this is not a false positive because email address is expected to be unique regardless of conditions.

@sunny
Copy link
Contributor Author

sunny commented Mar 25, 2020

Hi @koic! Thanks for taking the time to review this.

Depending on your application, you may have certain fields that need to validate uniqueness under certain conditions.

Here email does not have a uniqueness validation because it allows blank values in some conditions : the account has created without an email or the user has been anonymised.

Here's another example taken from my application (cults3d.com):

 validates :nick,
           presence: true,
           uniqueness: true,
           length: { minimum: 3 },
           exclusion: { in: ReservedWords.all },
           if: :registered?

In this example the nick is only tested for uniqueness (and other validators) if the user is registered. Until the user is considered registered, the model can have a blank (and therefore non-unique) email.

In both cases we cannot a unique index to these fields, since it would break the application.

Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for showing a specific use case. Perhaps it would be better to make it configurable, but I think this change is acceptable.

@koic koic merged commit bc09d9d into rubocop:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants