Consolidate Asset/Dandiset models; gate publish validation on datePublished - #419
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #419 +/- ##
==========================================
- Coverage 47.48% 47.45% -0.04%
==========================================
Files 19 19
Lines 2367 2396 +29
==========================================
+ Hits 1124 1137 +13
- Misses 1243 1259 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a15e2e9 to
1298b26
Compare
30cf2fd to
a08719f
Compare
Temporarily point the dandischema dependency at the consolidate-models branch (dandi/dandi-schema#419), where BareAsset.schemaKey is "BareAsset", so CI runs the dandi-cli client against the consolidated schema and checks that this branch still uploads schemaKey="Asset". For checking the test outcome only; not intended to be merged. Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
… value `prepare_metadata()` returns a `BareAsset`, whose top-level `schemaKey` is `"Asset"` with the current dandischema but `"BareAsset"` under dandi/dandi-schema#419 (which pins `BareAsset.schemaKey` to its class name). To keep this test passing against either dandischema version, the expected `schemaKey` is set to the default of the `schemaKey` field in `BareAsset` before the comparison, and the four `metadata2asset*.json` files now store a placeholder for the field. The later `validate()` call checks the data against the `Asset` class, so `data_as_dict` is promoted to an Asset (`schemaKey = "Asset"`) beforehand.
Temporarily point the dandischema dependency at the consolidate-models branch (dandi/dandi-schema#419), where BareAsset.schemaKey is "BareAsset", so CI runs the dandi-cli client against the consolidated schema and checks that this branch still uploads schemaKey="Asset". For checking the test outcome only; not intended to be merged. Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
a08719f to
1277a00
Compare
Temporarily point the dandischema dependency at the consolidate-models branch (dandi/dandi-schema#419), where BareAsset.schemaKey is "BareAsset", so CI runs the dandi-cli client against the consolidated schema and checks that this branch still uploads schemaKey="Asset". For checking the test outcome only; not intended to be merged. Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
… value `prepare_metadata()` returns a `BareAsset`, whose top-level `schemaKey` is `"Asset"` with the current dandischema but `"BareAsset"` under dandi/dandi-schema#419 (which pins `BareAsset.schemaKey` to its class name). To keep this test passing against either dandischema version, the expected `schemaKey` is set to the default of the `schemaKey` field in `BareAsset` before the comparison, and the four `metadata2asset*.json` files now store a placeholder for the field. The later `validate()` call checks the data against the `Asset` class, so `data_as_dict` is promoted to an Asset (`schemaKey = "Asset"`) beforehand.
Temporarily point the dandischema dependency at the consolidate-models branch (dandi/dandi-schema#419), where BareAsset.schemaKey is "BareAsset", so CI runs the dandi-cli client against the consolidated schema and checks that this branch still uploads schemaKey="Asset". For checking the test outcome only; not intended to be merged. Co-Authored-By: Claude Code 2.1.202 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
1e81506 to
18379cf
Compare
bff378f to
60cc4de
Compare
|
note to myself, |
|
@satra we are thinking to proceed with this transition on how we test for publisher vs draft dandiset or asset, so that we do not breed the models and rather add "rules" (which could be expressed in linkml as well). Side-effect -- jsonschema validation will be more relaxed for "PublishedDandiset" because they do not translate. After we migrate to linkml with What do you @satra think about this transition? |
60cc4de to
dbe5387
Compare
|
Yes, let's bump 1. The bump is already due, independently of this PR. 2. #419 then changes all four files further, so reusing Not breaking. Every Summary of the diff, published
|
| file | change | kind |
|---|---|---|
dandiset.json |
+ doi, + publishedBy, + datePublished, + releaseNotes |
optional, readOnly |
dandiset.json |
+ $defs.PublishActivity |
pulled in by publishedBy, its only $ref |
dandiset.json |
url.description reworded |
doc only |
asset.json |
+ publishedBy, + datePublished |
optional, readOnly |
asset.json |
+ $defs.PublishActivity |
as above |
Since all of these are readOnly, the Meditor (which hides readOnly properties) is unaffected.
published-dandiset.json and published-asset.json become byte-identical to their draft counterparts:
| file | change | effect |
|---|---|---|
published-dandiset.json |
- required: datePublished, publishedBy, url |
relaxation |
published-dandiset.json |
id.pattern widened to accept draft ids and lowercase instance prefixes |
relaxation |
published-dandiset.json |
doi.default "" -> null; datePublished/publishedBy/url gain default: null |
annotation only, default is not a constraint |
published-dandiset.json |
title "Published Dandiset" -> "Dandiset", + description |
cosmetic |
published-asset.json |
- required: datePublished, publishedBy |
relaxation |
published-asset.json |
id.pattern (^dandiasset:...$) dropped |
relaxation |
published-asset.json |
title "Published Asset" -> "Asset", + description |
cosmetic |
context.json drops the PublishedAsset and PublishedDandiset term mappings, since generate_context() walks the model classes and those two names are now aliases. No stored metadata carries those schemaKey values, so this is inert in practice, but it is a vocabulary removal and another reason not to reuse 0.7.0.
Also present in the diff but coming from master, not from this PR: + sameAs on both Dandiset schemas, access.readOnly on both Asset schemas, and a reordering of the LicenseType enum, which is the non-determinism tracked by #433 rather than a real change.
The one thing worth flagging: published-*.json stops expressing publication requirements
This is the change most likely to be read as breaking, so to be explicit about what it does and does not cost.
Enforcement is not lost through dandischema. validate() always runs Pydantic validation after the optional JSON Schema pass, and PublishedDandiset/PublishedAsset are aliases of Dandiset/Asset, whose check_publication_status re-checks every dropped rule once datePublished is set: publishedBy presence, url presence and pattern, doi presence, the id patterns, assetsSummary file and byte counts, and the sha2-256 digest check. dandi-archive's publish-time call sites pass schema_key='PublishedDandiset'/'PublishedAsset', so they keep the full check. Worth knowing too: validate(..., json_validation=True) uses the locally generated schema whenever schema_version == DANDI_SCHEMA_VERSION, and reads dandi/schema only for metadata stamped with an older version, fetching releases/<that version>/. Publishing 0.8.0 only adds releases/0.8.0/ and leaves every existing release directory alone, 0.7.0 included, so a record stamped 0.7.0 still gets validated against the strict releases/0.7.0/published-dandiset.json. The relaxation applies to 0.8.0-stamped metadata onward, not retroactively.
What is lost is enforcement for anyone consuming published-*.json standalone, outside dandischema: from 0.8.0 on, those files say nothing about publication. That follows from the constraints now being conditional on datePublished, which a flat required list cannot express. This is an acceptable gap for now, since the plan is to re-encode these requirements as LinkML rules in the LinkML translation, where gen-json-schema emits them as if/then and the published artifact gets the constraint back.
One direction in which the draft schemas get slightly stricter
"Additive" means added schema text, which is not quite the same as a pure relaxation. Neither asset schema sets additionalProperties at all (both Dandiset schemas set it to true), so unknown properties have always been accepted and never validated. Giving publishedBy and datePublished a declared subschema therefore type-checks documents that already carried those names:
| document | published 0.7.0 asset.json |
this branch |
|---|---|---|
| minimal valid asset | valid | valid |
+ "datePublished": 12345 |
valid | invalid, 12345 is not of type 'string' |
+ "publishedBy": 12345 |
valid | invalid, not valid under any of the given schemas |
+ "someRandomKey": 12345 |
valid | valid |
Nothing is at risk in practice: draft records do not carry these fields, and published records carry values that already had to satisfy the identical subschemas in published-*.json. Noting it for completeness, since it is the only place where this PR makes a schema stricter.
The schemas generated from `master` already differ from the published `0.7.0` in `dandi/schema`, and this branch changes all four of them further, so the current version cannot be reused. Add `"0.7.0"` to `ALLOWED_INPUT_SCHEMAS` so that metadata stamped with it stays valid input for `migrate()`. For the full diff against the published `0.7.0` and the reasoning behind the bump, see #419 (comment) Co-Authored-By: Claude Code 2.1.220 / Claude Opus 5 <noreply@anthropic.com>
|
dandi-cli fails due to "expected"
I now - re-wondering if "both" in our matrix should mean that server should have got it updated too, and thus potentially it should have all being peachy @candleindark ? edit, e.g. we do have one dandi-cli on linux (where we can test using docker) passing
|
The failures are expected due the way the tests are set up. However, the set up of those tests are buggy.
Particularly, "both", and "server", failed to set the run environment of the |
|
I see, so we kinda broke our testing setup when jump over to use |
Yes. I will push it out and see how it works. |
| # above. That sync must be skipped for this image to run the schema version | ||
| # under test; skipping it is safe because the base image's `uv sync` has already | ||
| # installed every other dependency. | ||
| ENV UV_NO_SYNC=1 |
There was a problem hiding this comment.
@yarikoptic This eliminate two CI test failures.
|
@yarikoptic I eliminated two test failures in #419 (comment). The remaining three test failures, not including the codecov ones, can't be eliminate in this PR and are expected (see "Expected This PR is good to merge. However, I want you to take a second look, particular at the changes I put in after your last approval. Note: The PR description has been updated with the latest changes. |
Version bump has been added. Fix regarding the failed test has been put it. There are remaining an expected failures from the CI.
Re-review is warranted.
|
oh -- what to add/do in https://github.com/dandi/dandi-schema/blob/master/dandischema/metadata.py#L303 |
|
@yarikoptic Nothing needs to be added to This PR neither adds nor removes a restriction on existing metadata. A draft record carries no The earlier PRs contribute exactly three deltas between the published
Checked rather than only reasoned about:
|
|
@yarikoptic Preferably, let's merge #434 before this PR is merged and released so that we don't have to adjust the |
…Published
Collapse the publication-specific model variants into their base classes so
that each class's schemaKey value matches its class name, which is what an
eventual LinkML translation needs to use schemaKey as a type designator
(designates_type). The three classes whose schemaKey differed from their class
name (BareAsset -> "Asset", PublishedAsset -> "Asset", PublishedDandiset ->
"Dandiset") were the blockers.
Changes (all in dandischema/models.py):
- Merge PublishedDandiset into Dandiset and PublishedAsset into Asset. The
publication-only fields (doi, publishedBy, datePublished, releaseNotes on
Dandiset; publishedBy, datePublished on Asset) become optional, and the
publication requirements move into a datePublished-gated
`check_publication_status` model validator on each class:
- when datePublished is None (a draft), the publication-only fields must be
absent;
- when datePublished is set (published), enforce the former Published*
requirements (publishedBy/url/doi presence, the stricter id/url patterns,
check_filesbytes, digest_sha256check). All violations are reported
together in one error.
dandi-archive's publish flow injects datePublished before validating, so the
gated checks fire exactly as the Published* classes did before.
- Keep BareAsset as a distinct class (Asset still inherits from it) but align
its schemaKey to Literal["BareAsset"], so both BareAsset and Asset are
schemaKey-aligned. The client (dandi-cli) is responsible for setting
schemaKey to "Asset" when uploading bare metadata as an Asset.
- Remove the Publishable mixin; add PublishedDandiset/PublishedAsset as
deprecated aliases of Dandiset/Asset for backward compatibility (dandi-cli,
dandi-archive). These will be removed in a follow-up once consumers migrate.
- Simplify DandiBaseModel.ensure_schemakey to a plain schemaKey == class-name
check now that no class intentionally diverges.
to_datacite now asserts the published precondition (datePublished set) that the
PublishedDandiset type used to guarantee. Tests updated for the merged models:
the published variants report the same missing fields as their base classes,
and the publication requirements are exercised on complete, datePublished
instances; new tests cover the publication-coherence invariant.
Generated JSON Schema diff: the draft schemas (Dandiset, Asset) gain only
optional, readOnly publication properties (nothing newly required, no pattern
tightened), so the dandi-archive Meditor is unaffected; the published-*.json
schemas become the relaxed versions, with publication strictness now enforced
by the gated Pydantic validator.
Co-Authored-By: Claude Code 2.1.161 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
# Conflicts:
# dandischema/tests/test_models.py
The `check_publication_status` model-validators on `Dandiset`/`Asset`
previously raised a single combined `ValueError`, which pydantic reports with
an empty `loc` (`()`), losing the per-field association and tripping consumers
that index `error["loc"][0]` (e.g. dandi-archive's `_encode_pydantic_error`).
Build a `list[InitErrorDetails]` instead — one entry per failing check, each
with its own `loc=(field,)`, `type="value_error"`, and the message carried in
`ctx={"error": ...}` — and raise it via
`pydantic_core.ValidationError.from_exception_data(...)`. This keeps the single
accumulating model-validator design (all violations reported together) while
giving every publication error a proper field `loc`, and preserves the exact
"Value error, <message>" text the former field-validators produced.
test_dandimeta_1 now asserts the per-field locs rather than one combined
message.
Co-Authored-By: Claude Code 2.1.198 / Claude Opus 4.8 claude-opus-4-8 <noreply@anthropic.com>
The schemas generated from `master` already differ from the published `0.7.0` in `dandi/schema`, and this branch changes all four of them further, so the current version cannot be reused. Add `"0.7.0"` to `ALLOWED_INPUT_SCHEMAS` so that metadata stamped with it stays valid input for `migrate()`. For the full diff against the published `0.7.0` and the reasoning behind the bump, see #419 (comment) Co-Authored-By: Claude Code 2.1.220 / Claude Opus 5 <noreply@anthropic.com>
99c09bf to
227cdd0
Compare
|
Updated the PR description to reflect that the two |
`models.yaml` no longer defines them since #419 consolidated them into `Dandiset` and `Asset`, so `gen-json-schema` emitted rootless schemas that accept any object. The filename `sed` existed only for those two targets and goes with them. Co-Authored-By: Claude Code 2.1.224 / Claude Opus 5 <noreply@anthropic.com>


Summary
Make every model class's
schemaKeyvalue equal its class name, so that an eventual LinkML translation can useschemaKeyas its type designator (designates_type). Three classes violated this (BareAsset→"Asset",PublishedAsset→"Asset",PublishedDandiset→"Dandiset"), and fixing them takes two distinct changes:PublishedDandisetmerges intoDandisetandPublishedAssetintoAsset. Publication requirements now fire via adatePublished-gated validator instead of separate classes.BareAsset.schemaKeyto"BareAsset"Stored
schemaKeyvalues in archive data are unchanged, and the old names remain as deprecated aliases sodandi-cli/dandi-archiveimports keep working. Adopting this release does, however, require one lockstepdandi-clichange (see Prerequisites).DANDI_SCHEMA_VERSIONis also bumped to0.8.0, which is due independently of the model consolidation: the schemas generated frommasteralready differ from the published0.7.0, and this PR changes all four of them further. A summary of the diff against the published0.7.0, and of why none of it is breaking for existing metadata, is in #419 (comment).Prerequisites (merge first)
missing_okparameter frommetadata.validate()#422 — removes themissing_okparameter ofmetadata.validate. That parameter is never really used, and its feature never actually worked as intended (see Remove themissing_okparameter frommetadata.validate()#422 for details); merging it first frees this PR from having to keep that behavior "working" while it reshapes the validation errors.schemaKey = "Asset"on bare asset metadata at upload, sinceBareAsset.schemaKeyis now"BareAsset"and the server does not normalize it.What changed in
dandischema/models.pyMerge
PublishedDandiset→Dandiset,PublishedAsset→Asset. Publication-only fields (doi,publishedBy,datePublished,releaseNotesonDandiset;publishedBy,datePublishedonAsset) become optional, and the publication requirements move into adatePublished-gatedcheck_publication_statusmodel validator:datePublishedisNone(draft) → publication-only fields must be absent;datePublishedset (published) → enforce the formerPublished*rules (publishedBy/url/doipresence, stricterid/urlpatterns,check_filesbytes,digest_sha256check).dandi-archive injects
datePublishedbefore validating, so the gated checks fire exactly as thePublished*classes did.Per-field validation errors. The publication validator accumulates all violations and raises them via
pydantic_core.ValidationError.from_exception_data(...), so each error carries its own fieldloc(e.g.('assetsSummary',)) plus the usual"Value error, …"message — rather than one model-level error with an emptyloc.Keep
BareAsset(Assetstill inherits it) withschemaKey: Literal["BareAsset"];Assetoverrides it toLiteral["Asset"](a localized# type: ignore[assignment]covers the narrowed override), andensure_schemakeypins each instance's value to its class name.Remove the
Publishablemixin; addPublishedDandiset/PublishedAssetas deprecated aliases.Simplify
ensure_schemakeyto a plainschemaKey == class-namecheck.to_dataciteasserts the published precondition (datePublishedset) thePublishedDandisettype used to guarantee.What changed outside
dandischema/models.pydandischema/consts.py:DANDI_SCHEMA_VERSIONbecomes0.8.0, and"0.7.0"joinsALLOWED_INPUT_SCHEMASso that metadata stamped with it remains valid input formigrate().The
tools/api-with-schema.Dockerfilechange that thetest-dandi-cliresults below depend on (addingENV UV_NO_SYNC=1and dropping the undeclareddevextra) was split out into #435 and merged tomaster, so it is no longer part of this PR's diff; this branch inherits it after being rebased ontomaster.Why gate on
datePublishedrather than an explicit (context-triggered) validatorThe publish-only checks could instead be triggered explicitly by the caller (e.g. a Pydantic validation-context flag passed by the publish flow) rather than by the presence of
datePublished. We chose data-driven gating because:gen-json-schemaemits it asif/then). A caller-supplied validation mode has no LinkML equivalent — LinkML describes only the data instance, not how validation is invoked — so it would have to live as hand-written Python forever, outside the source of truth, working against the migration this change is meant to enable.validate()and direct construction (Dandiset(**data)/model_validate). A context flag only ridesmodel_validate(..., context=...): the tersePublishedDandiset(**meta)form (used byto_dataciteand in tests) cannot pass one, so it would silently skip the publish checks. Beyond the call-site rewrites that would force, a constructor whose enforcement quietly depends on how it is invoked is a lasting programming hazard.datePublishedpresent ⟺ published is a real invariant: a draft never carries it, and the archive injects it only when building a publishable version. "Published" is a state of the record, not a mode a caller opts into.obj.datePublished is not None; with a context flag, whether an object was validated as published is ephemeral to the call and recorded nowhere on the object.Compatibility
Dandiset/Assetgain only optional,readOnlypublication properties — nothing newly required, no pattern tightened — so the dandi-archive Meditor is unaffected (it hidesreadOnlyprops). Thepublished-*.jsonschemas become the relaxed versions, with publication strictness enforced by the gated validator.published-dandiset.jsonandpublished-asset.jsonare now byte-identical todandiset.jsonandasset.json, becausepublish_model_schematagenerates each file from the class named inSCHEMA_MAPandPublishedDandiset/PublishedAssetare now aliases ofDandiset/Asset.datePublishedand a flatrequiredlist cannot express that; the gated validator enforces them instead. That is an acceptable gap for now: the plan is to re-encode these requirements as LinkML rules in the LinkML translation, wheregen-json-schemaemits them asif/then.PublishedDandiset/PublishedAsset/BareAssetimports continue to work indandi-cli/dandi-archive.Follow-ups (separate PRs)
dandischemapin in dandi-archive and dandi-cli once this change is merged and released. Both pindandischema, andDANDI_SCHEMA_VERSIONmust match across the two services, so they need to move to the same release together. This is also what clears the expectedtest-dandi-clifailures described under Test plan.dandischemain dandi-cli has been implemented in Downgrade outgoing metadata to server's schema version on upload dandi-cli#1898dandischemain dandi-archive is being tracked with Bumpdandischemapin once thedandi-clirelease deadlock is broken dandi-archive#2876_encode_pydantic_errorrobustness (_encode_pydantic_errorraises IndexError on validation errors with emptylocdandi-archive#2851). This PR's per-field errors avoid the empty-loccase at the source, but that fix is still worth having as defense-in-depth.PublishedDandiset/PublishedAssetaliases #440Test plan
tox -e py3(262 passed, 17 skipped — skips are environment-only: noDOI_PREFIX/ instance name not "DANDI")tox -e lint,typing0.7.0release indandi/schema(see the comment linked from the Summary)datePublished-gated publish requirements and the coherence invariantdandischemato this branch. After this PR was rebased ontomaster(following the TESTS: Fix dandi cli testing #435 split), that pin was re-resolved to the current tip (227cdd01) and the run re-triggered; this box will be checked once the refreshed run passes. The earlier run, against the now-dropped pre-split tip, had the archive's publication-validation suite (backend-ci) passing against the consolidated models and bothcli-integrationvariants (consolidated-model server against a released-dandischemaclient, on dandi-climasterandrelease) passing.Expected
test-dandi-clifailuresBumping
DANDI_SCHEMA_VERSIONahead of dandi-archive and dandi-cli leaves integration jobs red: three of them today, and two once dandi/dandi-cli#1894 merges, since theserverjob runs dandi-climasterand that PR drops the incidental requirement that both sides carry the sameDANDI_SCHEMA_VERSION. Each is version skew between the two sides of the test rather than a regression from the model changes, and none can be fixed from this PR; the remaining two clear when both services move to this release.Which jobs fail, and why
client, dandi-climasterclient, dandi-clireleaseserverboth(default and ember-dandi)clientclientjobs install this branch into the client only, so a0.8.0client meets the released0.7.0server. dandi-cli treats a server that is behind by a MINOR schema version as fatal (dandi/dandiapi.py:740-747), so nearly every server-backed test errors withSchemaVersionError: Server uses older incompatible schema version 0.7.0; client supports 0.8.0.serverjob is the mirror image: a released client against a0.8.0server. Only 5 of 1006 tests fail, allcli/tests/test_service_scripts.py::test_update_dandiset_from_doi, because that test builds its expectedschemaVersionand@contextfrom the client's ownDANDI_SCHEMA_VERSION(test_service_scripts.py:111-118) and so expects0.7.0where the server emits0.8.0. Build expected metadata with the server's DANDI schema version dandi-cli#1894 fixes that expectation, and since this job runs dandi-climaster, it goes green on a re-run once that PR merges.bothjobs install this branch on both sides and pass, which is the signal that the consolidated models and the new schema version work end to end.Before the
UV_NO_SYNC=1fix (added in #435, now inmasterand inherited here via rebase), thebothjobs failed exactly like theclientjobs, and theserverjob passed only because the ineffective override left both sides agreeing on0.7.0. Its red status here is a consequence of the override finally working.