Fix Rubocop Rails/UniqueValidationWithoutIndex cop (#27461)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

.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?

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

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

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

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

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
 

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