diff --git a/dandi/cli/cmd_service_scripts.py b/dandi/cli/cmd_service_scripts.py index 8e55645c0..8329df23d 100644 --- a/dandi/cli/cmd_service_scripts.py +++ b/dandi/cli/cmd_service_scripts.py @@ -7,6 +7,7 @@ import json import os from pathlib import PurePosixPath +import re from textwrap import indent from typing import Any, TypeVar import urllib.parse @@ -16,7 +17,7 @@ from dandischema.consts import DANDI_SCHEMA_VERSION from packaging.version import Version from requests.auth import HTTPBasicAuth -from requests.exceptions import HTTPError +from requests.exceptions import HTTPError, RequestException from dandi.consts import known_instances @@ -24,7 +25,7 @@ from .. import __version__, lgr from ..dandiapi import DandiAPIClient, RemoteBlobAsset, RESTFullAPIClient from ..dandiarchive import parse_dandi_url -from ..exceptions import NotFoundError +from ..exceptions import HTTP404Error, NotFoundError from ..utils import yaml_dump T = TypeVar("T") @@ -35,6 +36,180 @@ "https://api.datacite.org/dois": "https://doi.datacite.org/dois", } +#: Base URL of the DOI resolver used to look up citation metadata +DOI_RESOLVER_URL = "https://doi.org/" + +#: Content type requested from the DOI resolver for citation metadata +DOI_CSL_ACCEPT = "application/vnd.citationstyles.csl+json; charset=utf-8" + +#: Matches a bare DOI, e.g. ``10.48324/dandi.001827/0.260505.1322``. +#: 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+") + +#: Prefixes a DOI may be spelled with, in the order they are stripped. The +#: second is a resolver URL, and only that spelling may carry a query string or +#: fragment that is part of the URL rather than of the DOI. +DOI_PREFIX_REGEXES = (r"doi:", r"(?:https?://)?(?:dx\.)?doi\.org/") +DOI_URL_PREFIX_REGEX = DOI_PREFIX_REGEXES[1] + + +def normalize_doi(doi: str) -> str: + """Reduce a DOI given in any of its usual spellings to the bare DOI. + + A bare DOI (``10.48324/dandi.001827/0.260505.1322``), a ``doi:`` URI, and a + resolver URL (``https://doi.org/...``, ``http://dx.doi.org/...``) are all + accepted and reduced to the bare form. + + Parameters + ---------- + doi : str + The DOI as given by the user + + Returns + ------- + str + The bare DOI + + Raises + ------ + ValueError + If `doi` is not a syntactically valid DOI in any accepted spelling + """ + value = doi.strip() + 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: + # A resolver URL copied from a browser can carry a query string + # or fragment (``...?locatt=mode:legacy``). Those belong to the + # URL, not to the DOI, and keeping them would store a corrupted + # identifier in the Dandiset metadata. Bare DOIs are left alone, + # since ``?`` and ``#`` are legal (if rare) DOI characters. + value = re.split(r"[?#]", value, maxsplit=1)[0] + break + 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 " + "'doi:' or 'https://doi.org/'." + ) + return value + + +def fetch_doi_citation_metadata(doi: str) -> dict[str, Any]: + """Fetch the CSL JSON citation metadata for a bare `doi` from doi.org. + + Parameters + ---------- + doi : str + A bare DOI, as returned by `normalize_doi()` + + Returns + ------- + dict + The parsed CSL JSON record + + Raises + ------ + click.ClickException + If the DOI cannot be resolved, or if the resolver answers with + something other than a CSL JSON object. The exception message + describes what went wrong, so that the user is not left with a bare + `json.JSONDecodeError` traceback. + """ + url = f"{DOI_RESOLVER_URL}{doi}" + with RESTFullAPIClient( + DOI_RESOLVER_URL, headers={"Accept": DOI_CSL_ACCEPT} + ) as doiclient: + try: + r = doiclient.get(doi, json_resp=False) + except HTTP404Error as e: + # doi.org 302-redirects a registered DOI to its registration + # agency's content-negotiation endpoint, which can itself 404 when + # the record is not served as CSL. Only a 404 that came back from + # doi.org itself means the DOI is unregistered. + final_url = e.response.url if e.response is not None else url + final_netloc = urllib.parse.urlparse(str(final_url)).netloc + if final_netloc == urllib.parse.urlparse(DOI_RESOLVER_URL).netloc: + raise click.ClickException( + f"DOI {doi} is not registered: {url} returned 404. Check " + "the DOI for typos and make sure it has already been " + "published." + ) + raise click.ClickException( + f"DOI {doi} is registered but no citation metadata is available " + f"for it: {url} redirected to {final_url}, which returned 404. " + "The registration agency may not serve CSL JSON for this record, " + "or the metadata may not have propagated yet." + ) + except HTTPError as e: + status = e.response.status_code if e.response is not None else "?" + raise click.ClickException( + f"Failed to look up DOI {doi}: {url} returned HTTP {status}." + ) + except RequestException as e: + raise click.ClickException(f"Failed to look up DOI {doi} at {url}: {e}") + content_type = r.headers.get("Content-Type", "") + try: + doidata = r.json() + except ValueError: + raise click.ClickException( + f"DOI {doi} did not resolve to citation metadata: {url} answered " + f"with {content_type!r} instead of CSL JSON (final URL: {r.url}). " + "This usually means the DOI's registration agency does not serve " + "citation metadata for it, and doi.org fell back to redirecting " + "to the landing page." + ) + if not isinstance(doidata, dict): + raise click.ClickException( + f"DOI {doi} resolved to a JSON {type(doidata).__name__} rather than " + f"the expected CSL JSON object (final URL: {r.url})." + ) + return doidata + + +#: CSL JSON keys this command indexes directly, by the field that needs them +DOI_REQUIRED_KEYS = {"contributor": "author", "relatedResource": "title"} + + +def check_doi_fields(doi: str, doidata: dict[str, Any], fields: set[str]) -> None: + """Fail early if `doidata` lacks a key the requested `fields` will index. + + `fetch_doi_citation_metadata()` only guarantees a CSL JSON object. Some + real records (editorials, corrections, records with only organizational + creators) omit ``author`` or ``title``, which would otherwise surface as a + bare `KeyError` traceback part-way through building the new metadata. + + Parameters + ---------- + doi : str + The bare DOI, used in the error message + doidata : dict + The CSL JSON record + fields : set[str] + The Dandiset metadata fields the user asked to update + + Raises + ------ + click.ClickException + If a requested field needs a CSL key the record does not have + """ + missing = { + key: field + for field, key in DOI_REQUIRED_KEYS.items() + if field in fields and key not in doidata + } + if missing: + details = ", ".join( + f"{key!r} (needed for {field})" for key, field in sorted(missing.items()) + ) + raise click.ClickException( + f"DOI {doi} resolved to citation metadata without {details}. Re-run " + "with --fields limited to the fields its record can supply." + ) + @click.group() def service_scripts() -> None: @@ -247,7 +422,15 @@ def update_dandiset_from_doi( """ Update the metadata for the draft version of a Dandiset with information from a given DOI record. + + DOI may be given bare (``10.48324/dandi.001827/0.260505.1322``), as a + ``doi:`` URI, or as a resolver URL (``https://doi.org/...``). """ + try: + doi = normalize_doi(doi) + except ValueError as e: + raise click.UsageError(str(e)) + known_instance_names = [k.upper() for k in known_instances.keys()] # Strip instance name prefix from dandiset ID, if present @@ -258,15 +441,11 @@ def update_dandiset_from_doi( break start_time = datetime.now().astimezone() + # Resolve the DOI before talking to the archive, so that a bad DOI fails + # fast and without requiring credentials + doidata = fetch_doi_citation_metadata(doi) + check_doi_fields(doi, doidata, fields) with DandiAPIClient.for_dandi_instance(dandi_instance, authenticate=True) as client: - with RESTFullAPIClient( - "https://doi.org/", - headers={ - "Accept": "application/vnd.citationstyles.csl+json; charset=utf-8" - }, - ) as doiclient: - doidata = doiclient.get(doi) - d = client.get_dandiset(dandiset, "draft", lazy=False) original_metadata = d.get_raw_metadata() new_metadata = deepcopy(original_metadata) diff --git a/dandi/cli/tests/test_service_scripts.py b/dandi/cli/tests/test_service_scripts.py index 10a0a8713..7c99ac3c2 100644 --- a/dandi/cli/tests/test_service_scripts.py +++ b/dandi/cli/tests/test_service_scripts.py @@ -8,14 +8,22 @@ import sys import anys +import click from click.testing import CliRunner from dandischema.models import ID_PATTERN import pytest +import responses from dandi import __version__ from dandi.tests.fixtures import SampleDandiset -from ..cmd_service_scripts import service_scripts +from ..cmd_service_scripts import ( + DOI_CSL_ACCEPT, + check_doi_fields, + fetch_doi_citation_metadata, + normalize_doi, + service_scripts, +) DATA_DIR = Path(__file__).with_name("data") @@ -142,3 +150,199 @@ def test_update_dandiset_from_doi( else: expected["citation"] = citation assert metadata == expected + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "given", + [ + "10.48324/dandi.001827/0.260505.1322", + " 10.48324/dandi.001827/0.260505.1322 ", + "doi:10.48324/dandi.001827/0.260505.1322", + "DOI:10.48324/dandi.001827/0.260505.1322", + "https://doi.org/10.48324/dandi.001827/0.260505.1322", + "http://doi.org/10.48324/dandi.001827/0.260505.1322", + "https://dx.doi.org/10.48324/dandi.001827/0.260505.1322", + "doi.org/10.48324/dandi.001827/0.260505.1322", + ], +) +def test_normalize_doi(given: str) -> None: + assert normalize_doi(given) == "10.48324/dandi.001827/0.260505.1322" + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "given", + [ + "", + "not a doi", + "https://doi.org/", + "https://example.com/10.1234/foo", + "10.1/too-short-prefix", + ], +) +def test_normalize_doi_rejects_non_doi(given: str) -> None: + with pytest.raises(ValueError, match="does not look like a DOI"): + normalize_doi(given) + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "given,expected", + [ + # The DOI Handbook allows the registrant to subdivide the prefix. + ("10.1000.10/123", "10.1000.10/123"), + ("https://doi.org/10.1000.10/123", "10.1000.10/123"), + ("10.1000.10.5/123", "10.1000.10.5/123"), + # A resolver URL copied from a browser can carry a query string or + # fragment. Those belong to the URL, not to the DOI. + ("https://doi.org/10.1234/foo?locatt=mode:legacy", "10.1234/foo"), + ("https://doi.org/10.1234/foo#section", "10.1234/foo"), + ("http://dx.doi.org/10.1234/foo?x=1#y", "10.1234/foo"), + # A bare or doi:-prefixed DOI keeps them, since they are legal in a DOI. + ("10.1234/foo?bar", "10.1234/foo?bar"), + ("doi:10.1234/foo#bar", "10.1234/foo#bar"), + ], +) +def test_normalize_doi_prefix_and_url_suffix(given: str, expected: str) -> None: + assert normalize_doi(given) == expected + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "fields,record,missing", + [ + ({"contributor"}, {"title": "T"}, "author"), + ({"relatedResource"}, {"author": []}, "title"), + ({"contributor", "relatedResource"}, {}, "author"), + ], +) +def test_check_doi_fields_missing( + fields: set[str], record: dict, missing: str +) -> None: + with pytest.raises(click.ClickException, match=re.escape(repr(missing))): + check_doi_fields("10.1234/foo", record, fields) + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "fields,record", + [ + # Only the requested fields are required. + ({"name"}, {}), + ({"description"}, {}), + ({"contributor"}, {"author": []}), + ({"contributor", "relatedResource"}, {"author": [], "title": "T"}), + ], +) +def test_check_doi_fields_ok(fields: set[str], record: dict) -> None: + check_doi_fields("10.1234/foo", record, fields) + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_404_from_agency() -> None: + # doi.org 302s a registered DOI to its registration agency, which can 404 + # even though the DOI exists. That must not be reported as unregistered. + doi = "10.1234/registered-but-no-csl" + responses.add( + responses.GET, + f"https://doi.org/{doi}", + status=302, + headers={"Location": "https://data.crossref.org/nope"}, + ) + responses.add(responses.GET, "https://data.crossref.org/nope", status=404) + with pytest.raises(click.ClickException) as excinfo: + fetch_doi_citation_metadata(doi) + msg = str(excinfo.value) + assert "is registered but no citation metadata" in msg + # assert the whole redirect target, not a bare hostname substring, which + # CodeQL flags as incomplete URL sanitization + assert "https://data.crossref.org/nope" in msg + assert "not registered" not in msg + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_404_from_resolver() -> None: + # A 404 straight from doi.org does mean the DOI is not registered. + doi = "10.1234/does-not-exist" + responses.add(responses.GET, f"https://doi.org/{doi}", status=404) + with pytest.raises(click.ClickException, match="is not registered"): + fetch_doi_citation_metadata(doi) + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_non_json() -> None: + # doi.org falls back to redirecting to the landing page when the + # registration agency cannot serve CSL JSON, so we get HTML with a 200. + # See https://github.com/dandi/dandi-cli/issues/1855 + doi = "10.48324/dandi.001827/0.260505.1322" + responses.add( + responses.GET, + f"https://doi.org/{doi}", + body="Dandiset 001827", + status=200, + content_type="text/html; charset=utf-8", + ) + with pytest.raises(click.ClickException) as excinfo: + fetch_doi_citation_metadata(doi) + message = str(excinfo.value) + assert doi in message + assert "did not resolve to citation metadata" in message + assert "text/html" in message + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_not_found() -> None: + doi = "10.48324/dandi.999999/0.000000.0000" + responses.add( + responses.GET, + f"https://doi.org/{doi}", + body="DOI Not Found", + status=404, + content_type="text/plain", + ) + with pytest.raises(click.ClickException) as excinfo: + fetch_doi_citation_metadata(doi) + assert "is not registered" in str(excinfo.value) + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_ok() -> None: + doi = "10.1101/2020.01.17.909838" + responses.add( + responses.GET, + f"https://doi.org/{doi}", + json={"title": "A paper", "author": []}, + status=200, + ) + assert fetch_doi_citation_metadata(doi) == {"title": "A paper", "author": []} + + +@pytest.mark.ai_generated +@responses.activate +def test_fetch_doi_citation_metadata_requests_csl_json() -> None: + # The CSL Accept header used to be set on the session only, where + # `RESTFullAPIClient.request()` overrode it with "application/json" while + # building a JSON request. See https://github.com/dandi/dandi-cli/issues/1855 + doi = "10.1101/2020.01.17.909838" + responses.add( + responses.GET, f"https://doi.org/{doi}", json={"title": "A paper"}, status=200 + ) + fetch_doi_citation_metadata(doi) + assert responses.calls[0].request.headers["Accept"] == DOI_CSL_ACCEPT + + +@pytest.mark.ai_generated +def test_update_dandiset_from_doi_bad_doi() -> None: + r = CliRunner().invoke( + service_scripts, + ["update-dandiset-from-doi", "-d", "000001", "not-a-doi"], + ) + assert r.exit_code == 2 + assert "does not look like a DOI" in r.output + assert "Traceback" not in r.output