fix(website): Fix applying file mappings for compressed metadata files - #7119
fix(website): Fix applying file mappings for compressed metadata files#7119tombch wants to merge 10 commits into
Conversation
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
@claude review (please) |
|
Claude finished @tombch's task in 2m 59s —— View job Code reviewThe fix is correct: with files enabled and no column mapping, the factory previously handed the still-compressed Four points, all inline:
Nothing security-relevant here; no new input reaches the backend unvalidated, and CI on this branch is currently mostly
No code changes were made — review only. |
| } else if (extraFilesEnabled) { | ||
| // Ensure the metadata file is decompressed so that any file mappings | ||
| // can be applied correctly | ||
| mFile = new File([await metadataFile.text()], 'metadata.tsv'); | ||
| } else { | ||
| mFile = metadataFile.inner(); | ||
| } |
There was a problem hiding this comment.
This branch fires for every bulk upload when files are enabled, not just compressed ones, so it has two side effects beyond the bug being fixed:
- An already-plain
.tsv(or anExcelFile, whoseinner()is already a converted TSV) gets fully buffered into a JS string and copied into a newFilefor no reason. - If the user uploaded a compressed metadata file but no file mappings end up resolved,
applyFileMappingsreturns the file unchanged (fileMapping.ts:474), so we now upload the metadata decompressed to the backend where it used to go over the wire gzipped.
CompressedFile is the only ProcessedFile whose inner() differs from text(), so gating on it keeps the fix and avoids both:
| } else if (extraFilesEnabled) { | |
| // Ensure the metadata file is decompressed so that any file mappings | |
| // can be applied correctly | |
| mFile = new File([await metadataFile.text()], 'metadata.tsv'); | |
| } else { | |
| mFile = metadataFile.inner(); | |
| } | |
| } else if (extraFilesEnabled && metadataFile instanceof CompressedFile) { | |
| // Ensure the metadata file is decompressed so that any file mappings | |
| // can be applied correctly | |
| mFile = new File([await metadataFile.text()], 'metadata.tsv'); | |
| } else { | |
| mFile = metadataFile.inner(); | |
| } |
(needs CompressedFile added to the import from ./FileUpload/fileProcessing).
| } else if (extraFilesEnabled) { | ||
| // Ensure the metadata file is decompressed so that any file mappings | ||
| // can be applied correctly | ||
| mFile = new File([await metadataFile.text()], 'metadata.tsv'); |
There was a problem hiding this comment.
Note that the same file is decompressed twice per submission: the effect above (line 90-92) already calls metadataFile.text() to build the submission file mapping, and this runs it again at submit time. gzip/zstd decompression here is synchronous (fflate.decompressSync / fzstd.decompress in fileProcessing.ts), so for a large metadata file that's two main-thread stalls. Caching the decompressed text next to the mapping state (or threading the ProcessedFile through to DataUploadForm instead of materialising a File here, as your PR description suggests) would fix this along with the layering issue.
| METADATA_COMPRESSION_CASES.forEach(({ compressMetadata, description }) => { | ||
| test( | ||
| 'bulk submit blocks a submission with errors in file linkage or parsing' + description, | ||
| async ({ page, groupId, tmpDir }) => { |
There was a problem hiding this comment.
This test never reaches the code path the PR fixes: all four cases bail out in DataUploadForm.handleSubmit at the mapping/linkage error checks before applyFileMappings writes anything, and the mapping parse path already used metadataFile.text() (so it decompressed correctly before this fix too). The compressed variant here therefore passes both with and without the fix, while costing another ~3 min of CI on top of the extra run of the 240 s test above.
Consider parameterising only the success-path test at line 136 and leaving this one single-cased.
| const METADATA_COMPRESSION_CASES = [ | ||
| { compressMetadata: false, description: '' }, | ||
| { compressMetadata: true, description: ' with compressed metadata file' }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
The regression itself is entirely client-side (the file factory in FormOrUploadWrapper), so it can be pinned much more cheaply than by re-running a 240 s end-to-end submission. FormOrUploadWrapper.spec.tsx already has a MockSaveWrapper harness that calls the file factory directly; a bulk-mode case that uploads a gzipSynced TSV with files.enabled and asserts the returned metadataFile.text() contains the headers would fail before this fix and run in milliseconds. Worth adding regardless of whether you keep the Playwright parameterisation.
resolves #7120
Summary
applyFileMappingsto fail at writing to the file (as it cannot see the metadata headers) and the submission unable to continue.applyFileMappingsto later read correctly.Notes
FormOrUploadWrapper, and further processing occur in theDataUploadForm'shandleSubmitfunction, which silently depends on actions like making sure the file is decompressed in the files factory. So I'd be happy to expand on this if we don't think this is a sufficient fix for now.Screenshot
PR Checklist
🚀 Preview: Add
previewlabel to enable