From 0d14fcebae0b9a0e5da73049c960064338dfee8e Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 13 Nov 2023 10:58:28 +0100 Subject: [PATCH] Change link previews to keep original URL from the status (#27312) --- app/chewy/public_statuses_index.rb | 2 +- app/chewy/statuses_index.rb | 2 +- .../api/v1/conversations_controller.rb | 2 +- app/models/admin/status_batch_action.rb | 2 +- app/models/concerns/status_search_concern.rb | 2 +- app/models/preview_card.rb | 7 ++++++- app/models/preview_cards_status.rb | 18 ++++++++++++++++++ app/models/status.rb | 15 ++++++++++----- app/models/trends/links.rb | 4 +--- .../rest/preview_card_serializer.rb | 4 ++++ .../process_status_update_service.rb | 2 +- app/services/fetch_link_card_service.rb | 6 +++--- app/services/update_status_service.rb | 2 +- ...183200_add_url_to_preview_cards_statuses.rb | 7 +++++++ db/schema.rb | 3 ++- lib/tasks/tests.rake | 2 +- spec/helpers/media_component_helper_spec.rb | 4 +++- spec/services/fetch_link_card_service_spec.rb | 8 ++++---- spec/services/update_status_service_spec.rb | 8 ++++---- 19 files changed, 70 insertions(+), 30 deletions(-) create mode 100644 app/models/preview_cards_status.rb create mode 100644 db/migrate/20231006183200_add_url_to_preview_cards_statuses.rb diff --git a/app/chewy/public_statuses_index.rb b/app/chewy/public_statuses_index.rb index 4be204d4a..b5f0be5e5 100644 --- a/app/chewy/public_statuses_index.rb +++ b/app/chewy/public_statuses_index.rb @@ -53,7 +53,7 @@ class PublicStatusesIndex < Chewy::Index index_scope ::Status.unscoped .kept .indexable - .includes(:media_attachments, :preloadable_poll, :preview_cards, :tags) + .includes(:media_attachments, :preloadable_poll, :tags, preview_cards_status: :preview_card) root date_detection: false do field(:id, type: 'long') diff --git a/app/chewy/statuses_index.rb b/app/chewy/statuses_index.rb index 6b25dc9df..e315a2030 100644 --- a/app/chewy/statuses_index.rb +++ b/app/chewy/statuses_index.rb @@ -50,7 +50,7 @@ class StatusesIndex < Chewy::Index }, } - index_scope ::Status.unscoped.kept.without_reblogs.includes(:media_attachments, :preview_cards, :local_mentioned, :local_favorited, :local_reblogged, :local_bookmarked, :tags, preloadable_poll: :local_voters), delete_if: ->(status) { status.searchable_by.empty? } + index_scope ::Status.unscoped.kept.without_reblogs.includes(:media_attachments, :local_mentioned, :local_favorited, :local_reblogged, :local_bookmarked, :tags, preview_cards_status: :preview_card, preloadable_poll: :local_voters), delete_if: ->(status) { status.searchable_by.empty? } root date_detection: false do field(:id, type: 'long') diff --git a/app/controllers/api/v1/conversations_controller.rb b/app/controllers/api/v1/conversations_controller.rb index b3ca2f790..6a3567e62 100644 --- a/app/controllers/api/v1/conversations_controller.rb +++ b/app/controllers/api/v1/conversations_controller.rb @@ -41,10 +41,10 @@ class Api::V1::ConversationsController < Api::BaseController account: :account_stat, last_status: [ :media_attachments, - :preview_cards, :status_stat, :tags, { + preview_cards_status: :preview_card, active_mentions: [account: :account_stat], account: :account_stat, }, diff --git a/app/models/admin/status_batch_action.rb b/app/models/admin/status_batch_action.rb index 24c3979aa..8a8e2fa37 100644 --- a/app/models/admin/status_batch_action.rb +++ b/app/models/admin/status_batch_action.rb @@ -74,7 +74,7 @@ class Admin::StatusBatchAction # Can't use a transaction here because UpdateStatusService queues # Sidekiq jobs - statuses.includes(:media_attachments, :preview_cards).find_each do |status| + statuses.includes(:media_attachments, preview_cards_status: :preview_card).find_each do |status| next if status.discarded? || !(status.with_media? || status.with_preview_card?) authorize([:admin, status], :update?) diff --git a/app/models/concerns/status_search_concern.rb b/app/models/concerns/status_search_concern.rb index 3ef45754a..7252fde73 100644 --- a/app/models/concerns/status_search_concern.rb +++ b/app/models/concerns/status_search_concern.rb @@ -40,7 +40,7 @@ module StatusSearchConcern properties << 'media' if with_media? properties << 'poll' if with_poll? properties << 'link' if with_preview_card? - properties << 'embed' if preview_cards.any?(&:video?) + properties << 'embed' if preview_card&.video? properties << 'sensitive' if sensitive? properties << 'reply' if reply? end diff --git a/app/models/preview_card.rb b/app/models/preview_card.rb index a1751c426..837592743 100644 --- a/app/models/preview_card.rb +++ b/app/models/preview_card.rb @@ -50,7 +50,9 @@ class PreviewCard < ApplicationRecord enum type: { link: 0, photo: 1, video: 2, rich: 3 } enum link_type: { unknown: 0, article: 1 } - has_and_belongs_to_many :statuses + has_many :preview_cards_statuses, dependent: :delete_all, inverse_of: :preview_card + has_many :statuses, through: :preview_cards_statuses + has_one :trend, class_name: 'PreviewCardTrend', inverse_of: :preview_card, dependent: :destroy has_attached_file :image, processors: [:thumbnail, :blurhash_transcoder], styles: ->(f) { image_styles(f) }, convert_options: { all: '-quality 90 +profile "!icc,*" +set date:modify +set date:create +set date:timestamp' }, validate_media_type: false @@ -64,6 +66,9 @@ class PreviewCard < ApplicationRecord before_save :extract_dimensions, if: :link? + # This can be set by the status when retrieving the preview card using the join model + attr_accessor :original_url + def appropriate_for_trends? link? && article? && title.present? && description.present? && image.present? && provider_name.present? end diff --git a/app/models/preview_cards_status.rb b/app/models/preview_cards_status.rb new file mode 100644 index 000000000..341771e4d --- /dev/null +++ b/app/models/preview_cards_status.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: preview_cards_statuses +# +# preview_card_id :bigint(8) not null +# status_id :bigint(8) not null +# url :string +# +class PreviewCardsStatus < ApplicationRecord + # Composite primary keys are not properly supported in Rails. However, + # we shouldn't need this anyway... + self.primary_key = nil + + belongs_to :preview_card + belongs_to :status +end diff --git a/app/models/status.rb b/app/models/status.rb index 1c41ef1d5..41c895029 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -79,8 +79,8 @@ class Status < ApplicationRecord has_many :local_bookmarked, -> { merge(Account.local) }, through: :bookmarks, source: :account has_and_belongs_to_many :tags - has_and_belongs_to_many :preview_cards + has_one :preview_cards_status, inverse_of: :status # Because of a composite primary key, the dependent option cannot be used has_one :notification, as: :activity, dependent: :destroy has_one :status_stat, inverse_of: :status has_one :poll, inverse_of: :status, dependent: :destroy @@ -142,24 +142,25 @@ class Status < ApplicationRecord # The `prepend: true` option below ensures this runs before # the `dependent: destroy` callbacks remove relevant records before_destroy :unlink_from_conversations!, prepend: true + before_destroy :reset_preview_card! cache_associated :application, :media_attachments, :conversation, :status_stat, :tags, - :preview_cards, :preloadable_poll, + preview_cards_status: [:preview_card], account: [:account_stat, user: :role], active_mentions: { account: :account_stat }, reblog: [ :application, :tags, - :preview_cards, :media_attachments, :conversation, :status_stat, :preloadable_poll, + preview_cards_status: [:preview_card], account: [:account_stat, user: :role], active_mentions: { account: :account_stat }, ], @@ -226,7 +227,11 @@ class Status < ApplicationRecord end def preview_card - preview_cards.first + preview_cards_status&.preview_card&.tap { |x| x.original_url = preview_cards_status.url } + end + + def reset_preview_card! + PreviewCardsStatus.where(status_id: id).delete_all end def hidden? @@ -244,7 +249,7 @@ class Status < ApplicationRecord end def with_preview_card? - preview_cards.any? + preview_cards_status.present? end def with_poll? diff --git a/app/models/trends/links.rb b/app/models/trends/links.rb index fcbdb1a5f..b4eae9f70 100644 --- a/app/models/trends/links.rb +++ b/app/models/trends/links.rb @@ -54,9 +54,7 @@ class Trends::Links < Trends::Base !(original_status.account.silenced? || status.account.silenced?) && !(original_status.spoiler_text? || original_status.sensitive?) - original_status.preview_cards.each do |preview_card| - add(preview_card, status.account_id, at_time) if preview_card.appropriate_for_trends? - end + add(original_status.preview_card, status.account_id, at_time) if original_status.preview_card&.appropriate_for_trends? end def add(preview_card, account_id, at_time = Time.now.utc) diff --git a/app/serializers/rest/preview_card_serializer.rb b/app/serializers/rest/preview_card_serializer.rb index 3e1c4bde3..039262cd5 100644 --- a/app/serializers/rest/preview_card_serializer.rb +++ b/app/serializers/rest/preview_card_serializer.rb @@ -8,6 +8,10 @@ class REST::PreviewCardSerializer < ActiveModel::Serializer :provider_url, :html, :width, :height, :image, :image_description, :embed_url, :blurhash, :published_at + def url + object.original_url.presence || object.url + end + def image object.image? ? full_asset_url(object.image.url(:original)) : nil end diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 4ff92da01..2db0e80e7 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -280,7 +280,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end def reset_preview_card! - @status.preview_cards.clear + @status.reset_preview_card! LinkCrawlWorker.perform_in(rand(1..59).seconds, @status.id) end diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 13775e63c..c6b600dd7 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -19,7 +19,7 @@ class FetchLinkCardService < BaseService @status = status @original_url = parse_urls - return if @original_url.nil? || @status.preview_cards.any? + return if @original_url.nil? || @status.with_preview_card? @url = @original_url.to_s @@ -62,9 +62,9 @@ class FetchLinkCardService < BaseService def attach_card with_redis_lock("attach_card:#{@status.id}") do - return if @status.preview_cards.any? + return if @status.with_preview_card? - @status.preview_cards << @card + PreviewCardsStatus.create(status: @status, preview_card: @card, url: @original_url) Rails.cache.delete(@status) Trends.links.register(@status) end diff --git a/app/services/update_status_service.rb b/app/services/update_status_service.rb index d1c2b990f..cdfe28365 100644 --- a/app/services/update_status_service.rb +++ b/app/services/update_status_service.rb @@ -123,7 +123,7 @@ class UpdateStatusService < BaseService def reset_preview_card! return unless @status.text_previously_changed? - @status.preview_cards.clear + @status.reset_preview_card! LinkCrawlWorker.perform_async(@status.id) end diff --git a/db/migrate/20231006183200_add_url_to_preview_cards_statuses.rb b/db/migrate/20231006183200_add_url_to_preview_cards_statuses.rb new file mode 100644 index 000000000..f7c6de462 --- /dev/null +++ b/db/migrate/20231006183200_add_url_to_preview_cards_statuses.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddURLToPreviewCardsStatuses < ActiveRecord::Migration[7.0] + def change + add_column :preview_cards_statuses, :url, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 37020c2d7..a0062c8ce 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_09_07_150100) do +ActiveRecord::Schema[7.0].define(version: 2023_10_06_183200) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -811,6 +811,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_09_07_150100) do 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.string "url" end create_table "relays", force: :cascade do |t| diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 7f8e72dd8..209a73efa 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -69,7 +69,7 @@ namespace :tests do exit(1) end - unless Status.find(12).preview_cards.pluck(:url) == ['https://joinmastodon.org/'] + unless PreviewCard.where(id: PreviewCardsStatus.where(status_id: 12).select(:preview_card_id)).pluck(:url) == ['https://joinmastodon.org/'] puts 'Preview cards not deduplicated as expected' exit(1) end diff --git a/spec/helpers/media_component_helper_spec.rb b/spec/helpers/media_component_helper_spec.rb index 71a9af6f3..149f6a83a 100644 --- a/spec/helpers/media_component_helper_spec.rb +++ b/spec/helpers/media_component_helper_spec.rb @@ -49,10 +49,12 @@ describe MediaComponentHelper do end describe 'render_card_component' do - let(:status) { Fabricate(:status, preview_cards: [Fabricate(:preview_card)]) } + let(:status) { Fabricate(:status) } let(:result) { helper.render_card_component(status) } before do + PreviewCardsStatus.create(status: status, preview_card: Fabricate(:preview_card)) + without_partial_double_verification do allow(helper).to receive(:current_account).and_return(status.account) end diff --git a/spec/services/fetch_link_card_service_spec.rb b/spec/services/fetch_link_card_service_spec.rb index f44cbb750..d8ca310b2 100644 --- a/spec/services/fetch_link_card_service_spec.rb +++ b/spec/services/fetch_link_card_service_spec.rb @@ -120,7 +120,7 @@ RSpec.describe FetchLinkCardService, type: :service do let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis') } it 'decodes the HTML' do - expect(status.preview_cards.first.title).to eq('SJISのページ') + expect(status.preview_card.title).to eq('SJISのページ') end end @@ -128,7 +128,7 @@ RSpec.describe FetchLinkCardService, type: :service do let(:status) { Fabricate(:status, text: 'Check out http://example.com/sjis_with_wrong_charset') } it 'decodes the HTML despite the wrong charset header' do - expect(status.preview_cards.first.title).to eq('SJISのページ') + expect(status.preview_card.title).to eq('SJISのページ') end end @@ -136,7 +136,7 @@ RSpec.describe FetchLinkCardService, type: :service do let(:status) { Fabricate(:status, text: 'Check out http://example.com/koi8-r') } it 'decodes the HTML' do - expect(status.preview_cards.first.title).to eq('Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.') + expect(status.preview_card.title).to eq('Московя начинаетъ только въ XVI ст. привлекать внимане иностранцевъ.') end end @@ -144,7 +144,7 @@ RSpec.describe FetchLinkCardService, type: :service do let(:status) { Fabricate(:status, text: 'Check out http://example.com/windows-1251') } it 'decodes the HTML' do - expect(status.preview_cards.first.title).to eq('сэмпл текст') + expect(status.preview_card.title).to eq('сэмпл текст') end end diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb index 9c53ebb2f..eb38230b0 100644 --- a/spec/services/update_status_service_spec.rb +++ b/spec/services/update_status_service_spec.rb @@ -23,11 +23,11 @@ RSpec.describe UpdateStatusService, type: :service do end context 'when text changes' do - let!(:status) { Fabricate(:status, text: 'Foo') } + let(:status) { Fabricate(:status, text: 'Foo') } let(:preview_card) { Fabricate(:preview_card) } before do - status.preview_cards << preview_card + PreviewCardsStatus.create(status: status, preview_card: preview_card) subject.call(status, status.account_id, text: 'Bar') end @@ -45,11 +45,11 @@ RSpec.describe UpdateStatusService, type: :service do end context 'when content warning changes' do - let!(:status) { Fabricate(:status, text: 'Foo', spoiler_text: '') } + let(:status) { Fabricate(:status, text: 'Foo', spoiler_text: '') } let(:preview_card) { Fabricate(:preview_card) } before do - status.preview_cards << preview_card + PreviewCardsStatus.create(status: status, preview_card: preview_card) subject.call(status, status.account_id, text: 'Foo', spoiler_text: 'Bar') end