From d17fb7013116767fc5c7d5eef63218bd8c45b023 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 9 Mar 2022 09:06:17 +0100 Subject: Change how changes to media attachments are stored for edits (#17696) * Change how changes to media attachments are stored for edits Fix not being able to re-order media attachments * Fix not broadcasting updates when polls/media is changed through ActivityPub * Various fixes and improvements * Update app/models/report.rb Co-authored-by: Claire * Add tracking of media attachment description changes * Change poll in status edit to have a structure closer to the real one Co-authored-by: Claire --- .../activitypub/process_status_update_service.rb | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'app/services/activitypub/process_status_update_service.rb') diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 7438a7c53..11afa894f 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -76,13 +76,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end end - removed_media_attachments = previous_media_attachments - next_media_attachments - added_media_attachments = next_media_attachments - previous_media_attachments + added_media_attachments = next_media_attachments - previous_media_attachments - MediaAttachment.where(id: removed_media_attachments.map(&:id)).update_all(status_id: nil) MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id) - @media_attachments_changed = true if removed_media_attachments.any? || added_media_attachments.any? + @status.ordered_media_attachment_ids = next_media_attachments.map(&:id) + @status.media_attachments.reload + + @media_attachments_changed = true if @status.ordered_media_attachment_ids_changed? end def update_poll! @@ -215,19 +216,13 @@ class ActivityPub::ProcessStatusUpdateService < BaseService return if @status.edits.any? - @status.snapshot!( - media_attachments_changed: false, - at_time: @status.created_at - ) + @status.snapshot!(at_time: @status.created_at) end def create_edit! return unless significant_changes? - @status.snapshot!( - media_attachments_changed: @media_attachments_changed || @poll_changed, - account_id: @account.id - ) + @status.snapshot!(account_id: @account.id) end def skip_download? -- cgit From b2cd34474b58b8a1f5e01ba73d236551dd0a878f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 9 Mar 2022 20:06:51 +0100 Subject: Add rate limit for editing (#17728) --- app/controllers/api/v1/statuses_controller.rb | 1 + app/models/status.rb | 5 +++-- app/models/status_edit.rb | 4 ++++ app/services/activitypub/process_status_update_service.rb | 4 ++-- app/services/update_status_service.rb | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-) (limited to 'app/services/activitypub/process_status_update_service.rb') diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index f48aeb945..3fe137bfd 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -10,6 +10,7 @@ class Api::V1::StatusesController < Api::BaseController before_action :set_thread, only: [:create] override_rate_limit_headers :create, family: :statuses + override_rate_limit_headers :update, family: :statuses # This API was originally unlimited, pagination cannot be introduced without # breaking backwards-compatibility. Arbitrarily high number to cover most diff --git a/app/models/status.rb b/app/models/status.rb index db10eedc2..12daee2de 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -212,7 +212,7 @@ class Status < ApplicationRecord public_visibility? || unlisted_visibility? end - def snapshot!(account_id: nil, at_time: nil) + def snapshot!(account_id: nil, at_time: nil, rate_limit: true) edits.create!( text: text, spoiler_text: spoiler_text, @@ -221,7 +221,8 @@ class Status < ApplicationRecord media_descriptions: ordered_media_attachments.map(&:description), poll_options: preloadable_poll&.options, account_id: account_id || self.account_id, - created_at: at_time || edited_at + created_at: at_time || edited_at, + rate_limit: rate_limit ) end diff --git a/app/models/status_edit.rb b/app/models/status_edit.rb index 94a387c36..6da9b4b85 100644 --- a/app/models/status_edit.rb +++ b/app/models/status_edit.rb @@ -17,6 +17,8 @@ # class StatusEdit < ApplicationRecord + include RateLimitable + self.ignored_columns = %w( media_attachments_changed ) @@ -26,6 +28,8 @@ class StatusEdit < ApplicationRecord delegate :id, :type, :url, :preview_url, :remote_url, :preview_remote_url, :text_url, :meta, :blurhash, to: :media_attachment end + rate_limit by: :account, family: :statuses + belongs_to :status belongs_to :account, optional: true diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 11afa894f..1260c0482 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -216,13 +216,13 @@ class ActivityPub::ProcessStatusUpdateService < BaseService return if @status.edits.any? - @status.snapshot!(at_time: @status.created_at) + @status.snapshot!(at_time: @status.created_at, rate_limit: false) end def create_edit! return unless significant_changes? - @status.snapshot!(account_id: @account.id) + @status.snapshot!(account_id: @account.id, rate_limit: false) end def skip_download? diff --git a/app/services/update_status_service.rb b/app/services/update_status_service.rb index 1c63ab656..055e5968d 100644 --- a/app/services/update_status_service.rb +++ b/app/services/update_status_service.rb @@ -131,7 +131,7 @@ class UpdateStatusService < BaseService return if @status.edits.any? - @status.snapshot!(at_time: @status.created_at) + @status.snapshot!(at_time: @status.created_at, rate_limit: false) end def create_edit! -- cgit From d3aa9cf7744eede43c5c1b9097eb1f0720185ccd Mon Sep 17 00:00:00 2001 From: Claire Date: Sat, 12 Mar 2022 09:30:31 +0100 Subject: Fix Updates being forwarded even when not processable or causing no change (#17699) * Fix Updates being forwarded even when not processable or causing no change * Refactor and ensure status edit is strictly newer than last known edit --- app/services/activitypub/process_status_update_service.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'app/services/activitypub/process_status_update_service.rb') diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 1260c0482..70e9cb7d9 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -15,6 +15,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService # Only native types can be updated at the moment return if !expected_type? || already_updated_more_recently? + last_edit_date = status.edited_at.presence || status.created_at + # Only allow processing one create/update per status at a time RedisLock.acquire(lock_options) do |lock| if lock.acquired? @@ -37,6 +39,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService raise Mastodon::RaceConditionError end end + + forward_activity! if significant_changes? && @status_parser.edited_at.present? && @status_parser.edited_at > last_edit_date end private @@ -263,4 +267,12 @@ class ActivityPub::ProcessStatusUpdateService < BaseService PollExpirationNotifyWorker.remove_from_scheduled(poll.id) if @previous_expires_at.present? && @previous_expires_at > poll.expires_at PollExpirationNotifyWorker.perform_at(poll.expires_at + 5.minutes, poll.id) end + + def forward_activity! + forwarder.forward! if forwarder.forwardable? + end + + def forwarder + @forwarder ||= ActivityPub::Forwarder.new(@account, @json, @status) + end end -- cgit From 92a86b958ee7b96dbcbc17237dfcb1af0f15740c Mon Sep 17 00:00:00 2001 From: Claire Date: Sat, 12 Mar 2022 19:33:10 +0100 Subject: Fix issues with processing toot edits (#17748) * Fix searching for an already-known status by URL not working * Fix Update processing from statuses prior to 20220302232632 `ordered_media_attachment_ids_changed?` would return `true` when going from `nil` to anything (including `[]`). * Add tests --- .../activitypub/process_status_update_service.rb | 11 +++++++---- .../process_status_update_service_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) (limited to 'app/services/activitypub/process_status_update_service.rb') diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 70e9cb7d9..47a788c30 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -13,7 +13,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @poll_changed = false # Only native types can be updated at the moment - return if !expected_type? || already_updated_more_recently? + return @status if !expected_type? || already_updated_more_recently? last_edit_date = status.edited_at.presence || status.created_at @@ -41,13 +41,16 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end forward_activity! if significant_changes? && @status_parser.edited_at.present? && @status_parser.edited_at > last_edit_date + + @status end private def update_media_attachments! - previous_media_attachments = @status.media_attachments.to_a - next_media_attachments = [] + previous_media_attachments = @status.media_attachments.to_a + previous_media_attachments_ids = @status.ordered_media_attachment_ids || previous_media_attachments.map(&:id) + next_media_attachments = [] as_array(@json['attachment']).each do |attachment| media_attachment_parser = ActivityPub::Parser::MediaAttachmentParser.new(attachment) @@ -87,7 +90,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @status.ordered_media_attachment_ids = next_media_attachments.map(&:id) @status.media_attachments.reload - @media_attachments_changed = true if @status.ordered_media_attachment_ids_changed? + @media_attachments_changed = true if @status.ordered_media_attachment_ids != previous_media_attachments_ids end def update_poll! diff --git a/spec/services/activitypub/process_status_update_service_spec.rb b/spec/services/activitypub/process_status_update_service_spec.rb index 40b405217..788c7c9d9 100644 --- a/spec/services/activitypub/process_status_update_service_spec.rb +++ b/spec/services/activitypub/process_status_update_service_spec.rb @@ -46,6 +46,26 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do expect(status.reload.spoiler_text).to eq 'Show more' end + context 'with no changes and originally with no ordered_media_attachment_ids' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Note', + content: 'Hello world', + } + end + + before do + status.update(ordered_media_attachment_ids: nil) + subject.call(status, json) + end + + it 'does not record an update' do + expect(status.reload.edited?).to be false + end + end + context 'originally without tags' do before do subject.call(status, json) -- cgit