From 1f73b81bd82fdd5fea177071e56debb07557c2dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?KMY=EF=BC=88=E9=9B=AA=E3=81=82=E3=81=99=E3=81=8B=EF=BC=89?= Date: Fri, 15 Dec 2023 08:40:59 +0900 Subject: [PATCH] =?UTF-8?q?Fix:=20=E4=B8=80=E9=83=A8=E3=82=B5=E3=83=BC?= =?UTF-8?q?=E3=83=90=E3=83=BC=E3=81=AE=E6=8A=95=E7=A8=BF=E3=81=8C=E5=8F=82?= =?UTF-8?q?=E7=85=A7=E5=BC=95=E7=94=A8=E3=81=A7=E3=81=8D=E3=81=AA=E3=81=84?= =?UTF-8?q?=E5=95=8F=E9=A1=8C=E3=83=BB=E5=8F=82=E7=85=A7=E5=BC=95=E7=94=A8?= =?UTF-8?q?=E6=99=82=E3=80=81=E3=83=95=E3=82=A7=E3=83=83=E3=83=81=E3=82=92?= =?UTF-8?q?=E5=9F=BA=E6=9C=AC=E3=81=97=E3=81=AA=E3=81=84=E3=82=88=E3=81=86?= =?UTF-8?q?=E5=A4=89=E6=9B=B4=20(#360)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/lib/activitypub/activity/create.rb | 2 +- app/lib/activitypub/tag_manager.rb | 6 ++- app/services/process_references_service.rb | 13 ++++- app/services/resolve_url_service.rb | 9 ++-- .../20231214225249_index_to_statuses_url.rb | 13 +++++ db/schema.rb | 3 +- .../process_references_service_spec.rb | 53 ++++++++++++++++--- 7 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 db/migrate/20231214225249_index_to_statuses_url.rb diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index bf1ddb8ce1..00d0f7574c 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -544,7 +544,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity url = quote if url.present? - ResolveURLService.new.call(url, on_behalf_of: @account, local_only: true).present? + ActivityPub::TagManager.instance.uri_to_resource(url, Status)&.local? else false end diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index 33829e8894..eef0f6a209 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -208,7 +208,7 @@ class ActivityPub::TagManager uri_to_resource(uri, Account) end - def uri_to_resource(uri, klass) + def uri_to_resource(uri, klass, url: false) return if uri.nil? if local_uri?(uri) @@ -221,7 +221,9 @@ class ActivityPub::TagManager elsif OStatus::TagManager.instance.local_id?(uri) klass.find_by(id: OStatus::TagManager.instance.unique_tag_to_local_id(uri, klass.to_s)) else - klass.find_by(uri: uri.split('#').first) + resource = klass.find_by(uri: uri.split('#').first) + resource ||= klass.where('uri != url').find_by(url: uri.split('#').first) if url + resource end rescue ActiveRecord::RecordNotFound nil diff --git a/app/services/process_references_service.rb b/app/services/process_references_service.rb index f19130f5a7..910a8c086e 100644 --- a/app/services/process_references_service.rb +++ b/app/services/process_references_service.rb @@ -141,11 +141,20 @@ class ProcessReferencesService < BaseService end def url_to_status(url) - ResolveURLService.new.call(url, on_behalf_of: @status.account, fetch_remote: @fetch_remote && @no_fetch_urls.exclude?(url)) + status = ActivityPub::TagManager.instance.uri_to_resource(url, Status, url: true) + status ||= ResolveURLService.new.call(url, on_behalf_of: @status.account) if @fetch_remote && @no_fetch_urls.exclude?(url) + referrable?(status) ? status : nil + end + + def referrable?(target_status) + return false if target_status.nil? + return @referrable if defined?(@referrable) + + @referrable = StatusPolicy.new(@status.account, target_status).show? end def quotable?(target_status) - target_status.account.allow_quote? && (!@status.local? || StatusPolicy.new(@status.account, target_status).quote?) + target_status.account.allow_quote? && StatusPolicy.new(@status.account, target_status).quote? end def add_references diff --git a/app/services/resolve_url_service.rb b/app/services/resolve_url_service.rb index 1194afc368..19a94e77ad 100644 --- a/app/services/resolve_url_service.rb +++ b/app/services/resolve_url_service.rb @@ -6,16 +6,15 @@ class ResolveURLService < BaseService USERNAME_STATUS_RE = %r{/@(?#{Account::USERNAME_RE})/(?[0-9]+)\Z} - def call(url, on_behalf_of: nil, fetch_remote: true, local_only: false) + def call(url, on_behalf_of: nil) @url = url @on_behalf_of = on_behalf_of - @fetch_remote = fetch_remote if local_url? process_local_url - elsif !local_only && fetch_remote && !fetched_resource.nil? + elsif !fetched_resource.nil? process_url - elsif !local_only + else process_url_from_db end end @@ -38,7 +37,7 @@ class ResolveURLService < BaseService return account unless account.nil? end - return unless @on_behalf_of.present? && (!@fetch_remote || [401, 403, 404].include?(fetch_resource_service.response_code)) + return unless @on_behalf_of.present? && [401, 403, 404].include?(fetch_resource_service.response_code) # It may happen that the resource is a private toot, and thus not fetchable, # but we can return the toot if we already know about it. diff --git a/db/migrate/20231214225249_index_to_statuses_url.rb b/db/migrate/20231214225249_index_to_statuses_url.rb new file mode 100644 index 0000000000..ab3eff41ab --- /dev/null +++ b/db/migrate/20231214225249_index_to_statuses_url.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class IndexToStatusesURL < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def up + add_index :statuses, :url, name: :index_statuses_on_url, algorithm: :concurrently, opclass: :text_pattern_ops, where: 'url IS NOT NULL AND url <> uri' + end + + def down + remove_index :statuses, name: :index_statuses_on_url + end +end diff --git a/db/schema.rb b/db/schema.rb index 05c9ffd07d..2f7544372f 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.1].define(version: 2023_12_12_225737) do +ActiveRecord::Schema[7.1].define(version: 2023_12_14_225249) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1247,6 +1247,7 @@ ActiveRecord::Schema[7.1].define(version: 2023_12_12_225737) do t.index ["quote_of_id", "account_id"], name: "index_statuses_on_quote_of_id_and_account_id" t.index ["reblog_of_id", "account_id"], name: "index_statuses_on_reblog_of_id_and_account_id" t.index ["uri"], name: "index_statuses_on_uri", unique: true, opclass: :text_pattern_ops, where: "(uri IS NOT NULL)" + t.index ["url"], name: "index_statuses_on_url", opclass: :text_pattern_ops, where: "((url IS NOT NULL) AND ((url)::text <> (uri)::text))" end create_table "statuses_tags", primary_key: ["tag_id", "status_id"], force: :cascade do |t| diff --git a/spec/services/process_references_service_spec.rb b/spec/services/process_references_service_spec.rb index dedd47244d..3f4004e9d7 100644 --- a/spec/services/process_references_service_spec.rb +++ b/spec/services/process_references_service_spec.rb @@ -258,18 +258,57 @@ RSpec.describe ProcessReferencesService, type: :service do end let(:text) { 'BT:https://example.com/test_post' } + shared_examples 'reference once' do |uri, url| + it 'reference it' do + expect(subject.size).to eq 1 + expect(subject[0][1]).to eq 'BT' + + status = Status.find_by(id: subject[0][0]) + expect(status).to_not be_nil + expect(status.id).to eq remote_status.id + expect(status.uri).to eq uri + expect(status.url).to eq url + end + end + before do stub_request(:get, 'https://example.com/test_post').to_return(status: 200, body: Oj.dump(object_json), headers: { 'Content-Type' => 'application/activity+json' }) end - it 'reference it' do - expect(subject.size).to eq 1 - expect(subject[0][1]).to eq 'BT' + it_behaves_like 'reference once', 'https://example.com/test_post', 'https://example.com/test_post' - status = Status.find_by(id: subject[0][0]) - expect(status).to_not be_nil - expect(status.id).to eq remote_status.id - expect(status.url).to eq 'https://example.com/test_post' + context 'when uri and url is difference and url is not accessable' do + let(:remote_status) { Fabricate(:status, account: account, uri: 'https://example.com/test_post', url: 'https://example.com/test_post_ohagi', text: 'Lorem ipsum') } + let(:text) { 'BT:https://example.com/test_post_ohagi' } + let(:object_json) do + { + id: 'https://example.com/test_post', + url: 'https://example.com/test_post_ohagi', + to: ActivityPub::TagManager::COLLECTIONS[:public], + '@context': ActivityPub::TagManager::CONTEXT, + type: 'Note', + actor: account.uri, + attributedTo: account.uri, + content: 'Lorem ipsum', + } + end + + before do + stub_request(:get, 'https://example.com/test_post_ohagi').to_return(status: 404) + end + + it_behaves_like 'reference once', 'https://example.com/test_post', 'https://example.com/test_post_ohagi' + + it 'do not request to uri' do + subject + expect(a_request(:get, 'https://example.com/test_post_ohagi')).to_not have_been_made + end + + context 'when url and uri is specified at the same time' do + let(:text) { 'BT:https://example.com/test_post_ohagi BT:https://example.com/test_post' } + + it_behaves_like 'reference once', 'https://example.com/test_post', 'https://example.com/test_post_ohagi' + end end end end