From 2ec9bff36e3a7efef501ec796d27cd73cede904c Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 22 Apr 2024 04:04:05 -0400 Subject: [PATCH] Fix Rubocop `Rails/UniqueValidationWithoutIndex` cop (#27461) Co-authored-by: Claire --- .rubocop_todo.yml | 9 ---- ...o_webauthn_credentials_user_id_nickname.rb | 49 +++++++++++++++++++ ...d_index_to_account_alias_uri_account_id.rb | 49 +++++++++++++++++++ ...om_filter_statuses_status_custom_filter.rb | 49 +++++++++++++++++++ ...59_add_index_to_identities_uid_provider.rb | 49 +++++++++++++++++++ db/schema.rb | 4 ++ lib/tasks/tests.rake | 39 +++++++++++++++ 7 files changed, 239 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20231018192110_add_index_to_webauthn_credentials_user_id_nickname.rb create mode 100644 db/migrate/20231018193209_add_index_to_account_alias_uri_account_id.rb create mode 100644 db/migrate/20231018193355_add_index_to_custom_filter_statuses_status_custom_filter.rb create mode 100644 db/migrate/20231018193659_add_index_to_identities_uid_provider.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0bc9d2831c9..3f2e9aee62e 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -54,15 +54,6 @@ Rails/OutputSafety: Exclude: - 'config/initializers/simple_form.rb' -# Configuration parameters: Include. -# Include: app/models/**/*.rb -Rails/UniqueValidationWithoutIndex: - Exclude: - - 'app/models/account_alias.rb' - - 'app/models/custom_filter_status.rb' - - 'app/models/identity.rb' - - 'app/models/webauthn_credential.rb' - # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: AllowedMethods, AllowedPatterns. # AllowedMethods: ==, equal?, eql? diff --git a/db/migrate/20231018192110_add_index_to_webauthn_credentials_user_id_nickname.rb b/db/migrate/20231018192110_add_index_to_webauthn_credentials_user_id_nickname.rb new file mode 100644 index 00000000000..2e608eeade8 --- /dev/null +++ b/db/migrate/20231018192110_add_index_to_webauthn_credentials_user_id_nickname.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class AddIndexToWebauthnCredentialsUserIdNickname < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def up + add_index_to_table + rescue ActiveRecord::RecordNotUnique + remove_duplicates_and_reindex + end + + def down + remove_index_from_table + end + + private + + def remove_duplicates_and_reindex + deduplicate_records + reindex_records + rescue ActiveRecord::RecordNotUnique + retry + end + + def reindex_records + remove_index_from_table + add_index_to_table + end + + def add_index_to_table + add_index :webauthn_credentials, [:user_id, :nickname], unique: true, algorithm: :concurrently + end + + def remove_index_from_table + remove_index :webauthn_credentials, [:user_id, :nickname] + end + + def deduplicate_records + safety_assured do + execute <<~SQL.squish + DELETE FROM webauthn_credentials + WHERE id NOT IN ( + SELECT DISTINCT ON(user_id, nickname) id FROM webauthn_credentials + ORDER BY user_id, nickname, id ASC + ) + SQL + end + end +end diff --git a/db/migrate/20231018193209_add_index_to_account_alias_uri_account_id.rb b/db/migrate/20231018193209_add_index_to_account_alias_uri_account_id.rb new file mode 100644 index 00000000000..4d9036fdf8e --- /dev/null +++ b/db/migrate/20231018193209_add_index_to_account_alias_uri_account_id.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class AddIndexToAccountAliasUriAccountId < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def up + add_index_to_table + rescue ActiveRecord::RecordNotUnique + remove_duplicates_and_reindex + end + + def down + remove_index_from_table + end + + private + + def remove_duplicates_and_reindex + deduplicate_records + reindex_records + rescue ActiveRecord::RecordNotUnique + retry + end + + def reindex_records + remove_index_from_table + add_index_to_table + end + + def add_index_to_table + add_index :account_aliases, [:account_id, :uri], unique: true, algorithm: :concurrently + end + + def remove_index_from_table + remove_index :account_aliases, [:account_id, :uri] + end + + def deduplicate_records + safety_assured do + execute <<~SQL.squish + DELETE FROM account_aliases + WHERE id NOT IN ( + SELECT DISTINCT ON(account_id, uri) id FROM account_aliases + ORDER BY account_id, uri, id ASC + ) + SQL + end + end +end diff --git a/db/migrate/20231018193355_add_index_to_custom_filter_statuses_status_custom_filter.rb b/db/migrate/20231018193355_add_index_to_custom_filter_statuses_status_custom_filter.rb new file mode 100644 index 00000000000..4a23a8a9431 --- /dev/null +++ b/db/migrate/20231018193355_add_index_to_custom_filter_statuses_status_custom_filter.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class AddIndexToCustomFilterStatusesStatusCustomFilter < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def up + add_index_to_table + rescue ActiveRecord::RecordNotUnique + remove_duplicates_and_reindex + end + + def down + remove_index_from_table + end + + private + + def remove_duplicates_and_reindex + deduplicate_records + reindex_records + rescue ActiveRecord::RecordNotUnique + retry + end + + def reindex_records + remove_index_from_table + add_index_to_table + end + + def add_index_to_table + add_index :custom_filter_statuses, [:status_id, :custom_filter_id], unique: true, algorithm: :concurrently + end + + def remove_index_from_table + remove_index :custom_filter_statuses, [:status_id, :custom_filter_id] + end + + def deduplicate_records + safety_assured do + execute <<~SQL.squish + DELETE FROM custom_filter_statuses + WHERE id NOT IN ( + SELECT DISTINCT ON(status_id, custom_filter_id) id FROM custom_filter_statuses + ORDER BY status_id, custom_filter_id, id ASC + ) + SQL + end + end +end diff --git a/db/migrate/20231018193659_add_index_to_identities_uid_provider.rb b/db/migrate/20231018193659_add_index_to_identities_uid_provider.rb new file mode 100644 index 00000000000..f0ba08b57b4 --- /dev/null +++ b/db/migrate/20231018193659_add_index_to_identities_uid_provider.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +class AddIndexToIdentitiesUidProvider < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def up + add_index_to_table + rescue ActiveRecord::RecordNotUnique + remove_duplicates_and_reindex + end + + def down + remove_index_from_table + end + + private + + def remove_duplicates_and_reindex + deduplicate_records + reindex_records + rescue ActiveRecord::RecordNotUnique + retry + end + + def reindex_records + remove_index_from_table + add_index_to_table + end + + def add_index_to_table + add_index :identities, [:uid, :provider], unique: true, algorithm: :concurrently + end + + def remove_index_from_table + remove_index :identities, [:uid, :provider] + end + + def deduplicate_records + safety_assured do + execute <<~SQL.squish + DELETE FROM identities + WHERE id NOT IN ( + SELECT DISTINCT ON(uid, provider) id FROM identities + ORDER BY uid, provider, id ASC + ) + SQL + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 27c486487d8..6a52333a80e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -20,6 +20,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.string "uri", default: "", null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false + t.index ["account_id", "uri"], name: "index_account_aliases_on_account_id_and_uri", unique: true t.index ["account_id"], name: "index_account_aliases_on_account_id" end @@ -395,6 +396,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["custom_filter_id"], name: "index_custom_filter_statuses_on_custom_filter_id" + t.index ["status_id", "custom_filter_id"], name: "index_custom_filter_statuses_on_status_id_and_custom_filter_id", unique: true t.index ["status_id"], name: "index_custom_filter_statuses_on_status_id" end @@ -545,6 +547,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.bigint "user_id" + t.index ["uid", "provider"], name: "index_identities_on_uid_and_provider", unique: true t.index ["user_id"], name: "index_identities_on_user_id" end @@ -1235,6 +1238,7 @@ ActiveRecord::Schema[7.1].define(version: 2024_03_22_161611) do t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.index ["external_id"], name: "index_webauthn_credentials_on_external_id", unique: true + t.index ["user_id", "nickname"], name: "index_webauthn_credentials_on_user_id_and_nickname", unique: true t.index ["user_id"], name: "index_webauthn_credentials_on_user_id" end diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 6afc013eded..0caebf92a18 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -8,6 +8,7 @@ namespace :tests do '2' => 2017_10_10_025614, '2_4' => 2018_05_14_140000, '2_4_3' => 2018_07_07_154237, + '3_3_0' => 2020_12_18_054746, }.each do |release, version| ActiveRecord::Tasks::DatabaseTasks .migration_connection @@ -111,9 +112,41 @@ namespace :tests do exit(1) end + unless Identity.where(provider: 'foo', uid: 0).count == 1 + puts 'Identities not deduplicated as expected' + exit(1) + end + + unless WebauthnCredential.where(user_id: 1, nickname: 'foo').count == 1 + puts 'Webauthn credentials not deduplicated as expected' + exit(1) + end + + unless AccountAlias.where(account_id: 1, uri: 'https://example.com/users/foobar').count == 1 + puts 'Account aliases not deduplicated as expected' + exit(1) + end + puts 'No errors found. Database state is consistent with a successful migration process.' end + desc 'Populate the database with test data for 3.3.0' + task populate_v3_3_0: :environment do # rubocop:disable Naming/VariableNumber + ActiveRecord::Base.connection.execute(<<~SQL.squish) + INSERT INTO "webauthn_credentials" + (user_id, nickname, external_id, public_key, created_at, updated_at) + VALUES + (1, 'foo', 1, 'foo', now(), now()), + (1, 'foo', 2, 'bar', now(), now()); + + INSERT INTO "account_aliases" + (account_id, uri, acct, created_at, updated_at) + VALUES + (1, 'https://example.com/users/foobar', 'foobar@example.com', now(), now()), + (1, 'https://example.com/users/foobar', 'foobar@example.com', now(), now()); + SQL + end + desc 'Populate the database with test data for 2.4.3' task populate_v2_4_3: :environment do # rubocop:disable Naming/VariableNumber user_key = OpenSSL::PKey::RSA.new(2048) @@ -189,6 +222,12 @@ namespace :tests do VALUES (5, 'User', 4, 'default_language', E'--- kmr\n', now(), now()), (6, 'User', 1, 'interactions', E'--- !ruby/hash:ActiveSupport::HashWithIndifferentAccess\nmust_be_follower: false\nmust_be_following: true\nmust_be_following_dm: false\n', now(), now()); + + INSERT INTO "identities" + (provider, uid, user_id, created_at, updated_at) + VALUES + ('foo', 0, 1, now(), now()), + ('foo', 0, 1, now(), now()); SQL end