Fixes Rails/UniqueValidationWithoutIndex (part of #11482) #13163
Conversation
There was a problem hiding this comment.
Thanks @cyrillefr 🙏 , There is a mistake on the spree_tax_categories migration file, could you fix that ?
@openfoodfoundation/developers I added the SQL we should run to check for duplicates on the prod servers before we run the migrations. Could you have a quick review of them ?
| remove_index :customers, :email, name: :index_customers_on_email | ||
| add_index(:customers, [:email, :enterprise_id], unique: true, name: :index_customers_on_email) |
There was a problem hiding this comment.
SELECT email, enterprise_id, COUNT(*) FROM customers GROUP BY email, enterprise_id HAVING count(*) > 1;
There was a problem hiding this comment.
We have a problem here.
openfoodnetwork.org.au | CHANGED | rc=0 >>
(1 row)
openfoodnetwork.org.nz | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.in | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.ca | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.net | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.de | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.org.uk | CHANGED | rc=0 >>
(0 rows)
app.katuma.org | CHANGED | rc=0 >>
(0 rows)
openfoodnetwork.ie | CHANGED | rc=0 >>
(0 rows)
coopcircuits.fr | CHANGED | rc=0 >>
(4 rows)
openfood.hu | CHANGED | rc=0 >>
(300 rows)
openfoodnetwork.be | CHANGED | rc=0 >>
(0 rows)
The Australian server has only one test account in there that we can probably remove easily. The French email addresses all look like fake test accounts as well. But I'm not sure what's going on with openfood.hu. That needs some investigation. All those customer accounts are for the same enterprise (id 3) except one (id 60). One email is n/a and has 1718 customer records. That looks like someone did manual SQL queries. Maybe some data import?
| remove_index :exchanges, :sender_id, name: :index_exchanges_on_sender_id | ||
| add_index(:exchanges, [:sender_id, :order_cycle_id, :receiver_id, :incoming], |
There was a problem hiding this comment.
SELECT sender_id, order_cycle_id, receiver_id, incoming, COUNT(*) FROM exchanges GROUP BY sender_id, order_cycle_id, receiver_id, incoming HAVING COUNT(*) >1;There was a problem hiding this comment.
We have some offenders here, too:
$ ansible all_prod -u openfoodnetwork -a "psql -h localhost openfoodnetwork ofn_user -c '$sql'"
openfoodnetwork.org.au | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.org.nz | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.net | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.ca | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
coopcircuits.fr | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
3322 | 12333 | 3226 | t | 2
703 | 12260 | 1855 | t | 2
5792 | 31804 | 5792 | f | 2
209 | 7681 | 347 | t | 2
3949 | 13878 | 1680 | t | 2
3468 | 12333 | 3226 | t | 2
370 | 10482 | 370 | f | 2
3156 | 12260 | 1855 | t | 2
1680 | 13878 | 1680 | t | 2
(9 rows)
openfoodnetwork.de | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.in | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.org.uk | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
204016 | 217768 | 201237 | t | 2
201237 | 210286 | 201237 | f | 2
(2 rows)
openfoodnetwork.be | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
91 | 3996 | 91 | f | 2
(1 row)
openfood.hu | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
app.katuma.org | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
(0 rows)
openfoodnetwork.ie | CHANGED | rc=0 >>
sender_id | order_cycle_id | receiver_id | incoming | count
-----------+----------------+-------------+----------+-------
36 | 704 | 36 | f | 2
(1 row)
Those numbers are small enough to resolve manually, I think.
There was a problem hiding this comment.
We have four servers to resolve:
- coopcircuits.fr
- openfoodnetwork.org.uk
- openfoodnetwork.be
- openfoodnetwork.ie
There was a problem hiding this comment.
I had a look at openfoodnetwork.ie and the duplicate exchange has been created 100ms after the first one. I'm not sure where the glitch is. Maybe something in the Javascript sending two items? They are exactly the same. So I can delete one.
db/migrate/20250215083320_add_unique_index_variant_id_and_deleted_at_to_stock_items.rb
Outdated
Show resolved
Hide resolved
db/migrate/20250215085838_add_unique_index_name_and_deleted_at_to_tax_categories.rb
Outdated
Show resolved
Hide resolved
| remove_index :customers, :email, name: :index_customers_on_email | ||
| add_index(:customers, [:email, :enterprise_id], unique: true, name: :index_customers_on_email) |
There was a problem hiding this comment.
I was wondering why you removed the previous index instead of adding a new one. Apparently, when you have a multi-column index, you can also use it for the first column of that index alone. So queries by email alone are covered by the new index as well. 👍
There was a problem hiding this comment.
I have added the run migration in the "What should we test" bullet.
| remove_index :customers, :email, name: :index_customers_on_email | ||
| add_index(:customers, [:email, :enterprise_id], unique: true, name: :index_customers_on_email) |
There was a problem hiding this comment.
Why did you set the name of the index again? Do we care?
There was a problem hiding this comment.
I did not know this was optional actually. I will remove wherever I can. I might leave some when generated names hit the length max size (63 if I remember right).
…11482) - When you define a uniqueness validation in Active Record model, you also should add a unique index for the column. - Cf. https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsuniquevalidationwithoutindex - Therefore : migration files to match DB structure and Ruby code.
cyrillefr
force-pushed
the
FixRailsUniqueValidationWithoutIndex
branch
from
36bc465 to
680b582
Compare
|
Just added |
mkllnk
added
blocked
technical changes only
What? Why?
Contributes to #11482
The Rails/UniqueValidationWithoutIndex cop checks (via the db/schema.rb file) that AR models indexes are also defined at the DB level.
Example : -when you define a uniqueness validation in Active Record model, you also should add a unique index for the column.
What should we test?
Automatic specs should all pass.
Migration should proceed without any errors. (ie running
rails db:migrate)Any error would mean data inserted that do not respect DB constraints.
In dev/test environment, it should be fine.
But the real test would be in instances. Though checks in AR models should have prevented bad data to have been inserted.
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.