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/UniqueValidationWithoutIndex should not fail when using scope #231

Open
nickserv opened this issue Apr 10, 2020 · 13 comments
Open

Rails/UniqueValidationWithoutIndex should not fail when using scope #231

nickserv opened this issue Apr 10, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@nickserv
Copy link

nickserv commented Apr 10, 2020

A uniqueness validation over a scope should not fail Rails/UniqueValidationWithoutIndex, as a unique index would not necessarily be unique for the entire table when it's unique under the scope.


Expected behavior

Cop should not fail when a scope is used to prevent false positives and not encourage the user to change their schema in a way that could break scoped uniqueness functionality.

Actual behavior

Rails/UniqueValidationWithoutIndex: Uniqueness validation should be with a unique index.

Steps to reproduce the problem

validates :field_1, uniqueness: { scope: :field_2 }

RuboCop version

rubocop (0.81.0)
rubocop-rails (2.5.2)
@koic
Copy link
Member

koic commented Apr 12, 2020

RuboCop Rails 2.5.2 has an expected tests for uniqueness: { scope: :field_2 }.
https://github.com/rubocop-hq/rubocop-rails/blob/v2.5.2/spec/rubocop/cop/rails/unique_validation_without_index_spec.rb#L117-L135

Rails/UniqueValidationWithoutIndex cop inspects a model and db/schema.rb as a set.
Can you show me the model and db/schema.rb as a set?

@nickserv
Copy link
Author

nickserv commented Apr 13, 2020

@koic
Copy link
Member

koic commented Apr 13, 2020

Thank you showing code.

This issue seems to be the expected behavior, not a bug.
Here is a quote from #197 where Rails/UniqueValidationWithoutIndex cop were introduced:

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.

In this case, it is expected that the following unique index is generated in db/schema.rb.

t.index ["field_1", "field_2"], name: "idx_field_1_field_2", unique: true

@nickserv
Copy link
Author

Thanks for the explanation. Would it be possible for the cop to give a more specific message? I thought I already had an index, but didn’t realize it was incorrect.

@koic
Copy link
Member

koic commented May 6, 2020

Yeah, the offense message can be changed. On the other hand, offense messages are preferably simple, concise and short explanation. I'm not getting a good message yet. Do you have any good explanation?

@koic koic added the enhancement New feature or request label May 6, 2020
@nickserv
Copy link
Author

nickserv commented May 6, 2020

I don’t really know how to describe that syntax. Perhaps the error could just have a one-line code example of what’s expected in the model.

@kortirso
Copy link

@koic , I have same problem for scope with polymorphic

belongs_to :fieldable, polymorphic: true
validates :type, uniqueness: { scope: :fieldable }

I tried to add index with these migrations (with and without name options)
add_index :fields, [:type, :fieldable_type, :fieldable_id], name: "index_fields_on_type_and_fieldable", unique: true, algorithm: :concurrently
add_index :fields, [:fieldable_type, :fieldable_id, :type], name: "index_fields_on_type_and_fieldable", unique: true, algorithm: :concurrently

last version of schema
t.index ["type", "fieldable_type", "fieldable_id"], name: "index_fields_on_type_and_fieldable", unique: true

@tubbo
Copy link

tubbo commented Jul 18, 2020

this is happening for me as well.

model:

  validates :name, presence: true, uniqueness: { scope: :author }

schema:

  create_table "apps", force: :cascade do |t|
    t.string "name", null: false
    t.string "author_type", null: false
    t.bigint "author_id", null: false
    t.citext "authentication_token", null: false
    t.integer "failed_attempts", default: 0, null: false
    t.citext "unlock_token"
    t.datetime "locked_at"
    t.integer "sign_in_count", default: 0, null: false
    t.datetime "current_sign_in_at"
    t.datetime "last_sign_in_at"
    t.inet "current_sign_in_ip"
    t.inet "last_sign_in_ip"
    t.datetime "created_at", precision: 6, null: false
    t.datetime "updated_at", precision: 6, null: false
    t.index ["author_id", "author_type", "name"], name: "index_apps_on_author_id_and_author_type_and_name", unique: true
    t.index ["author_type", "author_id"], name: "index_apps_on_author_type_and_author_id"
  end

It seems like maybe the cop isn't checking for _type as well as _id in the list of indexes? Makes sense as this doesn't fail for me on a regular belongs_to:

model:

  validates :name, presence: true, uniqueness: { scope: :version }

schema:

  create_table "docs", force: :cascade do |t|
    t.bigint "version_id"
    t.string "name", null: false
    t.datetime "created_at", precision: 6, null: false
    t.datetime "updated_at", precision: 6, null: false
    t.index ["name", "version_id"], name: "index_docs_on_name_and_version_id", unique: true
    t.index ["name"], name: "index_docs_on_name"
    t.index ["version_id"], name: "index_docs_on_version_id"
  end

@alec-c4
Copy link

alec-c4 commented Jun 17, 2021

The same problem :(

model:

  validates :employee, uniqueness: { scope: :employable, conditions: -> { where(active: true) } }

migration:

    add_index :employments, [:id, :employable_id, :employee_id, :active], unique: true, name: "active_employments_index"

@schmijos
Copy link

schmijos commented Jun 29, 2021

I had to disable this cop for the following code:

Schema:

  create_table "time_slot_bookings", force: :cascade do |t|
    t.string "state", default: "pending"
    t.index ["state", "published_time_slot_id"], name: "index_time_slot_bookings_on_state_and_published_time_slot_id"
    

Validation:

  validates :state, uniqueness: { scope: :published_time_slot_id,
                                  conditions: -> { processed },
                                  message: 'should not be double booked' }

While I see that an index makes sense in general, uniqueness on the database is not possible in this case (without manually writing SQL).

@TorionJ
Copy link

TorionJ commented Jan 11, 2023

Hi! I'm still running into this issue...

Gems:
rubocop-rails: 2.15.2
rubocop: 1.34.1
ruby: 3.0.4
rails: 6.1.7

Model:
validates :name, :version, presence: true, uniqueness: { scope: :product_id }

Schema:
t.index ["name", "version", "product_id"], name: "index_ota_releases_on_name_and_version_and_product_id", unique: true

Is there an underlying fix for this issue besides silencing Rubocop?

@fatkodima
Copy link
Contributor

We can probably improve the handling of scopes/polymorphic associations/etc in the rubocop itself (unfortunately by introducing more and more complexity into it), but there is already a tool better suited for such checks and more - https://github.com/gregnavis/active_record_doctor. It has a check for missing unique indexes - https://github.com/gregnavis/active_record_doctor#detecting-uniqueness-validations-not-backed-by-an-index and handles all the mentioned problematic cases.

paulohenrique-gh added a commit to paulohenrique-gh/rails-vetor that referenced this issue Apr 22, 2024
Desabilitado cop Rails/UniqueValidationWithoutIndex pois continua apontando falha mesmo depois de incluir índice de unicidade no banco de dados. Problema é comum conforme rubocop/rubocop-rails#231
@lynnbright
Copy link

lynnbright commented Jun 7, 2024

We're still having this issue in our project as well. We used three methods to verify that the index has taken effect and decided to temporarily ignore the Rails/UniqueValidationWithoutIndex warning.

Gems

  • rails: 6.1.7
  • ruby: 3.0.6
  • rubocop: 1.60.2
  • rubocop-rails: 2.21.2

Model validation

validates :feature_type, presence: true, uniqueness: { scope: [:product_featureable_type, :product_featureable_id] }

Schema

create_table "product_features", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
  t.string "product_featureable_type", null: false
  t.uuid "product_featureable_id", null: false
  ...
  t.string "feature_type"
  t.index ["feature_type", "product_featureable_type", "product_featureable_id"], name: "index_on_feature_type_and_product_featureable", unique: true
  t.index ["product_featureable_type", "product_featureable_id"], name: "index_product_features_on_product_featureable"
end

Verification

1. Verify the index exists in the database

# In terminal

psql -U postgres your_database_name

your_database_name=# SELECT * FROM pg_indexes WHERE tablename = 'product_features' and indexname = 'index_on_feature_type_and_product_featureable';

 schemaname |    tablename     |                   indexname                   | tablespace |                                                                                 indexdef
------------+------------------+-----------------------------------------------+------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 public     | product_features | index_on_feature_type_and_product_featureable |            | CREATE UNIQUE INDEX index_on_feature_type_and_product_featureable ON public.product_features USING btree (feature_type, product_featureable_type, product_featureable_id)
(1 row)

2. Use active_record_doctor gem to identify whether the issue exists.

Before: Try removing one of the columns in the scope.
validates :feature_type, presence: true, uniqueness: { scope: [:product_featureable_id] }
# In terminal

bundle exec rake active_record_doctor

CleanShot 2024-06-07 at 12 53 43@2x

After: Use the full scope that we want.
validates :feature_type, presence: true, uniqueness: { scope: [:product_featureable_type, :product_featureable_id] }
# in your terminal

bundle exec rake active_record_doctor

We expect the message below to be listed, but it isn't. This indicates that the unique index has been added.

add a unique index on product_features(product_featureable_id, product_featureable_type, feature_type) - validating uniqueness in ProductFeature without an index can lead to duplicates

3. Create records manually in Rails console

The validation took effect and the transaction rollback as expected.

CleanShot 2024-06-07 at 13 03 41@2x

CleanShot 2024-06-07 at 13 05 07@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants