Refactor `Status._insert_record` slightly and tighten the test around reblogs of discarded statuses (#24516)

This commit is contained in:
Claire 2023-04-12 12:47:05 +02:00 committed by GitHub
parent cee357d538
commit f53d009778
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 34 additions and 23 deletions

View File

@ -391,33 +391,41 @@ class Status < ApplicationRecord
super || build_status_stat super || build_status_stat
end end
# Hack to use a "INSERT INTO ... SELECT ..." query instead of "INSERT INTO ... VALUES ..." query # This is a hack to ensure that no reblogs of discarded statuses are created,
# as this cannot be enforced through database constraints the same way we do
# for reblogs of deleted statuses.
#
# To achieve this, we redefine the internal method responsible for issuing
# the "INSERT" statement and replace the "INSERT INTO ... VALUES ..." query
# with an "INSERT INTO ... SELECT ..." query with a "WHERE deleted_at IS NULL"
# clause on the reblogged status to ensure consistency at the database level.
#
# Otherwise, the code is kept as close as possible to ActiveRecord::Persistence
# code, and actually calls it if we are not handling a reblog.
def self._insert_record(values) def self._insert_record(values)
if values.is_a?(Hash) && values['reblog_of_id'].present? return super unless values.is_a?(Hash) && values['reblog_of_id'].present?
primary_key = self.primary_key
primary_key_value = nil
if primary_key primary_key = self.primary_key
primary_key_value = values[primary_key] primary_key_value = nil
if !primary_key_value && prefetch_primary_key? if primary_key
primary_key_value = next_sequence_value primary_key_value = values[primary_key]
values[primary_key] = primary_key_value
end if !primary_key_value && prefetch_primary_key?
primary_key_value = next_sequence_value
values[primary_key] = primary_key_value
end end
# The following line is where we differ from stock ActiveRecord implementation
im = _compile_reblog_insert(values)
# Since we are using SELECT instead of VALUES, a non-error `nil` return is possible.
# For our purposes, it's equivalent to a foreign key constraint violation
result = connection.insert(im, "#{self} Create", primary_key || false, primary_key_value)
raise ActiveRecord::InvalidForeignKey, "(reblog_of_id)=(#{values['reblog_of_id']}) is not present in table \"statuses\"" if result.nil?
result
else
super
end end
# The following line is where we differ from stock ActiveRecord implementation
im = _compile_reblog_insert(values)
# Since we are using SELECT instead of VALUES, a non-error `nil` return is possible.
# For our purposes, it's equivalent to a foreign key constraint violation
result = connection.insert(im, "#{self} Create", primary_key || false, primary_key_value)
raise ActiveRecord::InvalidForeignKey, "(reblog_of_id)=(#{values['reblog_of_id']}) is not present in table \"statuses\"" if result.nil?
result
end end
def self._compile_reblog_insert(values) def self._compile_reblog_insert(values)

View File

@ -38,7 +38,10 @@ RSpec.describe ReblogService, type: :service do
let(:status) { Fabricate(:status, account: alice, visibility: :public) } let(:status) { Fabricate(:status, account: alice, visibility: :public) }
before do before do
status.discard # Update the in-database attribute without reflecting the change in
# the object. This cannot simulate all race conditions, but it is
# pretty close.
Status.where(id: status.id).update_all(deleted_at: Time.now.utc) # rubocop:disable Rails/SkipsModelValidations
end end
it 'raises an exception' do it 'raises an exception' do