feat(deployment): reject config where metadata and file category names overlap - #7264
Draft
corneliusroemer-agent wants to merge 1 commit into
Draft
feat(deployment): reject config where metadata and file category names overlap#7264corneliusroemer-agent wants to merge 1 commit into
corneliusroemer-agent wants to merge 1 commit into
Conversation
…verlap Released data, the website config and the SILO database config each flatten three sources into one set of metadata keys: the chart's commonMetadata, the organism's own schema.metadata (after metadataAdd is merged in), and the file categories from schema.files and submissionDataTypes.files.categories. Every one of those merges lets one source silently win over another, so an overlap never raises an error at runtime - a field quietly disappears, or a real metadata value gets replaced by a file-list JSON string. In the SILO database config it also emits a duplicate metadata entry. Validate it where the three lists are all in scope. The backend cannot do this: its config deliberately omits commonMetadata, because schema.metadata doubles as the whitelist of submittable fields (ProcessedSequenceEntryValidator rejects unknown metadata keys), so giving it commonMetadata would make `accession` and `submitter` acceptable as pipeline-supplied metadata. Two rules, both per organism: - schema.metadata (incl. metadataAdd) must not overlap commonMetadata - file categories must not overlap commonMetadata or schema.metadata The template renders no manifests; it exists only to fail early, so a bad config is caught by `helm lint` (which CI already runs over three values files) or at `helm install`, rather than becoming a silently missing field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TfNESPRGqoCmoh7Dr16Mt8
corneliusroemer-agent
force-pushed
the
validate-config-field-name-overlap
branch
from
September 8, 2026 17:11
cb2ca5d to
0f26043
Compare
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.
While reviewing #6816 I realized that things would break silently if file category names match metadata fields. Similar issues arise if metadata fields match common-metadata (reserved) fields like "accession".
The best way to validate this is in the helm chart as it's the only place where common-metadata vs normal metadata is defined, and it also knows about file categories. I first wanted to put the validation in the backend but that would mean duplicating the reserved names.
What I did not cover
perSegmentfields get expanded toname_L/name_M/name_Sby the SILO and website templates, so a file category named e.g.length_Lcould still collide with an expanded name. Catching that would mean duplicating the segment-expansion logic inside the validator, which did not feel worth it.I also left
earliestReleaseDateexactly as it is. It is the one name that is both injected by the backend and declared in organism metadata. Earliest release date should really go in common-metadata but its per-organism nature makes that a little involved so keeping it out of this PR for now.Claude description
Released data, the website config and the SILO database config each flatten three separate sources into one set of metadata keys: the
commonMetadatafields the chart adds to every organism, the organism's ownschema.metadata(aftermetadataAddis merged in), and the file categories fromschema.filesandsubmissionDataTypes.files.categories.Every one of those merges silently lets one source win over another, so an overlap never raises an error anywhere. A field just quietly disappears, or a real metadata value gets replaced by the file-list JSON string, and in the SILO database config you additionally get two metadata entries with the same name. This is an implicit requirement of code we already have, and nothing currently checks it.
This came up while reviewing #6816, which adds a
rawReadsfile category — but it applies to any file category or metadata field, so it is worth having independently of that PR.Why in helm rather than the backend
I tried this in the backend first and it is the wrong place. The backend config deliberately does not receive
commonMetadata, becauseschema.metadatadoubles as the whitelist of fields that may be submitted —ProcessedSequenceEntryValidatorrejects any metadata key outside it. Handing the backendcommonMetadatawould makeaccession,submitterand friends acceptable as pipeline-supplied metadata, so it cannot see the list it would need to check against. Doing it there means hand-mirroring the reserved names in Kotlin, which drifts: my first attempt already missedversionComment.In the chart all three lists are in scope at once, so there is no duplicated list to keep in sync.
What I did not cover
perSegmentfields get expanded toname_L/name_M/name_Sby the SILO and website templates, so a file category named e.g.length_Lcould still collide with an expanded name. Catching that would mean duplicating the segment-expansion logic inside the validator, which did not feel worth it — happy to add it if you disagree.I also left
earliestReleaseDateexactly as it is. It is the one name that is both injected by the backend and declared in organism metadata, which initially looked like it needed a special case. It does not: because the rule compares organism metadata againstcommonMetadata, andearliestReleaseDateis not incommonMetadata, there is nothing to exempt — and a file category colliding with it is still caught, via the organism-metadata side of the second rule. So no config migration is needed.For the reviewer
The first rule only forbids organism metadata overlapping
commonMetadata. The second forbids file categories overlapping either list — I included categories-vs-commonMetadataas well as categories-vs-organism-metadata since it costs nothing and the website and SILO configs really do merge all three, but say if you want it narrowed.🤖 Generated with Claude Code
https://claude.ai/code/session_01TfNESPRGqoCmoh7Dr16Mt8
🚀 Preview: Add
previewlabel to enable