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

Add Rails/UniqueValidationWithoutIndex cop #197

Merged
merged 1 commit into from Feb 17, 2020

Conversation

@pocke
Copy link
Member

pocke commented Feb 8, 2020

Target problem

The target is the ActiveRecord's uniqueness validation.

When you define uniqueness validation in a model, you also should add a unique index.
It has two reasons.

First, duplicated records may occur even if ActiveRecord's validation is defined.
The Rails Guide mentions this problem.

This helper validates that the attribute's value is unique right before the object gets saved. It does not create a uniqueness constraint in the database, so it may happen that two different database connections create two records with the same value for a column that you intend to be unique. To avoid that, you must create a unique index on that column in your database.
https://guides.rubyonrails.org/active_record_validations.html#uniqueness

Second, it will cause slow queries.
The validation executes a SELECT statement with the target column when inserting/updating a record. If the column does not have an index and the table is large, the query will be heavy.

Solution

Add a Rails/UniqueValidationWithoutIndex cop to detect the problem.

And, I implemented a db/schema.rb loader for the cop.
So now we can implement cops with DB schema information! I think we can more powerful cops for ActiveRecord with the loader.

Note

This pull request adds active_support dependency. But it actually does not increase dependencies in most cases because rubocop-rails is used in Rails projects.

Limitation

This cop ignores validates_uniqueness_of method in the current implementation.
Of course, it's better if the cop supports validates_uniqueness_of. But personally I think it is not important. Because Rails/Validation cop marks it "old-style", so I guess not many users use the method, especially rubocop-rails users.

But if someone sends a pull request to support the method, it will be acceptable. I just omitted it now to keep the first PR simple.


Before submitting the PR make sure the following are checked:

  • 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.
@pocke pocke changed the title [WIP] Rails/unique validation without index [WIP] Add Rails/UniqueValidationWithoutIndex cop Feb 8, 2020
@pocke pocke force-pushed the pocke:Rails/UniqueValidationWithoutIndex branch 2 times, most recently from ac6cf91 to d23f303 Feb 8, 2020
module Rails
# It loads db/schema.rb and return Schema object.
# Cops refers database schema information with this module.
module SchemaLoader

This comment has been minimized.

Copy link
@pocke

pocke Feb 8, 2020

Author Member

This is the schema loader.

I implemented it as a mixin at first, but it was a bad idea. So now it is a non-mixin module.

If it is a mixin, the @schema cache only works for a single cop and single file. So it needs to parse db/schema.rb many times.
But if the non-mixin module, just once parsing is enough.

@@ -496,6 +496,13 @@ Rails/UniqBeforePluck:
- aggressive
AutoCorrect: false

Rails/UniqueValidationWithoutIndex:
Description: 'Uniqueness validation should be with a unique index.'
Enabled: true

This comment has been minimized.

Copy link
@pocke

pocke Feb 8, 2020

Author Member

Should be the status pending, or not? I'm not sure, so if it should be pending, please tell me 🙏

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

RuboCop core has not yet released cop with pending. Here, let's try with true first.

@pocke pocke changed the title [WIP] Add Rails/UniqueValidationWithoutIndex cop Add Rails/UniqueValidationWithoutIndex cop Feb 8, 2020
@pocke pocke marked this pull request as ready for review Feb 8, 2020
@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Feb 8, 2020

Oops, I found a false positive with validation for a relation.

# If the table has a unique index for `user_id` column, the following validation is not offensive.
# But the cop registers an offense to the validation.
belongs_to :user
validates :user, uniqueness: true

I'll fix it soon.

@pocke pocke force-pushed the pocke:Rails/UniqueValidationWithoutIndex branch from d23f303 to 585b2d9 Feb 8, 2020
@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Feb 8, 2020

Oops, I found a false positive with validation for a relation.

Fixed and force-pushed.


module RuboCop
module Cop
# A mixin to extend cops for ActiveRecord features

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

The proper notation is Active Record in this case. Can you update it in this PR?

The proper names of Rails components have a space in between the words, like "Active Support". ActiveRecord is a Ruby module, whereas Active Record is an ORM.

https://github.com/rails/rails/blob/v6.0.2.1/guides/source/api_documentation_guidelines.md#wording

module ActiveRecordHelper
extend NodePattern::Macros

def_node_search :find_set_table_name, <<-PATTERN

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

Can you use squiggly heredoc in this PR?

This comment has been minimized.

Copy link
@pocke

pocke Feb 9, 2020

Author Member

Oops, I guess the cause is my old-style snippets. Thank you notice that!
https://github.com/pocke/dotfiles/blob/c5def33338c0a9f4b32086986c0fc4fd6250f4ff/snippets/ruby.snip#L73-L83

class UniqueValidationWithoutIndex < Cop
include ActiveRecordHelper

MSG = 'Uniqueness validation should be with a unique index'

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

Can you add a period to the end of the message?

@@ -4,6 +4,7 @@ $LOAD_PATH.unshift File.expand_path('lib', __dir__)
require 'rubocop/rails/version'
require 'English'

# rubocop:disable Metrics/BlockLength

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

Can you specify in the exclusion list of this cop at .rubocop.yml?
https://github.com/rubocop-hq/rubocop-rails/blob/v2.4.2/.rubocop.yml#L80-L84

@@ -33,6 +34,8 @@ Gem::Specification.new do |s|

# Rack::Utils::SYMBOL_TO_STATUS_CODE, which is used by HttpStatus cop, was
# introduced in rack 1.1
s.add_runtime_dependency 'activesupport'

This comment has been minimized.

Copy link
@koic

koic Feb 9, 2020

Member

This dependency should be added above the comment for Rack :-)

This comment has been minimized.

Copy link
@pocke

pocke Feb 9, 2020

Author Member

I did not confirm the result of sorting the dependencies by rubocop -a. 🙈

@pocke pocke force-pushed the pocke:Rails/UniqueValidationWithoutIndex branch from 585b2d9 to b3797d5 Feb 9, 2020
@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Feb 9, 2020

I confirmed the comments and fixed them, then force-pushed. Thank you for reviewing!

@andyw8

This comment has been minimized.

Copy link
Member

andyw8 commented Feb 9, 2020

What about if someone has set config.active_record.schema_format = :sql? There will no db/schema.rb

module RuboCop
module Cop
module Rails
# When you define uniqueness validation in Active Record model,

This comment has been minimized.

Copy link
@andyw8

andyw8 Feb 9, 2020

Member

Missing 'a' before 'define'?

module Cop
module Rails
# When you define uniqueness validation in Active Record model,
# you also should add a unique index for the column. It has two reasons.

This comment has been minimized.

Copy link
@andyw8

andyw8 Feb 9, 2020

Member

'It has two reasons' reads a little awkward – maybe change to 'There are two reasons' or 'There are two reasons for this'?


# It parses `db/schema.rb` and return it.
# It returns `nil` if it can't find `db/schema.rb`.
# So a cop that uses the loader should handle `nil` properly.

This comment has been minimized.

Copy link
@andyw8

andyw8 Feb 9, 2020

Member

What do you think about having it raise an exception if schema.rb is missing?

This comment has been minimized.

Copy link
@pocke

pocke Feb 14, 2020

Author Member

Personally I think missing schema.rb should be handled, it means it shouldn't display a stack trace. Because it is not a bug of rubocop-rails.
But I think it is a good idea to notify missing shcema.rb to the user. So, how about a warning instead of an exception?
For example: "db/schema.rb is missing. Disable Rails/UniqueValidationWithoutIndex cop if you don't use db/schema.rb."

This comment has been minimized.

Copy link
@koic

koic Feb 15, 2020

Member

I don't think there is a need to raise warnings or errors. OTOH, documenting the dependency on db/schema.rb will help users.

This comment has been minimized.

Copy link
@pocke

pocke Feb 17, 2020

Author Member

I do not have a strong opinion for it because I think both of them have pros and cons.
The warning can tell users that the cop does not work. It may be a bug, or using schema.sql instead.
The current behavior conceals it. but it will be easy to use rubocop-rails, because no-configuration is necessary even if the cop doesn't work.

By the way, I updated the documentation to mention existence of db/schema.rb.
https://github.com/rubocop-hq/rubocop-rails/pull/197/files#diff-af4cb6c7e18c12f0a26b2ef45466f731R15


context 'without db/schema.rb' do
it 'does nothing' do
expect_no_offenses(<<-RUBY)

This comment has been minimized.

Copy link
@koic

koic Feb 10, 2020

Member

There are still some <<- :-)

@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Feb 14, 2020

What about if someone has set config.active_record.schema_format = :sql? There will no db/schema.rb

Currently the cop does nothing if db/schema.rb is missing.
I guess we can support the sql format, but I guess it is more difficult than parsing db/schema.rb.
Pull request for sql format is welcome, but I do not have any plan for it.

@koic

This comment has been minimized.

Copy link
Member

koic commented Feb 15, 2020

Yeah. I think the cop's responsibility should not handle the existence of db/schema.rb. Each cop responsibility should be simple. So, it should be essentially SRP: Single Responsibility Principle.

@pocke pocke force-pushed the pocke:Rails/UniqueValidationWithoutIndex branch from b3797d5 to 9c9eac6 Feb 17, 2020
@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Feb 17, 2020

I applied the reviewed comments, Thanks!
And I commented on the comment thread. #197 (comment)

@koic I like the both of warning and no-warning ideas, so if you'd like no-warning, could you merge this pull request?

@koic

This comment has been minimized.

Copy link
Member

koic commented Feb 17, 2020

Yeah, I'm going to merge this one! Thanks!

@koic koic merged commit 2a217d0 into rubocop-hq:master Feb 17, 2020
7 checks passed
7 checks passed
ci/circleci: JRuby 9.2 Your tests passed on CircleCI!
Details
ci/circleci: Ruby 2.3 Your tests passed on CircleCI!
Details
ci/circleci: Ruby 2.4 Your tests passed on CircleCI!
Details
ci/circleci: Ruby 2.5 Your tests passed on CircleCI!
Details
ci/circleci: Ruby 2.6 Your tests passed on CircleCI!
Details
ci/circleci: Ruby 2.7 Your tests passed on CircleCI!
Details
ci/circleci: Ruby HEAD Your tests passed on CircleCI!
Details
@pocke pocke deleted the pocke:Rails/UniqueValidationWithoutIndex branch Feb 17, 2020
JonathanHallam added a commit to alphagov/whitehall that referenced this pull request Mar 26, 2020
rubocop-hq/rubocop-rails#197 introduced a new cop to rubocop-rails that checks for unique validation on an index if it's present in the model. This caused a number of failures when upgrading to 2.5.0 that this fixes.
JonathanHallam added a commit to alphagov/whitehall that referenced this pull request Mar 26, 2020
rubocop-hq/rubocop-rails#197 introduced a new cop to rubocop-rails that checks for unique validation on an index if it's present in the model. This caused a number of failures when upgrading to 2.5.0 that this fixes.
JonathanHallam added a commit to alphagov/whitehall that referenced this pull request Mar 26, 2020
rubocop-hq/rubocop-rails#197 introduced a new cop to rubocop-rails that checks for unique validation on an index if it's present in the model. This caused a number of failures when upgrading to 2.5.0 that this fixes.
JonathanHallam added a commit to alphagov/whitehall that referenced this pull request Mar 27, 2020
rubocop-hq/rubocop-rails#197 introduced a new cop to rubocop-rails that checks for unique validation on an index if it's present in the model. This caused a number of failures when upgrading to 2.5.0 that this fixes.
JonathanHallam added a commit to alphagov/whitehall that referenced this pull request Mar 30, 2020
rubocop-hq/rubocop-rails#197 introduced a new cop to rubocop-rails that checks for unique validation on an index if it's present in the model. This caused a number of failures when upgrading to 2.5.0 that this fixes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.