Skip to content

feat!(website, backend): Update bulk file submission/revision format to use metadata file - #6922

Open
tombch wants to merge 42 commits into
mainfrom
bulk-file-submissions
Open

feat!(website, backend): Update bulk file submission/revision format to use metadata file#6922
tombch wants to merge 42 commits into
mainfrom
bulk-file-submissions

Conversation

@tombch

@tombch tombch commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

resolves #6883 and #5820

PR description and docs still need updating with further details

Summary

Restructures how files are submitted in bulk submissions/revisions, as well as how file mappings are provided from the website to the backend for submissions/revisions - previously, bulk and form website submission/revisions attached a separate file mapping object. Now, file mappings for each submission are written to the metadata file by the website before sending for submission/revision.

New files.<category> columns are introduced within the metadata, and uploaded files must be linked to within here - this can be done simply by adding a space separated list of: the file names (name1 name2 name3, if uploading a flat folder of files) or the file paths ( name1::path1 name2::path2 name3::path3, if uploading a more complicated folder structure, to allow for different submissions to use the same file name within a bulk upload).

Screenshot

  1. Subfolders can be submitted (paths show in download info) using fileName::filePath in metadata file:
image
  1. Files referenced in metadata but not uploaded are added in a warning and files that are not referenced in the metadata but are uploaded are also warned on
image

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://bulk-file-submissions.loculus.org

@claude claude Bot added website Tasks related to the web application backend related to the loculus backend component labels Jul 17, 2026
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #6883, #5820

@tombch tombch changed the title feat!(website): Updated bulk file submission/revision format to use metadata file feat!(website): Update bulk file submission/revision format to use metadata file Jul 17, 2026
@tombch tombch changed the title feat!(website): Update bulk file submission/revision format to use metadata file feat!(website, backend): Update bulk file submission/revision format to use metadata file Jul 17, 2026
@tombch tombch added the preview Triggers a deployment to argocd label Jul 27, 2026
Comment thread integration-tests/tests/specs/features/file-sharing.spec.ts Outdated
headers.push(sourceHeader);
indices.push(sourceIndex);
}
});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to note - the files category columns currently appear as unmapped in the column mapper - they should probably be recognised as input fields similar to fastaIds field

@tombch
tombch marked this pull request as ready for review July 28, 2026 15:31
@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 5m 41s —— View job


Claude finished reviewing

Summary

This is a well-structured refactor that moves bulk file linkage from a separate JSON mapping into files.<category> columns embedded in the metadata TSV. The new fileMapping.ts linkage model (linked/reused/missing/orphaned/shadowed) is a nice improvement over the previous folder-only matching, and it's backed by strong test coverage on both sides (fileMapping.spec.ts, MetadataEntryTest.kt, and the expanded file-sharing.spec.ts integration tests covering reuse/replace/discard/add flows and blocking-error scenarios).

Backend parsing (extractAndValidateFileIdAndName in MetadataEntry.kt) correctly uses lastIndexOf for the name:id split — safe since : is already a forbidden filename character (FileMappingPreconditionValidator.validateFilename), so there's no ambiguity even for filenames that happen to contain the separator character before rejection. The old-JSON-mapping backward-compat path (submissionParams.files merged via mergeFileCategories, metadata columns taking priority) and the file-group-ownership re-validation against the merged data in SubmitModel.kt also look correct.

Two minor issues left as inline comments:

  1. GetSubmittedDataHelpers.kt:28 — hardcodes the : separator instead of reusing the already-defined FILE_NAME_ID_SEPARATOR constant, which could drift from the parse path over time.
  2. DataUploadForm.tsx:147 — recomputes getFileLinkage(...) and shadows the fileLinkage already produced by the useMemo above, which covers the same condition. Minor redundancy/readability nit, not a correctness bug.

I also independently confirmed the concern raised in your own review comment on ColumnMapping.ts:115: applyTo force-includes files.* columns regardless of what the mapping UI shows, so those columns can look "unmapped" in the mapper while still being passed through — worth addressing for UI clarity, but not a functional bug since the data does flow through correctly.

No security, performance, or documentation concerns beyond what's noted above. Didn't run the test suites myself (no Docker/gradle execution in this session) — the existing coverage looks thorough for the new logic.

Comment thread backend/src/main/kotlin/org/loculus/backend/utils/GetSubmittedDataHelpers.kt Outdated
Comment thread website/src/components/Submission/DataUploadForm.tsx Outdated
@tombch tombch removed the preview Triggers a deployment to argocd label Jul 29, 2026
validateSubmissionIdSetsForFiles(metadataSubmissionIds, fileSubmissionIds)
validateFileGroupOwnership(submittedFiles, submissionParams, uploadId)
submissionMetrics.timeWritePhase(endpoint, organism, VALIDATE_FILE_MAPPING_PHASE) {
// File mappings in submissionParams can contain submission Ids not present in the metadata

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah do I understand correctly that the backend actually still supports the old submission format and will accept having the files in a fileMapping JSON?

Sounds like a good rollout strategy! It might be good to still have a follow up PR where the JSON is completely removed to ensure the removal will be smooth - I started doing that locally now and does seem like it will still be a bit of a change!

@tombch tombch Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, currently I haven't removed any of the separate file mapping stuff from the backend, but for single and bulk submission/revision the website now sends the file mapping within the metadata tsv. The submit-edited-data endpoint didn't have to change how it handled the file mapping as that one is completely JSON for everything

@tombch tombch added the preview Triggers a deployment to argocd label Jul 30, 2026
@anna-parker anna-parker mentioned this pull request Jul 30, 2026
3 tasks
}

// TODO: Update when moving away from UUIDs to more user-friendly file IDs
val fileId = try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed adding a specific length check for now to improve the error message in case part of the UUID is accidentally deleted

This comment was marked as low quality.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but maybe it's quick enough it doesn't matter :)

submissionMetrics.timeWritePhase(endpoint, organism, VALIDATE_FILE_MAPPING_PHASE) {
// File mappings in submissionParams can contain submission Ids not present in the metadata
// This is implicitly validated for the file mappings within the metadata column
// TODO: This can be removed once file mappings JSON support is removed

@anna-parker anna-parker Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend tests actually all still rely on the fact that the fileMapping can be passed in submission and do not check that the files will also be parsed from the metadata. I guess this PR is for an intermittent state where both submission options work but might still make sense to update at least some of the tests - I ended up doing that in #7012 (when the fileMapping JSON is completely removed)

@anna-parker

anna-parker commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@tombch and I discussed some blockers for merging this PR:

  • Docs that need updating: https://loculus.org/for-users/submit-extra-files/, also backend endpoints in the FilesController need updating (ah claude actually did this for me in 5c693fb)
  • Adding a migration guide for users using the current feature in the PR description
  • Ensure files.rawReads shows as mapped to a keyword in the column mapping:
image

const text = columnMapping
? await (await columnMapping.applyTo(metadataFile)).text()
: await metadataFile.text();
if (!controller.signal.aborted) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its probably nicer to use if (controller.signal.aborted) return; to have less indented code

const [columnMapping, setColumnMapping] = useState<ColumnMapping | null>(null);

useEffect(() => {
const controller = new AbortController();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure we actually need a controller I think just a boolean called cancelled would be enough (it needs to be set to true in the cleanup function)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend related to the loculus backend component preview Triggers a deployment to argocd website Tasks related to the web application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MVP: feat(website, backend): Update bulk file submission format

3 participants