diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 886582460..13775e63c 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -61,9 +61,13 @@ class FetchLinkCardService < BaseService end def attach_card - @status.preview_cards << @card - Rails.cache.delete(@status) - Trends.links.register(@status) + with_redis_lock("attach_card:#{@status.id}") do + return if @status.preview_cards.any? + + @status.preview_cards << @card + Rails.cache.delete(@status) + Trends.links.register(@status) + end end def parse_urls diff --git a/db/post_migrate/20230803082451_add_unique_index_on_preview_cards_statuses.rb b/db/post_migrate/20230803082451_add_unique_index_on_preview_cards_statuses.rb new file mode 100644 index 000000000..936d7840e --- /dev/null +++ b/db/post_migrate/20230803082451_add_unique_index_on_preview_cards_statuses.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +class AddUniqueIndexOnPreviewCardsStatuses < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + add_index :preview_cards_statuses, [:status_id, :preview_card_id], name: :preview_cards_statuses_pkey, algorithm: :concurrently, unique: true + rescue ActiveRecord::RecordNotUnique + deduplicate_and_reindex! + end + + def down + remove_index :preview_cards_statuses, name: :preview_cards_statuses_pkey + end + + private + + def deduplicate_and_reindex! + deduplicate_preview_cards! + + safety_assured { execute 'REINDEX INDEX preview_cards_statuses_pkey' } + rescue ActiveRecord::RecordNotUnique + retry + end + + def deduplicate_preview_cards! + # Statuses should have only one preview card at most, even if that's not the database + # constraint we will end up with + duplicate_ids = select_all('SELECT status_id FROM preview_cards_statuses GROUP BY status_id HAVING count(*) > 1;').rows + + duplicate_ids.each_slice(1000) do |ids| + # This one is tricky: since we don't have primary keys to keep only one record, + # use the physical `ctid` + safety_assured do + execute "DELETE FROM preview_cards_statuses p WHERE p.status_id IN (#{ids.join(', ')}) AND p.ctid NOT IN (SELECT q.ctid FROM preview_cards_statuses q WHERE q.status_id = p.status_id LIMIT 1)" + end + end + end +end diff --git a/db/post_migrate/20230803112520_add_primary_key_to_preview_cards_statuses_join_table.rb b/db/post_migrate/20230803112520_add_primary_key_to_preview_cards_statuses_join_table.rb new file mode 100644 index 000000000..34877ac67 --- /dev/null +++ b/db/post_migrate/20230803112520_add_primary_key_to_preview_cards_statuses_join_table.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddPrimaryKeyToPreviewCardsStatusesJoinTable < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + safety_assured do + execute 'ALTER TABLE preview_cards_statuses ADD PRIMARY KEY USING INDEX preview_cards_statuses_pkey' + end + end + + def down + safety_assured do + # I have found no way to demote the primary key to an index, instead, re-create the index + execute 'CREATE UNIQUE INDEX CONCURRENTLY preview_cards_statuses_pkey_tmp ON preview_cards_statuses (status_id, preview_card_id)' + execute 'ALTER TABLE preview_cards_statuses DROP CONSTRAINT preview_cards_statuses_pkey' + execute 'ALTER INDEX preview_cards_statuses_pkey_tmp RENAME TO preview_cards_statuses_pkey' + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 59ab9dbed..0c8ede383 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_07_24_160715) do +ActiveRecord::Schema[7.0].define(version: 2023_08_03_112520) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -805,7 +805,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_24_160715) do t.index ["url"], name: "index_preview_cards_on_url", unique: true end - create_table "preview_cards_statuses", id: false, force: :cascade do |t| + create_table "preview_cards_statuses", primary_key: ["status_id", "preview_card_id"], force: :cascade do |t| t.bigint "preview_card_id", null: false t.bigint "status_id", null: false t.index ["status_id", "preview_card_id"], name: "index_preview_cards_statuses_on_status_id_and_preview_card_id" diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 3c88ce450..ef4d46fe0 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -63,6 +63,11 @@ namespace :tests do puts 'Account domains not properly normalized' exit(1) end + + unless Status.find(12).preview_cards.pluck(:url) == ['https://joinmastodon.org/'] + puts 'Preview cards not deduplicated as expected' + exit(1) + end end desc 'Populate the database with test data for 2.4.3' @@ -238,6 +243,11 @@ namespace :tests do (10, 2, '@admin hey!', NULL, 1, 3, now(), now()), (11, 1, '@user hey!', 10, 1, 3, now(), now()); + INSERT INTO "statuses" + (id, account_id, text, created_at, updated_at) + VALUES + (12, 1, 'check out https://joinmastodon.org/', now(), now()); + -- mentions (from previous statuses) INSERT INTO "mentions" @@ -326,6 +336,21 @@ namespace :tests do (1, 6, 2, 'Follow', 2, now(), now()), (2, 2, 1, 'Mention', 4, now(), now()), (3, 1, 2, 'Mention', 5, now(), now()); + + -- preview cards + + INSERT INTO "preview_cards" + (id, url, title, created_at, updated_at) + VALUES + (1, 'https://joinmastodon.org/', 'Mastodon - Decentralized social media', now(), now()); + + -- many-to-many association between preview cards and statuses + + INSERT INTO "preview_cards_statuses" + (status_id, preview_card_id) + VALUES + (12, 1), + (12, 1); SQL end end