Add accession_id field to FileMetadataModel and Collection — Closes #36 - #104
Draft
conradbzura wants to merge 24 commits into
Draft
Add accession_id field to FileMetadataModel and Collection — Closes #36#104conradbzura wants to merge 24 commits into
accession_id field to FileMetadataModel and Collection — Closes #36#104conradbzura wants to merge 24 commits into
Conversation
DCC users identify files and experiments by accession, and expect that lookup to ignore case. The usual mechanism for that is a collation-bearing index, which is unavailable here: Amazon DocumentDB 5.0 backs the deployed environments and supports neither the case-insensitive index property nor cursor.collation, both of which arrive only in DocumentDB 8.0. A case-insensitive regex is supported but cannot use an index, so it would scan the whole files collection on every lookup. Normalize instead of collate. Values are folded on the way in and filter values are folded the same way at the API boundary, leaving an ordinary indexed equality match that behaves identically on MongoDB and DocumentDB. Both sides route through this one function, because a divergence between the stored form and the queried form raises nothing -- documents simply become unmatchable. Upper case is the fold direction because it is the form both DCCs already publish, so the stored value stays the display value.
Pins the two properties the case-insensitive lookup contract rests on: folding is idempotent, so a value re-stamped by a later sync cannot drift, and any casing of a value folds to the same result, so a caller's casing cannot change which documents an accession filter matches.
DCC users identify files and experiments by accession, but the accession is not uniformly queryable: 4DN stores an opaque UUID in local_id and carries the accession only inside the persistent_id URL, while ENCODE stores it as local_id. The same lookup therefore needs a different query per DCC, and for 4DN it needs URL reconstruction. One consistently-named field gives callers a single input that works everywhere. Filter values are folded at the point a scalar becomes a MongoDB predicate, rather than at each to_query call site, so a later caller cannot bypass the normalization the stored form depends on. The leaf field name is matched on the last dotted segment, so the top-level accession_id and the nested collections.accession_id fold identically. Only the input types are declared here. The output fields are generated from the pydantic models, so the regenerated schema picks up all four surfaces at once. The field is deliberately left out of the distinct-values allowlist, which is for low-cardinality facet fields; accessions are unique per document.
The query builder had no coverage at all, so these tests pin the whole folding contract rather than only the new field: that folding reaches both the top-level and the nested collection path, that it survives the list-to-OR expansion, that it does not touch sibling fields or non-string leaves, and that a field merely containing the name as a substring is left alone. A property test asserts that an arbitrary re-casing of an accession produces the identical predicate, which is the invariant the case-insensitive lookup depends on.
An accession lookup is the query the field exists to serve, so it needs an index or it is a collection scan over every document. The stored value is already case-folded, so a plain index serves the case-insensitive match that DocumentDB 5.0 cannot serve through a collation. The materializer owns the denormalized files collection and its indexes, so both the top-level and the embedded collection paths are added there. The raw file and collection indexes are added for consistency with the every-field pattern those sets already follow; nothing queries the raw collections by accession today.
4DN local_id values are opaque UUIDs; the accession users recognize lives only inside the persistent_id URL. Both enrichment passes already parse it to key their Search API lookups, so stamping the field is an extension of work already being done rather than a new scan. The stamp is deliberately independent of whether the Search API returned metadata for a document. Both passes only update API-matched documents, so folding the write into their existing bulk operations would have left every unmatched file and collection without an accession -- a sync that reports success while the field is populated for only part of the DCC. It is therefore written from the parsed accession before the API is called, and before the early return taken when the fetch yields nothing. The collection pass runs pre-materialization so the materializer embeds the value into files.collections; the file pass runs post-materialization and writes to files directly. A document whose persistent_id carries no parseable accession is counted and logged rather than failing the sync.
The load-bearing case is a Search API that returns nothing: both passes previously updated only API-matched documents, so these tests fail if the stamp is ever folded back into the existing bulk operations. The remaining tests pin that an unparseable persistent_id leaves the field null without aborting the sync, and that stamping does not displace the experiment metadata the collection pass already promotes.
ENCODE already stores the accession as local_id, so this duplicates a value the document carries. The point is cross-DCC uniformity: 4DN's local_id is an opaque UUID, so only a separate field lets one query input resolve for both DCCs. Only the experiment-keyed collection gets an accession. The biosample-keyed fallback collection is synthesized locally and names no ENCODE experiment, so it is left unset rather than given a fabricated value.
Pins the file accession, the experiment-collection accession, and that the biosample-keyed fallback collection is left without one. A property test asserts the stored value is folded, so it matches what the query builder folds a filter value to. The experiment-collection test supplies a biosample term name because the collection block is gated on it -- an experiment accession alone builds no collection at all, which the arrangement would otherwise hide.
Both 4DN passes built an accession-keyed dict to drive their Search API fetch, and the accession_id stamping reused it. That dict is last-write-wins, so two documents resolving to one accession collapsed to a single entry and the loser was never stamped -- left with a null accession_id despite having parsed cleanly, contradicting the guarantee the helper documents. Which one lost depended on cursor order, and the end state is indistinguishable from a DCC that issues no accession. The stamping now walks a list of one pair per document, while the dict stays for the API fetch where keying by accession is genuinely required. No documents are affected on the current corpus: all 53,697 4DN files on dev resolve to distinct accessions. The guarantee was false rather than the data lost.
An upper-case-only pattern does not simply miss a mixed-case accession, it degrades: it matches the upper-case prefix and returns a truncated accession, so 4DNFImcjxzkh yielded 4DNFI. That is a plausible-looking wrong value rather than the None the callers already count and log, and every mixed-case value truncates to the same short prefix, so a handful of such rows would collide onto one accession and cost each other their stamp. Callers fold the extracted value through normalize_accession anyway, so matching leniently changes nothing for data 4DN actually publishes: a re-scan of all 53,697 dev files returns the identical accession set before and after.
normalize_accession already folds a blank accession to None on the write side, so a document written by some other path was the only way an empty string could reach the models -- where it would read as an accession that exists while matching no filter, since nothing stores one. Deliberately not a folding validator. These models are read-path only, so folding on read would make a mis-stored lower-case value display correctly while remaining permanently unfindable, converting a loud bug into a silent one. The fold belongs at the write and query boundaries.
Added for consistency with the every-field pattern these two spec lists follow, but nothing reads them: the API only ever queries the denormalized files collection, whose indexes the Rust materializer owns and which already carries both accession keys. The stamping passes match on _id and the enrichment cursors filter on submission, so neither touches an accession index either. That leaves pure index-build cost on every sync for a field no query names.
The materializer owns the denormalized files collection and the indexes on it, so its test suite is the only guard on that index list. It has never run automatically: the workflow invokes pytest only, and the Makefile builds the crate without testing it. A commit could therefore change the index list and its sole assertion together, unobserved. A separate job rather than a step in the existing one, because these tests do not vary with the Python matrix and running them per version would compile the crate three times to assert the same thing.
The fake bulk_write assigned each set key directly, so a dotted key such as extra.fourdn landed as a literal flat key instead of nesting. Any test asserting an enrichment payload shape would therefore have passed against a document real MongoDB would have written differently. It also counted matched rather than changed rows, making every modified_count assertion fiction. Routing through the existing _apply_update helper fixes both, since it already implements the nesting and reports whether the row changed.
The property test claiming to pin case-insensitivity compared a value against its own upper-cased form. Since the function's last operation is upper(), that comparison cannot fail by construction, so it asserted nothing while its docstring claimed the contract the whole feature rests on. It now states what it actually pins, strip and upper commuting, and a new property covers the real bidirectional contract over the alphabet the DCCs issue from. The blank-value strategy drew from four whitespace characters where strip removes roughly twenty-five, leaving the one a caller is most likely to paste, a non-breaking space, unexercised. Also grouped into a class per the repo convention, and added interior whitespace and the non-string input contract, the latter reachable because the ingest call sites pass upstream values through unguarded.
The guard that stops folding reaching a non-string value was asserted against a field that is not folded at all, so the test passed with the guard deleted. It now targets the accession key, where removing the guard raises. to_dict and to_query had no coverage of their own before this change, despite every filter the API serves passing through them. Added the flattening contract they had only implicitly: single-clause collapse, None dropping, the two upward clause merges, path construction at depth, and the no-prefix branch. One finding worth recording: to_dict emits fields in declaration order while a dict literal preserves the order written, so the two produce the same conjunction in a different sequence. The equivalence test compares clause sets, since order carries no meaning to MongoDB.
Nothing asserted that a lower-case filter returns an upper-case-stored document, which is the entire feature. Each side was pinned in isolation: the ingest tests cover what gets stored and the query-builder tests cover what predicate gets built, and nothing made the two forms meet. Change the fold on either side and every test stays green while every accession lookup silently returns nothing. Covered at the resolvers over four casings, plus the nested collection output, null serialization for a DCC that issues no accession, the single file lookup, and the count resolver, which builds its query through a separate call site. The HTTP tests drive the same invariant through real BSON, the mongomock matcher and JSON. The nested filter has to use that fixture rather than the shared double: the double resolves dotted paths with dict lookups and cannot traverse the collections array, so the test would fail against correct code. The distinct-values exclusion is pinned because it is a deliberate omission that reads as an oversight, and an introspection test pins the declared shape, which the byte-identical SDL guard cannot: regenerating a wrong schema makes that guard pass.
The load-bearing case is two documents sharing one accession: that test fails against the accession-keyed dict the stamping used to walk, so it guards the fix rather than merely describing it. Also covers the partial case, where the Search API returns metadata for only some accessions and every parsed document must still be stamped, since the two passes deliberately do not share a matching rule. Batching is exercised with a patched batch size so a document cannot be lost at a seam, and the unparseable warning is asserted because it is the operator's only signal that the field is partially populated -- a null accession_id is otherwise indistinguishable from a DCC that issues none. The ENCODE pipeline test covers the only writer of that DCC's collection accession, since ENCODE writes the files collection directly rather than through the materializer.
Both extractors had no coverage of any kind, despite being the only source of the 4DN accession and the place a mixed-case value silently truncated. Covers the canonical and download URL shapes, the token boundary against a trailing extension and a hyphenated suffix, an accession in a query string, the empty and no-match guards, and the minimum length the experiment pattern requires but does not document. The two extractors are also pinned as disjoint, so a collection URL cannot be stamped with a file accession or the reverse. A round-trip property records that what the extractor emits is already in stored form, so re-stamping on a later sync cannot change what is stored.
The property test reimplemented the fold as a local upper() call, which would have let the ingest side and the shared normalizer drift while staying green. It now asserts against normalize_accession itself. Also pins that local_id and accession_id legitimately disagree in case, since local_id is the DCC's own identifier and rewriting it would change the document key. The collection gate test records pre-existing behavior worth knowing: the whole collection block is conditional on the biosample term name, so a row carrying an experiment accession without one contributes no collection at all and that accession is queryable nowhere. Cosmetic before this field existed, a data-completeness question now.
Covers the default, the round trip, and the blank coercion. A property test records the deliberate absence of a folding validator: the read path returns exactly what was stored, so a mis-stored value stays visibly wrong rather than displaying correctly while remaining unfindable.
This module owns the raw C2M2 collections and the Rust materializer owns the denormalized files collection it builds. The module docstring states that split in prose only, so adding a files spec here would silently create a second writer competing with the materializer. Also asserts no index is declared twice, so appending a field to two loops fails here rather than issuing a redundant createIndex against a live database.
The 4DN collection accession is written to the raw collection before materialization and reaches the files collection only because enrich_file clones the whole collection document. Nothing verified that hop, and it would fail silently: the accession would simply be absent, which is indistinguishable from a DCC that issues none.
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
1 participant
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.
Summary
Add a cross-DCC
accession_idfield toFileMetadataModelandCollection, wire it through the GraphQL query layer, index it on the materializedfilescollection, and populate it for 4DN and ENCODE.DCC users identify files and experiments by accession, but the accession is not uniformly queryable today. 4DN stores an opaque UUID in
local_idand carries the accession only inside thepersistent_idURL; ENCODE stores it aslocal_id. The same lookup therefore needs a different query per DCC, and for 4DN it needs URL reconstruction. One consistently-named field gives callers a single input that works everywhere.Callers also expect that lookup to ignore case, and the usual mechanism for that is unavailable here. Amazon DocumentDB 5.0 backs the deployed environments and supports neither the case-insensitive index property nor
cursor.collation()— both arrive only in DocumentDB 8.0 — and a case-insensitive$regexcannot use an index, so it would scan the whole collection on every lookup. A collation-based implementation would also pass against a developer's local MongoDB and fail only once deployed. The field is therefore normalized rather than collated: stored already case-folded, with filter values folded the same way at the API boundary, leaving an ordinary indexed equality match that behaves identically on both engines.HuBMAP is deliberately not populated. It has no per-file accession concept — files are matched by filename within a dataset — and its dataset-level
hubmap_idis not ingested today. Tracked in #102.Closes #36
Closes #37
Closes #38
Proposed changes
The field and its query surface
Add
accession_idto both models and toFileMetadataInputandCollectionInput. The output fields require no code:FileMetadataTypeis generated from the pydantic model, so the regeneratedschema.graphqlpicks up all four surfaces at once.Fold filter values inside
to_query, at the single point where a scalar becomes a MongoDB predicate, rather than at each of the three resolver call sites — a later caller cannot then bypass the normalization the stored form depends on. Match on the last dotted segment so the top-levelaccession_idand the nestedcollections.accession_idfold identically.Keep the field out of the distinct-values allowlist. That allowlist is for low-cardinality facet fields a client can enumerate; an accession is unique per document.
Population
Populate 4DN in the two existing enrichment passes, which already parse the accession to key their Search API lookups. Write it independently of whether the API returned metadata for a document: both passes otherwise update only API-matched documents, which would leave the field populated for part of the DCC while the sync reported success.
Populate ENCODE at both document builders, where the accession is already in hand. Only the experiment-keyed collection gets one — the
biosample:-keyed fallback collection names no ENCODE experiment.Indexing
Add
accession_idandcollections.accession_idto the materializer's index list. The materializer owns the denormalizedfilescollection the API actually queries; the raw C2M2 collections are deliberately left unindexed for this field, since nothing reads them by accession.Defects found and fixed
Three latent defects surfaced during review, each verified against the live corpus (53,697 4DN files on dev: zero collisions, zero unparseable, entirely uppercase).
Stamping was keyed by accession rather than by document. Both passes build an accession-keyed dict for their API fetch, and the stamping reused it. That dict is last-write-wins, so two documents resolving to one accession collapsed to a single entry and the loser was never stamped — contradicting the guarantee the helper documents, non-deterministically, and ending in a null that is indistinguishable from a DCC issuing no accession. Now walks one pair per document. No documents are affected on the current corpus; the guarantee was false rather than the data lost.
The 4DN accession patterns were upper-case only. That does not simply miss a mixed-case accession, it truncates one:
4DNFImcjxzkhyielded4DNFI, a plausible-looking wrong value rather than theNonethe callers already count and log — and every such value truncates to the same prefix, manufacturing exactly the collisions above. Now case-insensitive. A re-scan of all 53,697 dev files returns an identical accession set before and after.An empty accession could reach the models as
"". Coerced toNonevia the existing validator, matching whatnormalize_accessionalready does on the write side. Deliberately not a folding validator: these models are read-path only, so folding on read would make a mis-stored value display correctly while remaining permanently unfindable.A fourth defect is out of scope and filed as #103:
files(input: [{}])is a legal document that flattens to{"$and": []}, which MongoDB rejects.Test infrastructure and CI
Run the materializer's tests in CI. They are the only guard on the
filesindex list and had never run automatically — the workflow invoked pytest only, and the Makefile builds the crate without testing it.Fix the fake collection's
bulk_write, which assigned$setkeys directly so a dotted key landed flat instead of nesting, and counted matched rather than changed rows.Test cases
The suite grows from 842 to 941 tests, plus one new materializer test.
TestNormalizeAccessionTestNormalizeAccessionTestNormalizeAccessionNoneTestNormalizeAccessionNonevalueAttributeErrortests/test_inputs.pyaccession_idkeyto_querybuilds the predicateisinstanceguard, which a non-folded field could not exercisetests/test_inputs.pyaccession_idinsidecollectionsto_queryflattens ittests/test_inputs.pyFileMetadataInputwith every field unsetto_dictconverts itto_queryrelies on receiving the full settests/test_inputs.pyto_queryflattens it$andrather than a nested oneTestFilesQueryTestFilesQuerycollections { accessionId }TestFileCountQueryfileCountfilters in lower caseTestDistinctValuesQuerydistinctValuesrequests eachtests/test_metadata_endpoint.py/metadatatests/test_metadata_endpoint.pyTestSetAccessionIdsTestSetAccessionIdsTestEnrich4dnApiMetadataTestExtractAccessionTestExtractExperimentAccessionNonetests/test_encode.pytests/test_encode.pyaccession_idequals the foldedlocal_idtests/test_indexes.pyfileand excludesfilesmaterialize/src/main.rsenrich_filerunsfilesonly by document cloneKnown limitation. The 4DN collection accession is written pre-materialization and reaches
filesthrough the materializer's whole-document clone. Test 23 pins that hop, but the rawcollectioncollection is not exposed through the API, so it could not be scanned for duplicate accessions the way the file side was.Migration note.
accession_idis written during sync and materialization, so existing documents stay null until the next sync. Until then an accession filter returns zero matches rather than erroring.