Repository navigation
Make name required and non-empty for Contributor and BaseType - #444
yarikoptic-gitmate wants to merge 4 commits into
Conversation
…meless items Scans public S3 manifests (dandiset.jsonld and optionally assets.jsonld) of a DANDI instance for nested objects whose class has an optional `name` (Contributor/Organization, BaseType subclasses, Resource, RelatedParticipant) but lack one, and cross-checks with current validation to identify versions which would turn from valid to invalid if `name` became required (see #442). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NdN9xPzPo1AzsnEMuU8UQs
`Contributor.name` (thus also `Organization.name`) and `BaseType.name` (thus `Anatomy`, `Disorder`, `GenericType`, `SpeciesType`, ... names) were Optional, so items without any human-readable label validated and were rendered as blank cards on the Dandiset landing page (dandi/dandi-archive#2936). Make them required with `min_length=1`. This also makes the Contributor hierarchy homogeneous for the LinkML migration: `Person` no longer needs to refine `name` from optional to required (#389/#405), only to add its "Last, First" pattern. `Resource.name` stays optional: >61k asset-level and 34 dandiset-level published records rely on identifier/url only (enforced by the existing `identifier_or_url` validator). Bump DANDI_SCHEMA_VERSION to 0.8.1 (0.8.0 is already released) and keep 0.8.0 as an allowed input and (lossless) migration target. The audit script defaults now to all classes with a `name` field, since the tightened ones no longer have it optional. Closes #442 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NdN9xPzPo1AzsnEMuU8UQs
Nested objects of any class whose fields other than `schemaKey` are all absent, null, empty, or equal to their defaults are reported as `empty_record` (disable with --no-empty-records). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NdN9xPzPo1AzsnEMuU8UQs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #444 +/- ##
==========================================
+ Coverage 47.96% 48.32% +0.36%
==========================================
Files 19 19
Lines 2427 2444 +17
==========================================
+ Hits 1164 1181 +17
Misses 1263 1263
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:
|
candleindark
left a comment
There was a problem hiding this comment.
The schema change itself looks good to me. The notes below are about downstream code that interacts with it. I also left some inline comments on the audit script.
1. dandi-cli constructs a nameless SexType
extract_sex constructs a SexType with name being None when the NWB subject.sex value is a URL, which is no longer valid with this PR.
Details
No dandi-cli test exercises this branch, which is why test-dandi-cli is green here. dandi-cli also pins dandischema < 0.15.0, so that pin needs a bump if this is released as 0.15.0.
2. Draft assets already marked VALID are not re-validated
After dandi-archive bumps its dandischema pin, draft assets validated under the old schema keep their VALID status. I'd suggest that dandi/dandi-archive#2024 (re-validating drafts upon a dandischema upgrade) cover draft assets too. For this PR: which Dandiset's assets.jsonld was skipped by the audit, and could it be scanned, together with the draft assets?
Details
Asset validation only runs for PENDING assets, so nothing re-validates an asset that is already VALID. The audit found no nameless items in published asset metadata, except that one assets.jsonld over 100 MB was skipped. Draft assets were not scanned at all, so we don't know whether any draft asset currently marked VALID would fail under 0.8.1. For non-embargoed Dandisets, the draft assets.jsonld manifests are on S3, so --include-draft --assets should be able to answer that.
3. aggregate_assets_summary can fail during publishing and leave the draft in PUBLISHING
publish_dandiset_task runs _publish_dandiset, which calls aggregate_assets_summary over all assets of the new version, none of which are re-validated at that point. A single nameless approach, measurementTechnique, species, or dataStandard item among them makes it raise, and the draft is left in PUBLISHING. Given the audit, I don't expect this PR to hit it (pending point 2), but the same applies to any future tightening of models whose instances are copied from asset metadata into AssetsSummary.
Why, and a reproduction
- The assets of the new version are the published assets carried over from earlier versions plus the draft's assets.
aggregate_assets_summarycopies the items above from their stored metadata and builds amodels.AssetsSummarywith the current models. publish_dandisetsetsPUBLISHING(via_lock_dandiset_for_publishing) in its own transaction and queues the task withtransaction.on_commit(...), so the task is queued only after thePUBLISHINGstatus is committed. If the task fails, its own transaction is rolled back, but the committedPUBLISHINGstatus is not.- The task has no error handling, so no error is shown to the user, and retrying returns "already being published" until the draft is modified.
I verified this with a test in dandi-archive (at 909cbe9b), both with its current dandischema==0.12.1 (using an ApproachType name over 150 characters, which fails at the same place) and with this PR's head (using a nameless ApproachType). For an unpublished asset marked VALID as well as for a carried-over published asset, publishing raises a ValidationError for AssetsSummary, no new version is created, the draft stays Publishing, a retry raises DandisetAlreadyPublishingError, and a metadata edit resets the draft to Pending.
Reproduction test (copy into dandiapi/api/tests/)
"""
Reproduce: a failure in `aggregate_assets_summary` during publishing leaves the draft in
`PUBLISHING` (dandi/dandi-schema#444 review).
Copy into `dandi-archive/dandiapi/api/tests/` and run with pytest. The trigger adapts to the
installed dandischema: a nameless `ApproachType` if `name` is required (dandi-schema#444), else a
151-character name, which violates the existing `max_length=150` and fails at the same place
(`models.AssetsSummary(**stats)`).
"""
from __future__ import annotations
from dandischema import models as schema_models
import pydantic
import pytest
from dandiapi.api.models import Asset, Version
from dandiapi.api.services.publish import publish_dandiset
from dandiapi.api.services.publish.exceptions import DandisetAlreadyPublishingError
from dandiapi.api.tests.factories import DraftVersionFactory, UserFactory
if schema_models.ApproachType.model_fields['name'].is_required():
BAD_APPROACH = {'schemaKey': 'ApproachType'}
else:
BAD_APPROACH = {'schemaKey': 'ApproachType', 'name': 'x' * 151}
@pytest.mark.django_db
@pytest.mark.parametrize('asset_kind', ['draft_marked_valid', 'published'])
def test_failed_summary_leaves_draft_publishing(
api_client,
draft_asset_factory,
published_asset_factory,
django_capture_on_commit_callbacks,
asset_kind,
):
user = UserFactory.create()
draft_version: Version = DraftVersionFactory.create(
status=Version.Status.VALID, dandiset__owners=[user]
)
dandiset = draft_version.dandiset
# An asset that was marked VALID earlier (or was published earlier) and is not re-validated
if asset_kind == 'published':
asset: Asset = published_asset_factory()
else:
asset = draft_asset_factory(status=Asset.Status.VALID)
Asset.objects.filter(id=asset.id).update(metadata={**asset.metadata, 'approach': [BAD_APPROACH]})
draft_version.assets.add(asset)
assert draft_version.publishable
# Publish through the service used by the API: lock, then run the task on commit (eager)
with (
pytest.raises(pydantic.ValidationError) as exc_info,
django_capture_on_commit_callbacks(execute=True),
):
publish_dandiset(user=user, dandiset=dandiset)
assert exc_info.value.title == 'AssetsSummary'
print(f'\n[{asset_kind}] task raised: {exc_info.value!r}')
# The publish transaction was rolled back ...
assert dandiset.versions.count() == 1
asset.refresh_from_db()
assert asset.published == (asset_kind == 'published')
# ... but the draft is left in PUBLISHING
draft_version.refresh_from_db()
assert draft_version.status == Version.Status.PUBLISHING
print(f'[{asset_kind}] draft status after failure: {draft_version.status}')
# Retrying is refused
with pytest.raises(DandisetAlreadyPublishingError):
publish_dandiset(user=user, dandiset=dandiset)
# Any metadata edit resets the status to PENDING (no PUBLISHING guard)
api_client.force_authenticate(user=user)
resp = api_client.put(
f'/api/dandisets/{dandiset.identifier}/versions/draft/',
{'metadata': {**draft_version.metadata, 'description': 'edited'}, 'name': 'edited'},
format='json',
)
assert resp.status_code == 200, resp.data
draft_version.refresh_from_db()
assert draft_version.status == Version.Status.PENDING
print(f'[{asset_kind}] draft status after a metadata edit: {draft_version.status}')To run it with this PR's dandischema: uv run --extra development --group test --with "dandischema @ git+https://github.com/dandi/dandi-schema@5cd2a79b4b902c2482901015debdd28f20578827" pytest dandiapi/api/tests/test_pr444_publish_stuck.py -s
…baseline Address review of #444: - The verdict no longer comes from the `name`/`empty_record` heuristics, which counted empty records (not rejected by #444) as breaking and `name: ""` (rejected by `min_length=1`) as not breaking. Instead every Dandiset record (migrated) and, with --assets, every asset record is validated with the installed dandischema, and --baseline compares with the output of a run made with the dandischema preceding the change: BREAKS if a record valid in the baseline is invalid now, NEW_ERRORS if an already invalid record gets new errors, UNCHANGED otherwise. The scan results remain as pointers to the offending items. - Check only the classes #444 tightens (BaseType and Contributor subclasses) for missing names by default; others via --schema-key. - Cache manifests gzip-compressed, so that draft assets.jsonld fit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NdN9xPzPo1AzsnEMuU8UQs
|
Thanks @candleindark. The inline comments are addressed in 45994e1. On the review points: 2. Draft assets and the skipped manifest. I reran the reworked script (verdict now decided by validation, see the inline replies) over every manifest on S3. It ran twice, once with
1. dandi-cli
3. Generated by Claude Code |
Closes #442. Related: dandi/dandi-archive#2936, dandi/dandi-archive#2942, dandi/dandi-archive#2944, #389, #405.
Problem
Contributor.name(and soOrganization.name) andBaseType.name(and soAnatomy,Disorder,GenericType,SpeciesType,StandardsType, …) wereOptional. This let items with no human-readable label, e.g.{"schemaKey": "Organization"}or{"schemaKey": "GenericType"}, passvalidation, and the landing page rendered them as blank cards.
These classes were also inconsistent with each other for the LinkML
migration.
Persontightenednamefrom optional to required in a subclass,which LinkML only accepts as a
requiredrefinement inslot_usage(#405, pydantic2linkml#66). With
namerequired onContributor,Persononly adds its "Last, First" pattern.
Changes
Contributor.name: str(min_length=1).OrganizationandPersoninherit it.Person.namewas already required and keeps its pattern.BaseType.name: str(min_length=1,max_length=150), covering every subject-matter / type class.min_length=1rejects""but not whitespace-only names. The audit found no empty or blank names anywhere, so no pattern was added.Resource.nameandRelatedParticipant.namestay optional. See the audit below.DANDI_SCHEMA_VERSION0.8.0 → 0.8.1, because 0.8.0 is already released asschema-0.8.0. 0.8.0 is kept as an allowed input and as a migration target; downgrading is lossless.TestContributor.test_name_required(Contributor, Organization, Person) andTestBaseTypeName(everyBaseTypesubclass). ExistingContributortests now pass aname.tools/audit_nameless_items.py: an audit of the public S3 manifests (see below). It validates every Dandiset and asset record with the installed dandischema and, given--baseline(the output of a run with the preceding dandischema), reports which records turn from valid to invalid. Nested items with a missing or blankname, or with nothing butschemaKey, are listed as pointers.Impact on existing metadata (audit of all public S3 manifests, 2026-09-29)
tools/audit_nameless_items.pywas run over everydandisets/*/<version>/dandiset.jsonld(629 published versions, 963 drafts present on S3) and over the
assets.jsonldof 628published versions (248,614 assets; one file over 100 MB was skipped).
contributorwasGeneratedBy[].wasAssociatedWith[])related_publications)name(any class)Rerun on 2026-10-06 with validation-based verdicts (
mastervs this PR,--include-draft --assets --max-assets-mb 0):852,353 asset records were validated (346,906 published, 505,447 in drafts on S3), including the 148 MB
manifest of 000571/0.260515.0242 skipped above. No asset record turns invalid. The only records that
do are the Dandiset records below (published) and the drafts of 000631, 001262, 001341 and 001755.
279 drafts have no manifest on S3 (embargoed or not yet written) and were not covered.
Published versions that validate with 0.8.0 and would not validate with 0.8.1:
namewasGeneratedBy[0].wasAssociatedWith[0](Project "Targeted Neuromodulation by Nanosecond Pulsed Electric Fields", idNIH 1R21EY034258){"schemaKey": "Organization", "roleName": [], "contactPoint": [], "includeInCitation": false}"National Institutes of Health": the same award number is on the NIH Funder contributor. Removing the empty item is an alternative.about[1]{"schemaKey": "Anatomy", "identifier": "UBERON:0004727"}"cochlear nerve"(UBERON label)about[0]{"schemaKey": "GenericType"}The drafts of 000631, 001262 and 001341 contain the same items. The 001755
draft has
about[0] = {"schemaKey": "GenericType", "identifier": "pubmed:39491780"},which should become a
relatedResourceor get a name.Published versions keep their original
schemaVersionand are not changed bythis PR. The table is for the archive admins, if they want to patch them.
Considered and not done here: a generic "nothing but
schemaKey" ruleThe audit script also flags nested records of any class whose other fields
are all absent, empty or at their defaults (
empty_record). Across allmanifests this adds only one class beyond those already covered by the
namerequirement:
{"schemaKey": "ContactPoint"}. It appears inethicsApproval[].contactPoint,access[].contactPointandOrganization.contactPoint[]: 93 published versions in 59 Dandisets, and 79drafts. New ones keep appearing every year from 2021 to 2026, so tooling
(likely the metadata editor) produces them, not users. No asset-level record
is empty.
A generic invalidating validator was left out for these reasons:
model_validatorthat LinkML can't translate (Handle LinkML migration issue ofpydantic2linkml: Unable to translate the logic contained in the after validation function#391).Better options: drop such records, which loses no information (in the
archive on save, or with a dandischema helper), and fix whatever emits them.
That is filed as dandi/dandi-archive#2944. Our published JSON Schema
contributes:
TransitionalGenerateJsonSchemadropsnullfromOptionalfields, so
contactPoint: nulland{}are both rejected, and{"schemaKey": "ContactPoint"}is the smallest valid value. Advisory(non-fatal) reporting of such records is proposed separately as
dandischema.lint.Follow-ups (not in this PR)
nameas required, and drafts with nameless items will show validation errors and cannot be published until fixed.extract_sex(sex given as an IRI) andextract_species(NCBITaxon IRI whose label lookup fails) build namelessSexType/SpeciesType. Fixed, together with thedandischema < 0.15.0pin bump, in Give SexType/SpeciesType built from IRIs a name; allow dandischema 0.15.x dandi-cli#1949, which should be merged before this is released.aggregate_assets_summaryfail during publishing and leaves the draft inPUBLISHING(A failure in the publish task leaves the draft stuck inPUBLISHINGdandi-archive#2966). Therevalidatecommand can't re-validate drafts/assets already markedVALIDafter a dandischema upgrade (revalidatemanagement command can't re-validate assets or draft versions already marked VALID dandi-archive#2965, Do re-validate dandisets upon dandischema upgrade dandi-archive#2024).Contributorgetsnamerequired: true, andPerson'sslot_usage.namedrops therequiredrefinement.🤖 Generated with Claude Code
https://claude.ai/code/session_01NdN9xPzPo1AzsnEMuU8UQs