diff options
| -rw-r--r-- | app/lib/activitypub/parser/media_attachment_parser.rb | 4 | ||||
| -rw-r--r-- | app/models/concerns/status_snapshot_concern.rb | 35 | ||||
| -rw-r--r-- | app/models/media_attachment.rb | 7 | ||||
| -rw-r--r-- | app/models/status.rb | 21 | ||||
| -rw-r--r-- | app/services/activitypub/process_status_update_service.rb | 22 | ||||
| -rw-r--r-- | app/services/update_status_service.rb | 28 | ||||
| -rw-r--r-- | spec/models/media_attachment_spec.rb | 8 | ||||
| -rw-r--r-- | spec/services/activitypub/process_status_update_service_spec.rb | 31 | ||||
| -rw-r--r-- | spec/services/update_status_service_spec.rb | 17 |
9 files changed, 121 insertions, 52 deletions
diff --git a/app/lib/activitypub/parser/media_attachment_parser.rb b/app/lib/activitypub/parser/media_attachment_parser.rb index 1798e58a4..30bea1f0e 100644 --- a/app/lib/activitypub/parser/media_attachment_parser.rb +++ b/app/lib/activitypub/parser/media_attachment_parser.rb @@ -27,7 +27,9 @@ class ActivityPub::Parser::MediaAttachmentParser end def description - @json['summary'].presence || @json['name'].presence + str = @json['summary'].presence || @json['name'].presence + str = str.strip[0...MediaAttachment::MAX_DESCRIPTION_LENGTH] if str.present? + str end def focus diff --git a/app/models/concerns/status_snapshot_concern.rb b/app/models/concerns/status_snapshot_concern.rb new file mode 100644 index 000000000..9741b9aeb --- /dev/null +++ b/app/models/concerns/status_snapshot_concern.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module StatusSnapshotConcern + extend ActiveSupport::Concern + + included do + has_many :edits, class_name: 'StatusEdit', inverse_of: :status, dependent: :destroy + end + + def edited? + edited_at.present? + end + + def build_snapshot(account_id: nil, at_time: nil, rate_limit: true) + # We don't use `edits#new` here to avoid it having saved when the + # status is saved, since we want to control that manually + + StatusEdit.new( + status_id: id, + text: text, + spoiler_text: spoiler_text, + sensitive: sensitive, + ordered_media_attachment_ids: ordered_media_attachment_ids&.dup || media_attachments.pluck(:id), + media_descriptions: ordered_media_attachments.map(&:description), + poll_options: preloadable_poll&.options&.dup, + account_id: account_id || self.account_id, + created_at: at_time || edited_at, + rate_limit: rate_limit + ) + end + + def snapshot!(**options) + build_snapshot(**options).save! + end +end diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index a3115637e..21c663e47 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -185,7 +185,7 @@ class MediaAttachment < ApplicationRecord remotable_attachment :thumbnail, IMAGE_LIMIT, suppress_errors: true, download_on_assign: false validates :account, presence: true - validates :description, length: { maximum: MAX_DESCRIPTION_LENGTH }, if: :local? + validates :description, length: { maximum: MAX_DESCRIPTION_LENGTH } validates :file, presence: true, if: :local? validates :thumbnail, absence: true, if: -> { local? && !audio_or_video? } @@ -258,7 +258,6 @@ class MediaAttachment < ApplicationRecord after_commit :enqueue_processing, on: :create after_commit :reset_parent_cache, on: :update - before_create :prepare_description, unless: :local? before_create :set_unknown_type before_create :set_processing @@ -306,10 +305,6 @@ class MediaAttachment < ApplicationRecord self.type = :unknown if file.blank? && !type_changed? end - def prepare_description - self.description = description.strip[0...MAX_DESCRIPTION_LENGTH] unless description.nil? - end - def set_type_and_extension self.type = begin if VIDEO_MIME_TYPES.include?(file_content_type) diff --git a/app/models/status.rb b/app/models/status.rb index 5b984543e..bf5a49b7f 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -35,6 +35,7 @@ class Status < ApplicationRecord include Paginable include Cacheable include StatusThreadingConcern + include StatusSnapshotConcern include RateLimitable rate_limit by: :account, family: :statuses @@ -59,8 +60,6 @@ class Status < ApplicationRecord belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true - has_many :edits, class_name: 'StatusEdit', inverse_of: :status, dependent: :destroy - has_many :favourites, inverse_of: :status, dependent: :destroy has_many :bookmarks, inverse_of: :status, dependent: :destroy has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy @@ -212,24 +211,6 @@ class Status < ApplicationRecord public_visibility? || unlisted_visibility? end - def snapshot!(account_id: nil, at_time: nil, rate_limit: true) - edits.create!( - text: text, - spoiler_text: spoiler_text, - sensitive: sensitive, - ordered_media_attachment_ids: ordered_media_attachment_ids || media_attachments.pluck(:id), - 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, - rate_limit: rate_limit - ) - end - - def edited? - edited_at.present? - end - alias sign? distributable? def with_media? diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index 47a788c30..6dc14d8c2 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -4,6 +4,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService include JsonLdHelper def call(status, json) + raise ArgumentError, 'Status has unsaved changes' if status.changed? + @json = json @status_parser = ActivityPub::Parser::StatusParser.new(@json) @uri = @status_parser.uri @@ -17,16 +19,19 @@ class ActivityPub::ProcessStatusUpdateService < BaseService last_edit_date = status.edited_at.presence || status.created_at + # Since we rely on tracking of previous changes, ensure clean slate + status.clear_changes_information + # Only allow processing one create/update per status at a time RedisLock.acquire(lock_options) do |lock| if lock.acquired? Status.transaction do - create_previous_edit! + record_previous_edit! update_media_attachments! update_poll! update_immediate_attributes! update_metadata! - create_edit! + create_edits! end queue_poll_notifications! @@ -216,19 +221,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService { redis: Redis.current, key: "create:#{@uri}", autorelease: 15.minutes.seconds } end - def create_previous_edit! - # We only need to create a previous edit when no previous edits exist, e.g. - # when the status has never been edited. For other cases, we always create - # an edit, so the step can be skipped - - return if @status.edits.any? - - @status.snapshot!(at_time: @status.created_at, rate_limit: false) + def record_previous_edit! + @previous_edit = @status.build_snapshot(at_time: @status.created_at, rate_limit: false) if @status.edits.empty? end - def create_edit! + def create_edits! return unless significant_changes? + @previous_edit&.save! @status.snapshot!(account_id: @account.id, rate_limit: false) end diff --git a/app/services/update_status_service.rb b/app/services/update_status_service.rb index 055e5968d..c4c934976 100644 --- a/app/services/update_status_service.rb +++ b/app/services/update_status_service.rb @@ -4,6 +4,8 @@ class UpdateStatusService < BaseService include Redisable include LanguagesHelper + class NoChangesSubmittedError < StandardError; end + # @param [Status] status # @param [Integer] account_id # @param [Hash] options @@ -17,6 +19,8 @@ class UpdateStatusService < BaseService @status = status @options = options @account_id = account_id + @media_attachments_changed = false + @poll_changed = false Status.transaction do create_previous_edit! @@ -32,18 +36,24 @@ class UpdateStatusService < BaseService broadcast_updates! @status + rescue NoChangesSubmittedError + # For calls that result in no changes, swallow the error + # but get back to the original state + + @status.reload end private def update_media_attachments! - previous_media_attachments = @status.media_attachments.to_a + previous_media_attachments = @status.ordered_media_attachments.to_a next_media_attachments = validate_media! added_media_attachments = next_media_attachments - previous_media_attachments MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id) @status.ordered_media_attachment_ids = (@options[:media_ids] || []).map(&:to_i) & next_media_attachments.map(&:id) + @media_attachments_changed = previous_media_attachments.map(&:id) != @status.ordered_media_attachment_ids @status.media_attachments.reload end @@ -69,20 +79,23 @@ class UpdateStatusService < BaseService # If for some reasons the options were changed, it invalidates all previous # votes, so we need to remove them - poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple + @poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple poll.options = @options[:poll][:options] poll.hide_totals = @options[:poll][:hide_totals] || false poll.multiple = @options[:poll][:multiple] || false poll.expires_in = @options[:poll][:expires_in] - poll.reset_votes! if poll_changed + poll.reset_votes! if @poll_changed poll.save! @status.poll_id = poll.id elsif previous_poll.present? previous_poll.destroy + @poll_changed = true @status.poll_id = nil end + + @poll_changed = true if @previous_expires_at != @status.preloadable_poll&.expires_at end def update_immediate_attributes! @@ -90,8 +103,11 @@ class UpdateStatusService < BaseService @status.spoiler_text = @options[:spoiler_text] || '' if @options.key?(:spoiler_text) @status.sensitive = @options[:sensitive] || @options[:spoiler_text].present? if @options.key?(:sensitive) || @options.key?(:spoiler_text) @status.language = valid_locale_cascade(@options[:language], @status.language, @status.account.user&.preferred_posting_language, I18n.default_locale) - @status.edited_at = Time.now.utc + # We raise here to rollback the entire transaction + raise NoChangesSubmittedError unless significant_changes? + + @status.edited_at = Time.now.utc @status.save! end @@ -137,4 +153,8 @@ class UpdateStatusService < BaseService def create_edit! @status.snapshot!(account_id: @account_id) end + + def significant_changes? + @status.changed? || @poll_changed || @media_attachments_changed + end end diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index 7360b23cf..cbd9a09c5 100644 --- a/spec/models/media_attachment_spec.rb +++ b/spec/models/media_attachment_spec.rb @@ -186,14 +186,6 @@ RSpec.describe MediaAttachment, type: :model do expect(media.valid?).to be false end - describe 'descriptions for remote attachments' do - it 'are cut off at 1500 characters' do - media = Fabricate(:media_attachment, description: 'foo' * 1000, remote_url: 'http://example.com/blah.jpg') - - expect(media.description.size).to be <= 1_500 - end - end - describe 'size limit validation' do it 'rejects video files that are too large' do stub_const 'MediaAttachment::IMAGE_LIMIT', 100.megabytes diff --git a/spec/services/activitypub/process_status_update_service_spec.rb b/spec/services/activitypub/process_status_update_service_spec.rb index 788c7c9d9..f87adcae1 100644 --- a/spec/services/activitypub/process_status_update_service_spec.rb +++ b/spec/services/activitypub/process_status_update_service_spec.rb @@ -46,6 +46,29 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do expect(status.reload.spoiler_text).to eq 'Show more' end + context 'with no changes at all' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Note', + content: 'Hello world', + } + end + + before do + subject.call(status, json) + end + + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.edited?).to be false + end + end + context 'with no changes and originally with no ordered_media_attachment_ids' do let(:payload) do { @@ -61,8 +84,12 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do subject.call(status, json) end - it 'does not record an update' do - expect(status.reload.edited?).to be false + it 'does not create any edits' do + expect(status.reload.edits).to be_empty + end + + it 'does not mark status as edited' do + expect(status.edited?).to be false end end diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb index 78cc89cd4..71a73be5b 100644 --- a/spec/services/update_status_service_spec.rb +++ b/spec/services/update_status_service_spec.rb @@ -3,6 +3,23 @@ require 'rails_helper' RSpec.describe UpdateStatusService, type: :service do subject { described_class.new } + context 'when nothing changes' do + let!(:status) { Fabricate(:status, text: 'Foo', language: 'en') } + + before do + allow(ActivityPub::DistributionWorker).to receive(:perform_async) + subject.call(status, status.account_id, text: 'Foo') + end + + it 'does not create an edit' do + expect(status.reload.edits).to be_empty + end + + it 'does not notify anyone' do + expect(ActivityPub::DistributionWorker).to_not have_received(:perform_async) + end + end + context 'when text changes' do let!(:status) { Fabricate(:status, text: 'Foo') } let(:preview_card) { Fabricate(:preview_card) } |