Add accession_id field to FileMetadataModel and Collection — Closes #36, #37, #38, & #103 - #104
Merged
Merged
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.
Four defects in one pass, all of which leave a null or stale accession_id that is indistinguishable from a DCC issuing none. The file accession is stamped on the raw file collection before materialization rather than on files afterwards. The materializer rebuilds files from the raw documents on every run, so the later write was erased by any standalone materialize-dcc invocation -- silently, and only for the file level, leaving the corpus internally inconsistent with the collection accessions still present. The accession-to-id lookup keys a list rather than a single id, so two files resolving to one accession are both enriched instead of the loser being dropped by cursor order. The collection pass collects and stamps before fetching experiment metadata. It previously fetched first, and the fetch catches only ClientError, so a timeout propagated and nothing was stamped at all -- the file pass already ordered this correctly. A failed stamp batch is logged and skipped rather than raised. Stamping is now the first write of each pass, so an escaping BulkWriteError would abort enrichment that would otherwise have succeeded and fail the whole sync, costing more than the accessions it failed to write.
An upper-case-only pattern did not merely miss a mixed-case accession, it matched the upper-case prefix and returned a truncated one -- 4DNFImcjxzkh became 4DNFI, a plausible-looking wrong answer rather than the None the callers already count and log, and every such value truncated to the same prefix. Matching leniently while returning the raw match would have moved the failure rather than removed it. The extracted value is also the key for the Search API round trip, and the portal answers with its own upper-case form, so a mixed-case match joined against nothing: the file kept a correct accession_id and silently lost every enriched field, without being counted in the unparseable warning that is the operator's only signal. Folding at extraction makes the canonical accession the only value any caller can obtain.
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 files collection and its indexes, so its tests are the only guard on that index list and had never run automatically -- the workflow invoked pytest only, and the Makefile builds the crate without testing it. A guard is only as good as its reproducibility. Cargo.lock was gitignored, so every run re-resolved dependencies and a semver compatible upstream release could turn the job red on an unrelated change, which is how a guard gets labelled flaky and then ignored. It is now tracked and enforced with --locked. The toolchain is pinned by action SHA like every other action here, and a cargo cache keeps the job from compiling the full dependency tree on each push.
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.
The Rust materializer creates the files indexes at the end of its run and is their only writer, but the ENCODE sync never invokes it -- it writes documents straight into files. On a database where ENCODE is the only DCC synced, files therefore carried no index at all, and every accession lookup scanned the whole collection on a public endpoint. The new spec list is deliberately narrow. Mirroring the materializer's full set here would recreate the duplicate-writer problem that keeps files out of the data specs in the first place; ensuring only the two accession keys costs nothing when the materializer has already made them, because identical keys derive identical default names.
An accession filter fails silently in both directions. A query against an unstamped corpus returns no matches and no error, which reads exactly like the accession not existing, and a DCC that issues no accession at all looks identical. Neither the API nor a client can tell those apart. This log is the only place the distinction is visible, which also makes it the signal that a standalone re-materialization dropped the file accessions. It is advisory: a coverage shortfall is not a sync failure, and a counting error never propagates into the sync path.
FakeCollection.create_index was synchronous while Motor's is awaited, so it returned None into an await and any code path that ensured indexes could not be tested at all -- the divergence surfaced the moment one needed to be. It is now async, with register_index kept as the synchronous seam for test arrangement, so a test that only needs the double pre-seeded does not have to be async to say so. The existing arrangement helpers move to that seam rather than becoming async themselves, which keeps sixty call sites unchanged.
The double's bulk_write is shared infrastructure, but the file that exists to pin it against real Mongo semantics had no bulk_write test at all. Its dotted-key nesting was covered only incidentally by an enrichment test, and its changed-versus-matched row counting was covered nowhere: every existing assertion is on rows that genuinely change, so reverting that half would have been invisible while making every modified_count assertion in the suite fiction.
The headline field of this work was discoverable only by reading the GraphQL SDL. It has more caveats than compression_format, which earned two paragraphs for exactly this reason, and none of them were written down: it is permanently null for HuBMAP, null for any 4DN file whose persistent_id does not parse, case-folded so it can legitimately differ from ENCODE's local_id in case, and null everywhere until each DCC is re-synced. That last one matters most. A filter against an unstamped corpus returns no matches and no error, so a deployment that has not re-synced looks exactly like one where the accession does not exist, and nothing told an operator which they were looking at. The 4DN module's entity-matching table also still described its patterns as upper-case only, stale since they were made case-insensitive, and had gained no row for the field its sibling ENCODE module documents.
conradbzura
force-pushed
the
36-accession-id-field-and-population
branch
from
August 12, 2026 16:33
c9719bd to
5c8d82c
Compare
conradbzura
changed the title
accession_id field to FileMetadataModel and Collection — Closes #36accession_id field to FileMetadataModel and Collection — Closes #36, #37, #38, & #103
conradbzura
changed the title
accession_id field to FileMetadataModel and Collection — Closes #36, #37, #38, & #103
conradbzura
marked this pull request as ready for review
August 12, 2026 17:11
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
accession_idtoFileMetadataModelandCollection, expose it on both filter inputs, index it on the materializedfilescollection, and populate it for 4DN and ENCODE.An accession is the identifier a DCC user actually has, and today it is reachable by a different query per DCC. 4DN stores an opaque UUID in
local_idand carries the accession only inside thepersistent_idURL. ENCODE stores the accession aslocal_id. A caller holding4DNFIMCJXZKHtherefore has to know which DCC issued it and reconstruct a URL to find the file. One consistently-named field replaces that with a single input.Callers expect accession lookup to ignore case, and the usual mechanism for that is unavailable here. Amazon DocumentDB 5.0 backs the deployed environments (
cloudformation/database.ymlpinsdocdb5.0) and supports neither the case-insensitive index property norcursor.collation(); both arrive in DocumentDB 8.0. A case-insensitive$regexcannot use an index, so it would scan the 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 identically at the API boundary, leaving an ordinary indexed equality match that behaves the same on both engines.src/cfdb/accessions.pyowns the fold. Ingest and query both route through it, so the stored form and the queried form cannot drift apart independently.HuBMAP is deliberately unpopulated. It has no per-file accession concept — files are matched by filename within a dataset — and its dataset-level
hubmap_idis not ingested. Tracked in #102.Closes #36
Closes #37
Closes #38
Closes #103
Proposed changes
The field and its query surface
Add
accession_idto both models and toFileMetadataInputandCollectionInput. The output fields need no code:FileMetadataTypeis generated from the pydantic model, so regeneratingschema.graphqlpicks up all four surfaces.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 full flattened path —
accession_idandcollections.accession_id— rather than on the last dotted segment. Both paths are known statically, so enumerating them costs nothing and closes a trap: this codebase keeps DCC-native values underextra.<dcc>.*, and a futureextra.fourdn.accession_idholding an upstream value verbatim would be folded on query while stored unfolded. Nothing raises in that case; the documents simply become unmatchable. Full-path matching makes a new field fail closed.Keep the field out of the distinct-values allowlist, which exists for low-cardinality facet fields a client can enumerate. An accession is unique per document.
Population and durability
Both 4DN accessions are stamped on the raw C2M2 collections before materialization. The materializer rebuilds
filesfrom those raw documents and drops what was there, so a value written tofilesafterwards survives only until the nextmake materialize-dcc— a documented operator command. Stamping the raw document instead letsenrich_filecarry it forward on every rebuild, the file's by in-place mutation and the collection's by whole-document clone.Stamping runs before each Search API fetch and independently of its result. Both passes otherwise update only API-matched documents, which would leave the field populated for part of a DCC while the sync reported success. A failed stamp batch is logged and skipped rather than raised, because stamping is now the first write of each pass and an escaping
BulkWriteErrorwould abort enrichment that would otherwise have succeeded.The extractors return the accession already folded. An upper-case-only pattern does not merely miss a mixed-case accession, it matches the upper-case prefix and returns a truncated one:
4DNFImcjxzkhyields4DNFI, a plausible-looking wrong answer rather than theNonethe callers already count and log. The extracted value is also the key for the Search API round trip, and the portal answers with its own upper-case form, so returning the raw match would move that failure rather than remove it — the file would keep a correctaccession_idand silently lose every enriched field.ENCODE populates both builders directly. Only the experiment-keyed collection gets an accession; the
biosample:-keyed fallback names no ENCODE experiment.Log accession coverage per DCC after each sync. A filter against an unstamped corpus returns no matches and no error, which is indistinguishable from the accession not existing and from a DCC that issues none. This log is the only place those cases separate.
Indexing
Add
accession_idandcollections.accession_idto the materializer's index list, which owns thefilescollection the API queries. The raw C2M2 collections are left unindexed for this field, since both stamping passes match by_id.The ENCODE sync writes
filesdirectly and never invokes the materializer, so a database with ENCODE as its only DCC would carry no index onfilesat all. It now ensures the two accession keys itself. The spec list is deliberately narrow — mirroring the materializer's full set would recreate the duplicate-writer problem that keepsfilesout of the data specs.Empty filter clauses
files(input: [{}])is a legal GraphQL document that flattened to{"$and": []}, which MongoDB and DocumentDB both reject outright. An empty clause list now collapses to{}. This is also what makes a blank accession safe to drop:accessionId: [""]empties the enclosing$orthe same way.A blank accession contributes no constraint. Emitting
{accession_id: null}instead would match documents whose accession is null or absent — all of HuBMAP, every 4DN file whosepersistent_iddid not parse, and the entire corpus before the first post-deploy sync. That made a blank value the only filter in the schema that widened the result set, where every sibling string field matches nothing.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.materialize/Cargo.lockis now tracked and enforced with--locked, the toolchain action is SHA-pinned, and a cargo cache keeps the job from compiling the dependency tree on every push.FakeCollection.create_indexwas synchronous while Motor's is awaited, so it returnedNoneinto anawaitand no code path that ensures indexes could be tested. It is now async, withregister_indexas the synchronous seam for test arrangement.bulk_writeroutes$setthrough the shared update helper, so dotted keys nest as Mongo nests them andmodified_countcounts changed rather than matched rows.Test cases
102 tests across ten files. Grouped by the behavior they pin.
tests/test_accessions.pytests/test_accessions.pytests/test_accessions.pyNonetests/test_inputs.pyaccession_idat top level and insidecollectionsto_queryflattens ittests/test_inputs.pyaccession_idunderextra.fourdnto_querybuilds the predicatetests/test_inputs.pyto_querybuilds the querytests/test_inputs.pyto_querybuilds the querytests/test_inputs.pyto_querybuilds the querytests/test_schema.pytests/test_schema.pydistinctValuesrequests eachtests/test_schema.pyaccessionIdis a nullable String on both output types and a list on both inputstests/test_metadata_endpoint.py/metadatatests/test_metadata_endpoint.pyfiles(input: [{}])is POSTedtests/test_sync.pypersistent_idcarries an accessionfilecollection and leavesfilesuntouchedtests/test_sync.pypersistent_idresolves to one accessiontests/test_sync.pypersistent_idand an API keyed on the canonical formtests/test_sync.pytests/test_sync.pybulk_writethat raisesBulkWriteErrortests/test_sync.pyfilestests/test_sync.pytests/test_fourdn.pytests/test_fourdn.pypersistent_idcarrying a file accessionNonetests/test_encode.pytests/test_encode.pyaccession_idand leaveslocal_idas publishedtests/test_indexes.pytests/test_fake_collection.pybulk_writeapplies that updatemodified_countmeans modifiedmaterialize/src/main.rsenrich_filerunsMigration.
accession_idis written during sync, so existing documents stay null until each DCC is re-synced. Until then an accession filter returns zero matches rather than erroring. The per-DCC coverage log makes that state visible to an operator.Known limitation. The raw
collectioncollection is not exposed through the API, so it could not be scanned for duplicate accessions the way the file side was against the 53,697 4DN files on dev.