Give a descriptive error when a DOI cannot be resolved to citation metadata

[adityasingh2400]

Fixes #1855

update-dandiset-from-doi crashed with a bare json.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 RESTFullAPIClient session, but RESTFullAPIClient.request() sets accept: application/json on the request whenever json_resp is 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 carries accept: application/json and never the CSL type. Crossref happens to serve JSON for that anyway, because it redirects to api.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 DataCite 10.48324 DOI.

The second is that there was no error handling at all on that path, so the user got a JSONDecodeError raised 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 a click.ClickException naming the DOI, the URL, and the content type actually received. normalize_doi() now accepts 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 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, and author[*].given/family/ORCID/affiliation are 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.JSONDecodeError from dandiapi.py line 326. After, it reports that the DOI answered with text/html instead of CSL JSON and explains that the registration agency likely does not serve citation metadata. The Accept header actually sent went from application/json to application/vnd.citationstyles.csl+json; charset=utf-8.

New tests are marked @pytest.mark.ai_generated. They give 18 passed. The 6 deselected are the VCR test_update_dandiset_from_doi cases, 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_generated as CLAUDE.md asks. I reviewed and tested everything before submitting.

[codecov]

Codecov Report

❌ Patch coverage is 95.62044% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.52%. Comparing base (2b5f8ea) to head (d15b9c7).
⚠️ Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
dandi/cli/cmd_service_scripts.py	89.09%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
+ Coverage   76.96%   77.52%   +0.56%     
==========================================
  Files          88       89       +1     
  Lines       12882    13457     +575     
==========================================
+ Hits         9914    10433     +519     
- Misses       2968     3024      +56     

Flag	Coverage Δ
unittests	77.52% <95.62%> (+0.56%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yarikoptic-gitmate]

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:

1. doidata["author"] / doidata["title"] can still raise raw KeyError tracebacks for registered DOIs whose CSL record omits those keys — the failure class this PR targets (most severe; anchored at the fetch_doi_citation_metadata() call since the indexing sites are outside the diff).
2. normalize_doi() keeps ?query/#fragment from pasted resolver URLs as part of the DOI, which can persist a corrupted identifier into Dandiset metadata.
3. 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.
4. DOI_REGEX rejects valid legacy sub-registrant DOIs (10.1000.10/123) that the old code accepted.

---

Generated by Claude Code

[yarikoptic-gitmate]

Since \S+ matches ? and #, a resolver URL pasted straight from a browser — e.g. https://doi.org/10.1234/foo?locatt=mode:legacy — normalizes to 10.1234/foo?locatt=mode:legacy. On a successful lookup that corrupted identifier is then persisted into the Dandiset's relatedResource metadata (and a #fragment variant silently looks up a different DOI than the one stored, since requests drops the fragment before sending).

Stripping the query string/fragment only in the URL spellings keeps bare DOIs containing ? (technically legal, if rare) working:

Suggested change

	for prefix_regex in DOI_PREFIX_REGEXES:
	if m := re.match(prefix_regex, value, flags=re.I):
	value = value[m.end() :].strip()
	break
	for prefix_regex in DOI_PREFIX_REGEXES:
	if m := re.match(prefix_regex, value, flags=re.I):
	# a resolver URL pasted from a browser may carry a query string
	# or fragment that is not part of the DOI itself
	value = re.split(r"[?#]", value[m.end() :].strip(), maxsplit=1)[0]
	break

---

Generated by Claude Code

[adityasingh2400]

Done, with one deliberate deviation. Your prose says to strip only in the URL spellings, but the suggestion loop strips for every entry in DOI_PREFIX_REGEXES, which includes doi:. That would take doi:10.1234/foo?bar down to 10.1234/foo, which is the case your prose sets out to protect.

So I gated it on the URL prefix specifically:

for prefix_regex in DOI_PREFIX_REGEXES:
    if m := re.match(prefix_regex, value, flags=re.I):
        value = value[m.end() :].strip()
        if prefix_regex is DOI_URL_PREFIX_REGEX:
            value = re.split(r"[?#]", value, maxsplit=1)[0]
        break

Covered by test_normalize_doi_prefix_and_url_suffix, which asserts both directions: https://doi.org/10.1234/foo?locatt=mode:legacy becomes 10.1234/foo, while 10.1234/foo?bar and doi:10.1234/foo#bar keep their suffix.

[yarikoptic-gitmate]

fetch_doi_citation_metadata() only validates that the record is a dict, but downstream this command still hard-indexes doidata["author"] (line 387) and doidata["title"] (line 442). A DOI that resolves to valid CSL JSON without one of those keys — real for some Crossref record types such as editorials/corrections, or records with only organizational creators — still crashes with a raw KeyError traceback, the exact failure class this PR sets out to eliminate. (name/description are safe: copy_str_from_doi_to_metadata() uses .get().)

Validating up front, only for the fields actually being updated, keeps the descriptive-error guarantee:

Suggested change

	doidata = fetch_doi_citation_metadata(doi)
	doidata = fetch_doi_citation_metadata(doi)
	for field, key in (("contributor", "author"), ("relatedResource", "title")):
	if field in fields and key not in doidata:
	raise click.ClickException(
	f"Citation metadata for DOI {doi} has no {key!r} field, which "
	f"is needed to update {field!r}."
	)

---

Generated by Claude Code

[adityasingh2400]

Done. Same logic, lifted into a named helper so it can be tested directly rather than only through the command:

DOI_REQUIRED_KEYS = {"contributor": "author", "relatedResource": "title"}

def check_doi_fields(doi, doidata, fields):
    missing = {
        key: field
        for field, key in DOI_REQUIRED_KEYS.items()
        if field in fields and key not in doidata
    }
    ...

called right after fetch_doi_citation_metadata(doi). Only the requested --fields are checked, so --fields name,description still works on a record with no author or title, which copy_str_from_doi_to_metadata() handles with .get() anyway.

test_check_doi_fields_missing and test_check_doi_fields_ok cover both directions.

[yarikoptic]

@adityasingh2400 thanks for the PR. did you use that service script in battlefield for some dandisets?

[yarikoptic]

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?

[adityasingh2400]

Thanks both. All four review points are addressed in fee1ed1.

1. DOI_REGEX now allows registrant subdivision, 10\.\d{4,9}(?:\.\d+)*/\S+, so 10.1000.10/123 is accepted again.
2. Query strings and fragments are stripped only from the resolver-URL spelling, so https://doi.org/10.1234/foo?locatt=mode:legacy normalizes to 10.1234/foo while a bare 10.1234/foo?bar keeps its ?.
3. A 404 is only reported as unregistered when it came from doi.org itself. If doi.org redirected first and the agency 404s, the message now says the DOI is registered but has no citation metadata, and names the final URL.
4. New check_doi_fields() validates only the CSL keys the requested --fields will index, author for contributor and title for relatedResource, so a record without them fails with a message instead of a KeyError.

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.