From ad1b10368d7599def9399813a74ad314e5be11fc Mon Sep 17 00:00:00 2001 From: Daniel Pinto <642149+Mouxy@users.noreply.github.com> Date: Wed, 18 Mar 2026 15:41:22 +0000 Subject: [PATCH] Fix export size doubling from re-exporting prior export blobs (#2707) * Exclude export/import blobs and attachments from account exports Export and import file blobs were being assigned the account's account_id (via the Current.account default), causing subsequent exports to include previous export/import ZIP files in the data. This roughly doubled the export size each time and made imports fail with an IntegrityError on unrecognized "Account::Export" record type. Filter out blobs attached to Export/Import records in BlobRecordSet, FileRecordSet, and a new AttachmentRecordSet. Co-Authored-By: Claude Opus 4.6 * Use STI base type for export blob exclusion filter Rails polymorphic_name returns base_class.name for STI models, so ActiveStorage stores record_type as "Export" rather than the concrete subclass names. Filter on the base type accordingly. Co-Authored-By: Claude Opus 4.6 * Fix export filtering for mixed-use Active Storage blobs Exclude only internal-only blobs for account export metadata/file sets, scope attachment subqueries by account, and add regression coverage for blobs shared between user records and export attachments. * Simplify export blob filtering Exports always create fresh blobs, so a blob can never be legitimately shared between an Export/Import and a real account record. Replace the internal_only/external split with a single excluded_blob_ids subquery and drop the contrived shared-blob test. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .../active_storage/attachment_record_set.rb | 11 +++++++ .../active_storage/blob_record_set.rb | 8 +++++ .../active_storage/file_record_set.rb | 6 +++- app/models/account/data_transfer/manifest.rb | 2 +- .../account/data_transfer/record_set.rb | 1 + test/models/account/export_test.rb | 31 +++++++++++++++++++ 6 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 app/models/account/data_transfer/active_storage/attachment_record_set.rb diff --git a/app/models/account/data_transfer/active_storage/attachment_record_set.rb b/app/models/account/data_transfer/active_storage/attachment_record_set.rb new file mode 100644 index 000000000..0c87c10af --- /dev/null +++ b/app/models/account/data_transfer/active_storage/attachment_record_set.rb @@ -0,0 +1,11 @@ +class Account::DataTransfer::ActiveStorage::AttachmentRecordSet < Account::DataTransfer::RecordSet + def initialize(account) + super(account: account, model: ::ActiveStorage::Attachment) + end + + private + def records + ::ActiveStorage::Attachment.where(account: account) + .where.not(record_type: INTERNAL_RECORD_TYPES) + end +end diff --git a/app/models/account/data_transfer/active_storage/blob_record_set.rb b/app/models/account/data_transfer/active_storage/blob_record_set.rb index 0c4a553c1..7adc5742e 100644 --- a/app/models/account/data_transfer/active_storage/blob_record_set.rb +++ b/app/models/account/data_transfer/active_storage/blob_record_set.rb @@ -8,6 +8,14 @@ class Account::DataTransfer::ActiveStorage::BlobRecordSet < Account::DataTransfe end private + def records + ::ActiveStorage::Blob.where(account: account).where.not(id: excluded_blob_ids) + end + + def excluded_blob_ids + ::ActiveStorage::Attachment.where(account: account, record_type: INTERNAL_RECORD_TYPES).select(:blob_id) + end + def import_batch(files) batch_data = files.map do |file| data = load(file) diff --git a/app/models/account/data_transfer/active_storage/file_record_set.rb b/app/models/account/data_transfer/active_storage/file_record_set.rb index 421a2e520..f64f53d3a 100644 --- a/app/models/account/data_transfer/active_storage/file_record_set.rb +++ b/app/models/account/data_transfer/active_storage/file_record_set.rb @@ -5,7 +5,11 @@ class Account::DataTransfer::ActiveStorage::FileRecordSet < Account::DataTransfe private def records - ::ActiveStorage::Blob.where(account: account) + ::ActiveStorage::Blob.where(account: account).where.not(id: excluded_blob_ids) + end + + def excluded_blob_ids + ::ActiveStorage::Attachment.where(account: account, record_type: INTERNAL_RECORD_TYPES).select(:blob_id) end def export_record(blob) diff --git a/app/models/account/data_transfer/manifest.rb b/app/models/account/data_transfer/manifest.rb index 9de76f9bf..f7a7791e1 100644 --- a/app/models/account/data_transfer/manifest.rb +++ b/app/models/account/data_transfer/manifest.rb @@ -58,7 +58,7 @@ class Account::DataTransfer::Manifest ::Webhook::Delivery ), Account::DataTransfer::ActiveStorage::BlobRecordSet.new(account), - *build_record_sets(::ActiveStorage::Attachment), + Account::DataTransfer::ActiveStorage::AttachmentRecordSet.new(account), Account::DataTransfer::ActionText::RichTextRecordSet.new(account), Account::DataTransfer::ActiveStorage::FileRecordSet.new(account) ].then { set_importable_model_names(it) } diff --git a/app/models/account/data_transfer/record_set.rb b/app/models/account/data_transfer/record_set.rb index 2fba9402d..ff7b79b5c 100644 --- a/app/models/account/data_transfer/record_set.rb +++ b/app/models/account/data_transfer/record_set.rb @@ -3,6 +3,7 @@ class Account::DataTransfer::RecordSet class ConflictError < IntegrityError; end IMPORT_BATCH_SIZE = 100 + INTERNAL_RECORD_TYPES = %w[Export Account::Import].freeze attr_accessor :importable_model_names attr_reader :account, :model, :attributes diff --git a/test/models/account/export_test.rb b/test/models/account/export_test.rb index d449764b8..4c7d261b9 100644 --- a/test/models/account/export_test.rb +++ b/test/models/account/export_test.rb @@ -60,6 +60,37 @@ class Account::ExportTest < ActiveSupport::TestCase end end + test "export excludes blobs and attachments from previous exports" do + first_export = Account::Export.create!(account: Current.account, user: users(:david)) + first_export.build + assert first_export.completed? + + first_export_blob = first_export.file.blob + first_export_attachment = first_export.file.attachment + + second_export = Account::Export.create!(account: Current.account, user: users(:david)) + second_export.build + assert second_export.completed? + + second_export.file.open do |file| + reader = ZipKit::FileReader.read_zip_structure(io: file) + filenames = reader.map(&:filename) + + blob_entries = filenames.select { |f| f.start_with?("data/active_storage_blobs/") } + blob_ids = blob_entries.map { |f| File.basename(f, ".json") } + assert_not_includes blob_ids, first_export_blob.id, "Export should not include blob metadata from previous exports" + + storage_entries = filenames.select { |f| f.start_with?("storage/") } + assert_not storage_entries.any? { |f| f == "storage/#{first_export_blob.key}" }, + "Export should not include blob file from previous exports" + + attachment_entries = filenames.select { |f| f.start_with?("data/active_storage_attachments/") } + attachment_ids = attachment_entries.map { |f| File.basename(f, ".json") } + assert_not_includes attachment_ids, first_export_attachment.id, + "Export should not include attachment records from previous exports" + end + end + test "build succeeds when rich text references missing blob" do blob = ActiveStorage::Blob.create_and_upload!( io: file_fixture("moon.jpg").open,