Improve uniquness checks on import#2983
Open
monorkin wants to merge 6 commits into
Open
Conversation
This solves a possible uniquness validation skip as the duplicate detector would be empty on restart.
The ZIP file validating application logic was odd, I moved the application-specificc stuff to the record set and let the zip file validate that there aren't any FS collisions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens import collision detection during the check phase so invalid imports fail early, and fixes cursor collisions between record sets that share the same model (e.g., ActiveStorage blobs vs files). It also hardens ZIP handling by rejecting archives with duplicate entry names and makes ZIP globbing behave more like filesystem matching.
Changes:
- Introduces a
cursor_keyfor record sets and uses it in import job cursors to avoid collisions between distinct record sets sharing a model. - Adds uniqueness validation during import checks via a per-record-set duplicate detector (based on unique DB indexes) and adds additional targeted collision checks (e.g., board publication key, user email normalization).
- Rejects ZIPs with duplicate manifest entry names and adjusts glob semantics to avoid matching nested paths unless explicitly requested.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/models/zip_file_test.rb | Adds coverage for rejecting ZIPs with duplicate entry names and for pathname-aware glob behavior. |
| test/models/account/data_transfer/record_set_test.rb | Adds tests for unique-index collision detection within imports and for case-insensitive file collisions. |
| test/models/account/data_transfer/manifest_test.rb | Adds tests for cursor-key-based resumption and disambiguation of record sets sharing a model. |
| app/models/zip_file/reader.rb | Enforces unique ZIP entry names and makes glob matching pathname-aware. |
| app/models/account/import.rb | Changes check-step iteration to ignore per-record progress within a record set (to support whole-set uniqueness checks). |
| app/models/account/data_transfer/user_record_set.rb | Adds ID-collision detection and email normalization-based uniqueness checking. |
| app/models/account/data_transfer/record_set/duplicate_detector.rb | Adds a digest-based duplicate detector for unique key sets. |
| app/models/account/data_transfer/record_set.rb | Adds cursor keys, filename ambiguity detection, and in-import uniqueness detection based on unique indexes. |
| app/models/account/data_transfer/manifest.rb | Switches cursor logic from model name to cursor_key and adds explicit failure for unknown cursors. |
| app/models/account/data_transfer/board/publication_record_set.rb | Adds a specialized record set to detect board publication key collisions. |
| app/models/account/data_transfer/active_storage/file_record_set.rb | Updates file enumeration to match nested storage paths. |
| app/models/account/data_transfer/action_text/rich_text_record_set.rb | Adds ID-collision and uniqueness checks during import validation. |
| app/jobs/account/data_import_job.rb | Stores/uses cursor keys (not model names) for check/process step resumption. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+39
| duplicates = @reader.map(&:filename).tally.filter_map { |name, count| name if count > 1 } | ||
|
|
||
| if duplicates.any? | ||
| raise ZipFile::InvalidFileError, "Duplicate entries in zip: #{duplicates.join(', ')}" | ||
| end |
Comment on lines
+32
to
+33
| # ZIP files can technically contian duplicate entries in their manifest. | ||
| # But that doesn't produce a valid filesystem, so we flat-out reject them here. |
Comment on lines
+94
to
+98
| duplicates = file_list.map(&:downcase).tally.filter_map { |name, count| name if count > 1 } | ||
|
|
||
| if duplicates.any? | ||
| raise IntegrityError, "#{model} has multiple files for the same record: #{duplicates.join(', ')}" | ||
| end |
Comment on lines
+160
to
+162
| if columns = duplicate_detector.detect(data) | ||
| raise IntegrityError, "#{model} has multiple records with the same #{columns.to_sentence}: #{data.values_at(*columns).join(', ')}" | ||
| end |
Comment on lines
+154
to
+157
| # Remap the data being imported to the account we are importing it to. | ||
| # This ensures that uniquness checks are valid, else someone could change | ||
| # the account_id in the export to point to different accounts and circumvent | ||
| # this check. |
Comment on lines
+17
to
+21
| # NOTE: The digest is here to avoid storing whole row values in memory, which could be quite large. | ||
| # (remeber, the value is user controlled, someone could craft a malicious import to baloon memory) | ||
| # This is an improvement, but it still could be a problem if the number of unique values is very large. | ||
| # In that case, we may need to use a more memory-efficient approach, such as storing the digests in a | ||
| # temporary file, or we could switch to a Bloom filter, or a Merkel tree. |
Comment on lines
+27
to
+28
| # The last_id is ignored so that we can check for record uniquness when interrupted | ||
| record_set.check(from: zip, callback: callback) |
jeremy
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This improves our collision detection logic to fail early so any invalid imports get discarded instead of retried.
Few things changed:
This fixes a collision between blobs and files that went unnoticed up until now
There was a trade-off here; because this relies on a new duplicate detector object, when a job gets interrupted the whole record set has to re-run its checks instead of continuing from the last checked record, this re-fills the duplicate detector.
Just a note about the duplicate detector, I intentionally over-engineered it a bit to prevent specially crafted ZIP files from filling the memory up. This doesn't work with extremely large imports though. I think this is good enough for now, but if we run into problems we can change it to use a Bloom filter, or store hashes in a sorted tempfile.
I spiked out these implementations with Claude quickly, and it struggles to produce something nice so I shelved it for now and fixed the easy exploit vector of just creating a ZIP with many index entries but no data in it.