Give a descriptive error when a DOI cannot be resolved to citation metadata - #1900
Conversation
…n metadata `dandi service-scripts update-dandiset-from-doi` crashed with a bare `json.JSONDecodeError: Expecting value: line 1 column 1 (char 0)` traceback whenever doi.org answered with anything other than JSON. Two things were wrong. The CSL Accept header was set on the `RESTFullAPIClient` session, but `RESTFullAPIClient.request()` sets `accept: application/json` on the request itself whenever `json_resp` is true, and per-request headers win over session headers. The resolver therefore never saw the citation format we meant to ask for. The checked-in VCR cassettes record this: every captured request carries `accept: application/json`. Crossref happens to serve JSON for that, which is why the existing tests pass, but a resolver that does not will redirect to the landing page and return HTML with a 200. And when that happened there was no error handling at all, so the user got a `JSONDecodeError` out of the requests internals with nothing pointing at the DOI. The fetch now moves into `fetch_doi_citation_metadata()`, which requests the raw response so the intended CSL Accept header survives, and turns a 404, another HTTP error, a non-JSON body, and a non-object body each into a `click.ClickException` naming the DOI, the URL, and the content type received. `normalize_doi()` additionally accepts the DOI as a bare DOI, a `doi:` URI, or a resolver URL, and rejects anything else with a `click.UsageError` instead of a traceback. That also fixes the `relatedResource` record, whose url was built as `https://doi.org/{doi}` and so came out doubled when the user passed a resolver URL. The lookup now happens before connecting to the archive, so a bad DOI fails fast without needing credentials. Closes dandi#1855
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1900 +/- ##
==========================================
+ Coverage 77.34% 77.65% +0.30%
==========================================
Files 89 89
Lines 13325 13549 +224
==========================================
+ Hits 10306 10521 +215
- Misses 3019 3028 +9
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:
|
yarikoptic-gitmate
left a comment
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Reviewed the diff — the core fix is sound (json_resp=False correctly stops the client from overriding the CSL Accept header, except-clause ordering is right, the 18 new tests pass locally, and flake8 is clean). Left 4 inline comments with suggestions:
doidata["author"]/doidata["title"]can still raise rawKeyErrortracebacks for registered DOIs whose CSL record omits those keys — the failure class this PR targets (most severe; anchored at thefetch_doi_citation_metadata()call since the indexing sites are outside the diff).normalize_doi()keeps?query/#fragmentfrom pasted resolver URLs as part of the DOI, which can persist a corrupted identifier into Dandiset metadata.- The 404 message misdiagnoses RA-endpoint 404s ("not registered — check for typos") for DOIs that are registered but whose agency doesn't serve CSL metadata.
DOI_REGEXrejects valid legacy sub-registrant DOIs (10.1000.10/123) that the old code accepted.
Generated by Claude Code
|
@adityasingh2400 thanks for the PR. did you use that service script in battlefield for some dandisets? |
|
note that we have an interactive AI-assisted (for better or for worse) helper to provide extended metadata entry/curation assistance 
so it could fetch even more than what DOI provides or can provide in principle. Did you try it out? |
…ssing CSL keys Claude-Session: https://claude.ai/code/session_0189zxefNUxZmXLRED6jPeMy
|
Thanks both. All four review points are addressed in
Five tests cover these. Against the pre-review source 7 of the new cases fail, including the 404 one asserting the old wording, and the 2 that pass are the bare-DOI cases that were already correct. Full file is 35 passed, 6 skipped. @yarikoptic on your two questions, honestly: no to both. I have not run this against a real Dandiset, only against mocked resolver responses and the live DOI resolver for the happy path, and I have not tried the interactive metadata helper. So I cannot claim this is better than what that helper already gets you. If the helper covers this ground, the useful part of this PR is probably just the error handling rather than the feature, and I am happy for it to be scoped down or closed. Still a draft. Say the word and I will mark it ready. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
@yarikoptic on your two questions, honestly: no to both. I have not run this against a real Dandiset, only against mocked resolver responses and the live DOI resolver for the happy path, and I have not tried the interactive metadata helper.
ok, I have tried on original to https://github.com//issues/1855 case and it was still failing on master version and passed fine here
❯ DANDI_DEVEL=1 dandi service-scripts update-dandiset-from-doi -d 216023 -e overwrite -F all -i dandi-sandbox https://doi.org/10.48324/dandi.001827/0.260505.1322
2026-09-14 17:02:31,797 [ WARNING] A newer version (0.78.0) of dandi/dandi-cli is available. You are using 0.76.8+8.gd15b9c76
2026-09-14 17:02:33,521 [ INFO] Adding new value to Dandiset contributor field:
affiliation: []
includeInCitation: true
name: Gutekundt, Claire-Anne
roleName:
- dcite:Author
schemaKey: Person
2026-09-14 17:02:33,521 [ INFO] Setting Dandiset name to ': Direct Delivery of Modulated Kilohertz Frequency Waveforms Enable Simultaneous Electrical Stimulation and Recording with Minimal-Artifact'
2026-09-14 17:02:33,521 [ INFO] Setting Dandiset description to 'This dataset contains the raw in vivo data for the manuscript in the title. Matlab code to reproduce the manuscript figures is available, contact authors to access the scripts.'
2026-09-14 17:02:33,523 [ INFO] Adding new value to Dandiset relatedResource field:
identifier: 10.48324/dandi.001827/0.260505.1322
name: ': Direct Delivery of Modulated Kilohertz Frequency Waveforms Enable Simultaneous
Electrical Stimulation and Recording with Minimal-Artifact'
relation: dcite:IsDescribedBy
schemaKey: Resource
url: https://doi.org/10.48324/dandi.001827/0.260505.1322
Show diff from old metadata to new? [Y/n] Y
--- dandiset.yaml:old
+++ dandiset.yaml:new
@@ -17,17 +17,47 @@
roleName:
- dcite:ContactPerson
schemaKey: Person
+- affiliation: []
+ includeInCitation: true
+ name: Gutekundt, Claire-Anne
+ roleName:
+ - dcite:Author
+ schemaKey: Person
dateCreated: '2025-01-20T23:27:26.240363+00:00'
-description: 'test '
+description: This dataset contains the raw in vivo data for the manuscript in
+ the title. Matlab code to reproduce the manuscript figures is available,
+ contact authors to access the scripts.
id: DANDI-SANDBOX:216023/draft
identifier: DANDI-SANDBOX:216023
license:
- spdx:CC0-1.0
manifestLocation:
- https://api.sandbox.dandiarchive.org/api/dandisets/216023/versions/draft/assets/
-name: test
+name: ': Direct Delivery of Modulated Kilohertz Frequency Waveforms Enable Simultaneous
+ Electrical Stimulation and Recording with Minimal-Artifact'
+relatedResource:
+- identifier: 10.48324/dandi.001827/0.260505.1322
+ name: ': Direct Delivery of Modulated Kilohertz Frequency Waveforms Enable Simultaneous
+ Electrical Stimulation and Recording with Minimal-Artifact'
+ relation: dcite:IsDescribedBy
+ schemaKey: Resource
+ url: https://doi.org/10.48324/dandi.001827/0.260505.1322
repository: https://sandbox.dandiarchive.org
schemaKey: Dandiset
schemaVersion: 0.7.0
url: https://sandbox.dandiarchive.org/dandiset/216023/draft
version: draft
+wasGeneratedBy:
+- description: Metadata (contributor, name, description, relatedResource) was
+ enhanced with data from DOI 10.48324/dandi.001827/0.260505.1322 by DANDI cli
+ endDate: '2026-09-14 17:02:33.524033-04:00'
+ id: urn:uuid:1ae5c80c-f697-416c-8fa6-216c00a5f1df
+ name: Metadata extraction from DOI
+ schemaKey: Activity
+ startDate: '2026-09-14 17:02:31.800287-04:00'
+ wasAssociatedWith:
+ - identifier: RRID:SCR_019009
+ name: DANDI Command Line Interface
+ schemaKey: Software
+ url: https://github.com/dandi/dandi-cli
+ version: 0.76.8+8.gd15b9c76
Save modified Dandiset metadata? [Y/n] Y
2026-09-14 17:03:12,752 [ INFO] Saving ...
❯ DANDI_DEVEL=1 dandi service-scripts update-dandiset-from-doi -d 216023 -e overwrite -F all -i dandi-sandbox https://doi.org/10.48324/dandi.001827/0.260505.1322
2026-09-14 17:03:18,388 [ WARNING] A newer version (0.78.0) of dandi/dandi-cli is available. You are using 0.76.8+8.gd15b9c76
2026-09-14 17:03:20,280 [ INFO] Dandiset contributor field already up to date
2026-09-14 17:03:20,280 [ INFO] Dandiset name already up to date
2026-09-14 17:03:20,280 [ INFO] Dandiset description already up to date
2026-09-14 17:03:20,284 [ INFO] Dandiset relatedResource field already up to date
2026-09-14 17:03:20,284 [ INFO] No changes to Dandiset metadata
so let's not make the perfect to be the enemy of the good and proceed! If you open it up for review, we could merge AFAIK. It could be improved but since it is just a service script, I think the chances that it even gets used again are slim anyways, so likely not worth much more investsment and it does address prior BUG. Thank you @adityasingh2400 !
| #: The prefix may be subdivided by a registrant (``10.1000.10/123``), which the | ||
| #: DOI Handbook allows, so the leading number is followed by zero or more | ||
| #: ``.``-separated groups. | ||
| DOI_REGEX = re.compile(r"10\.\d{4,9}(?:\.\d+)*/\S+") |
There was a problem hiding this comment.
here, should use one from DOI_PREFIX_PATTERN etc defined in dandi-schema
https://github.com/dandi/dandi-schema/blob/master/dandischema/models.py#L72
as the instance could be "vendored" and we might change / adjust how DOIs look so knowledge should be 'centralized'
There was a problem hiding this comment.
Done in a69127e. DOI_REGEX is now built on UNVENDORED_DOI_PREFIX_PATTERN from dandischema.conf, so what a DOI prefix looks like is no longer defined here. That constant exists in every dandischema version the pin allows (0.12.0, 0.13.0 and 0.14.0 all define it).
I used the general pattern rather than the instance-specific DOI_PREFIX_PATTERN from dandischema.models on purpose. On a vendored instance that one is the escaped instance prefix, and this command is normally given a publication's DOI, which relatedResource records as dcite:IsDescribedBy, so matching only the instance's own prefix would reject the main input. The registrant subdivision case from the earlier review (10.1000.10/123) is still accepted, since the suffix (?:\.\d+)* is composed on top of the dandischema prefix. One small effect of following dandischema: the prefix no longer has an upper bound of nine digits, which matches how dandischema itself validates a DOI prefix.
pytest dandi/cli/tests/test_service_scripts.py: 38 passed, 6 skipped. flake8, codespell, black, isort and mypy are clean on both files.
| if not DOI_REGEX.fullmatch(value): | ||
| raise ValueError( | ||
| f"{doi!r} does not look like a DOI. Expected something like " | ||
| "'10.48324/dandi.001827/0.260505.1322', optionally prefixed with " |
There was a problem hiding this comment.
here specific current instance .doi_prefix could be used in principle...
There was a problem hiding this comment.
Done in a69127e. The example in the "does not look like a DOI" message now comes from a small example_doi() helper that reads get_instance_config().doi_prefix. On a vendored instance with its own prefix the example is a DOI that instance could have minted, {doi_prefix}/{instance_name.lower()}.000001/0.240101.1234, and I checked that this shape matches DANDI_DOI_PATTERN when DANDI_DOI_PREFIX and friends are set in the environment. Without a configured prefix it falls back to the real DANDI DOI that was hardcoded before.
I left the static example in the --help docstring as is, since a docstring cannot follow the runtime configuration. Two tests cover the helper, one for each case, by patching get_instance_config in the module so the global dandischema configuration is not touched.
Build DOI_REGEX on dandischema's UNVENDORED_DOI_PREFIX_PATTERN instead of a local copy of what a DOI prefix looks like, so that knowledge lives in one place. The general pattern is used rather than the instance-specific DOI_PREFIX_PATTERN because the DOI given to this command is usually a publication's rather than one minted by the instance; registrant subdivision of the prefix is still allowed. Derive the example DOI shown in the "does not look like a DOI" message from the instance configuration: on a vendored instance with its own doi_prefix the example is one that instance could have issued, otherwise it is a real DANDI DOI. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@yarikoptic thank you for running this against the original #1855 case, that is the confirmation I could not give myself. Both inline points are addressed in a69127e: the DOI prefix pattern now comes from dandischema, and the example DOI in the error message follows the instance's |
4 participants
Fixes #1855
update-dandiset-from-doicrashed with a barejson.JSONDecodeError: Expecting value: line 1 column 1 (char 0)whenever doi.org answered with anything other than JSON.There are two separate problems behind that traceback.
The first is a header bug. The CSL Accept header was set on the
RESTFullAPIClientsession, butRESTFullAPIClient.request()setsaccept: application/jsonon the request wheneverjson_respis true, and in requests a per-request header wins over a session header. The resolver therefore never saw the citation format we meant to ask for. The checked-in VCR cassettes record this, every captured request carriesaccept: application/jsonand never the CSL type. Crossref happens to serve JSON for that anyway, because it redirects toapi.crossref.org/.../transform, which is why the existing tests pass. A registration agency that does not will redirect to the landing page and return HTML with a 200, which is the reported failure on a DataCite10.48324DOI.The second is that there was no error handling at all on that path, so the user got a
JSONDecodeErrorraised from inside requests with nothing naming the DOI.The fetch moves into
fetch_doi_citation_metadata(), which requests the raw response so the intended CSL Accept header survives, and turns a 404, any other HTTP error, a non-JSON body, and a non-object body each into aclick.ClickExceptionnaming the DOI, the URL, and the content type actually received.normalize_doi()now accepts a bare DOI, adoi:URI, or a resolver URL, and rejects anything else with aclick.UsageErrorinstead of a traceback. That also fixes therelatedResourcerecord, whose url was built ashttps://doi.org/{doi}and came out doubled when the user passed a resolver URL. The lookup now happens before connecting to the archive, so a bad DOI fails fast without needing credentials.Only
title,abstract, andauthor[*].given/family/ORCID/affiliationare read, and all of those are present in both CSL JSON and the Crossref record the cassettes captured, so replaying the existing cassettes is unaffected. vcrpy matches on method and URI, not headers.Verified against the base ref with doi.org mocked to serve HTML at 200. Before, the run ends in
requests.exceptions.JSONDecodeErrorfromdandiapi.pyline 326. After, it reports that the DOI answered withtext/htmlinstead of CSL JSON and explains that the registration agency likely does not serve citation metadata. The Accept header actually sent went fromapplication/jsontoapplication/vnd.citationstyles.csl+json; charset=utf-8.New tests are marked
@pytest.mark.ai_generated. They give 18 passed. The 6 deselected are the VCRtest_update_dandiset_from_doicases, which need the docker archive fixture and could not run locally.AI assistance disclosure: this change was written with the help of Claude Code, and the added tests are marked
ai_generatedas CLAUDE.md asks. I reviewed and tested everything before submitting.