[BDC-1248] Fix DOI discovery

docs/howto/discoveryMetadataTools.md

@@ -25,7 +25,7 @@ of a convenience.
 
 ### Export Discovery Metadata into File
 
-Gen3's SDK can be used to export discovery metadata from a certain Gen3 environment into a file by using the `output_expanded_discovery_metadata()` function. By default this function will query for metadata with `guid_type=discovery_metadata` for the dump, and export the metadata into a TSV file. User can also specify a different `guid_type` values for this operation, and/or choose to export the metadata into a JSON file. When using TSV format, some certain fields from metadata will be flattened or "jsonified" so that each metadata record can be fitted into one row.
+Gen3's SDK can be used to export discovery metadata from a certain Gen3 environment into a file by using the `output_expanded_discovery_metadata()` function. By default this function will query for metadata with `guid_type=discovery_metadata` for the dump, and export the metadata into a TSV file. The default `limit` is 500 records, so pass an explicit limit for a larger export. User can also specify a different `guid_type` values for this operation, and/or choose to export the metadata into a JSON file. When using TSV format, some certain fields from metadata will be flattened or "jsonified" so that each metadata record can be fitted into one row.
 
 Example of usage:
 
@@ -388,6 +388,14 @@ See below for a full example using the dbGaP `DbgapMetadataInterface`.
 More interfaces may exist in the future for doing this by querying non-dbGaP
 sources.
 
+The DOI workflow uses `output_expanded_discovery_metadata()` to build its
+alternate-ID mapping. It passes a 10,000 record limit and fetches MDS in pages and
+includes all fetched pages in the TSV. Raise that DOI specific limit if a commons
+grows beyond it. In the dbGaP interface, records with missing required source
+metadata or an invalid `ReleaseDate` are logged and skipped individually while valid
+records continue. Contributors without a source `contributorType` are sent to
+DataCite as `Other`.
+
 ```python
 import os
 from requests.auth import HTTPBasicAuth

gen3/discovery_dois.py

@@ -21,6 +21,8 @@
 
 logging = get_logger(__name__)
 
+DOI_DISCOVERY_METADATA_EXPORT_LIMIT = 10_000
+
 
 class GetMetadataInterface(object):
     """
@@ -418,8 +420,9 @@ def _raise_exception_on_collision(datacite, identifier):
 def get_alternate_id_to_guid_mapping(metadata_field_for_alternate_id, auth):
     """
     Return mapping from the alternate ID in current Discovery Metadata
-    to Metadata GUID. This function uses the provided `metadata_field_for_alternate_id`
-    to find the actual value in the Discovery Metadata).
+    to Metadata GUID. This function uses the expanded Discovery metadata export
+    with an explicit limit and the provided `metadata_field_for_alternate_id`
+    to find the actual value in the Discovery Metadata.
 
     Args:
         metadata_field_for_alternate_id (str): Field in current Discovery Metadata
@@ -433,26 +436,30 @@ def get_alternate_id_to_guid_mapping(metadata_field_for_alternate_id, auth):
     """
     loop = get_or_create_event_loop_for_thread()
     output_filename = loop.run_until_complete(
-        output_expanded_discovery_metadata(auth, endpoint=auth.endpoint)
+        output_expanded_discovery_metadata(
+            auth,
+            endpoint=auth.endpoint,
+            limit=DOI_DISCOVERY_METADATA_EXPORT_LIMIT,
+        )
     )
 
     alternate_id_to_guid = {}
     all_discovery_metadata = {}
-    with open(output_filename) as metadata_file:
+    with open(output_filename, encoding="utf-8") as metadata_file:
         csv_parser_setting = {
             **BASE_CSV_PARSER_SETTINGS,
             "delimiter": get_delimiter_from_extension(output_filename),
         }
-        metadata_reader = csv.DictReader(metadata_file, **{**csv_parser_setting})
+        metadata_reader = csv.DictReader(metadata_file, **csv_parser_setting)
         for row in metadata_reader:
-            if row.get(metadata_field_for_alternate_id):
-                alternate_id_to_guid[row.get(metadata_field_for_alternate_id)] = row[
-                    "guid"
-                ]
+            alternate_id = row.get(metadata_field_for_alternate_id)
+            if alternate_id:
+                alternate_id_to_guid[alternate_id] = row["guid"]
                 all_discovery_metadata[row["guid"]] = row
             else:
                 logging.warning(
-                    f"Could not find field {metadata_field_for_alternate_id} on row: {row}. Skipping..."
+                    f"Could not find `{metadata_field_for_alternate_id}` on Discovery "
+                    f"metadata GUID `{row.get('guid', '')}`. Skipping..."
                 )
 
     return alternate_id_to_guid, all_discovery_metadata

gen3/external/nih/dbgap_doi.py

@@ -73,7 +73,9 @@ def __init__(
 
     def get_metadata_for_ids(self, ids):
         """
-        Return DOI metadata for each of the provided IDs.
+        Return DOI metadata for each provided ID with valid required source data.
+        Invalid records are logged and skipped without blocking valid records in
+        the same batch.
 
         Args:
             ids (List[str]): list of IDs to query for
@@ -97,55 +99,64 @@ def get_metadata_for_ids(self, ids):
 
         for phsid in ids:
             if not dbgap_fhir_metadata.get(phsid):
-                logging.error(
+                logging.warning(
                     f"{phsid} is missing dbGaP FHIR metadata. Cannot "
                     f"continue creating a DOI without it. Skipping..."
                 )
                 continue
 
             if not dbgap_study_registration_metadata.get(phsid):
-                logging.error(
+                logging.warning(
                     f"{phsid} is missing dbGaP Study Registration "
                     f"metadata. Cannot continue creating a DOI without it. Skipping..."
                 )
                 continue
 
-            doi_metadata = {}
+            try:
+                doi_metadata = {}
 
-            # 1) required fields
-            doi_metadata["creators"] = dbgapDOI._get_doi_creators(
-                phsid, dbgap_study_registration_metadata
-            )
-            doi_metadata["titles"] = dbgapDOI._get_doi_title(phsid, dbgap_fhir_metadata)
-            doi_metadata["publication_year"] = dbgapDOI._get_doi_publication_year(
-                phsid, dbgap_fhir_metadata
-            )
-            doi_metadata["doi_type_general"] = "Dataset"
+                # 1) required fields
+                doi_metadata["creators"] = dbgapDOI._get_doi_creators(
+                    phsid, dbgap_study_registration_metadata
+                )
+                doi_metadata["titles"] = dbgapDOI._get_doi_title(
+                    phsid, dbgap_fhir_metadata
+                )
+                doi_metadata["publication_year"] = dbgapDOI._get_doi_publication_year(
+                    phsid, dbgap_fhir_metadata
+                )
+                doi_metadata["doi_type_general"] = "Dataset"
 
-            # publisher is provided
-            doi_metadata["publisher"] = self.publisher
+                # publisher is provided
+                doi_metadata["publisher"] = self.publisher
 
-            # NOTE: This does NOT include the required landing page URL
-            #       b/c this requires the final ID (which should be generated
-            #       elsewhere).
-            # doi_metadata["url"] = None
+                # NOTE: This does NOT include the required landing page URL
+                #       b/c this requires the final ID (which should be generated
+                #       elsewhere).
+                # doi_metadata["url"] = None
 
-            # 2) optional fields
-            doi_metadata["version"] = dbgapDOI._get_doi_version(
-                phsid, dbgap_fhir_metadata
-            )
-            doi_metadata["contributors"] = dbgapDOI._get_doi_contributors(
-                phsid, dbgap_study_registration_metadata, dbgap_fhir_metadata
-            )
-            doi_metadata["descriptions"] = dbgapDOI._get_doi_descriptions(
-                phsid, dbgap_fhir_metadata
-            )
-            doi_metadata[
-                "alternateIdentifiers"
-            ] = dbgapDOI._get_doi_alternate_identifiers(phsid, dbgap_fhir_metadata)
-            doi_metadata["fundingReferences"] = dbgapDOI._get_doi_funding(
-                phsid, dbgap_fhir_metadata
-            )
+                # 2) optional fields
+                doi_metadata["version"] = dbgapDOI._get_doi_version(
+                    phsid, dbgap_fhir_metadata
+                )
+                contributors = dbgapDOI._get_doi_contributors(
+                    phsid, dbgap_study_registration_metadata, dbgap_fhir_metadata
+                )
+                for contributor in contributors:
+                    contributor.setdefault("contributorType", "Other")
+                doi_metadata["contributors"] = contributors
+                doi_metadata["descriptions"] = dbgapDOI._get_doi_descriptions(
+                    phsid, dbgap_fhir_metadata
+                )
+                doi_metadata[
+                    "alternateIdentifiers"
+                ] = dbgapDOI._get_doi_alternate_identifiers(phsid, dbgap_fhir_metadata)
+                doi_metadata["fundingReferences"] = dbgapDOI._get_doi_funding(
+                    phsid, dbgap_fhir_metadata
+                )
+            except Exception as exc:
+                logging.warning(f"Skipping dbGaP DOI metadata for `{phsid}`: {exc}")
+                continue
 
             all_doi_metadata[phsid] = doi_metadata
 
@@ -196,24 +207,20 @@ def _get_doi_title(phsid, dbgap_fhir_metadata):
     def _get_doi_publication_year(phsid, dbgap_fhir_metadata):
         date = dbgap_fhir_metadata.get(phsid, {}).get("ReleaseDate")
 
-        if not date:
-            logging.debug(f"dbgap_fhir_metadata: {dbgap_fhir_metadata}")
-            raise Exception(
-                f"ReleaseDate from dbgap FHIR does not match expected pattern "
-                f"YYYY-MM-DD: '{date}'. Unable to parse."
+        if not isinstance(date, str):
+            raise ValueError(
+                f"dbGaP FHIR ReleaseDate for `{phsid}` must begin with a four-digit "
+                f"year; received {date!r}."
             )
 
-        if date:
-            date = date.split("-")[0]
-
-            if len(date) != 4:
-                logging.debug(f"dbgap_fhir_metadata: {dbgap_fhir_metadata}")
-                raise Exception(
-                    f"ReleaseDate from dbgap FHIR does not match expected pattern "
-                    f"YYYY-MM-DD: '{date}'. Unable to parse."
-                )
+        publication_year = date.split("-", 1)[0]
+        if len(publication_year) != 4 or not publication_year.isdigit():
+            raise ValueError(
+                f"dbGaP FHIR ReleaseDate for `{phsid}` must begin with a four-digit "
+                f"year; received {date!r}."
+            )
 
-        return date
+        return publication_year
 
     @staticmethod
     def _get_doi_contributors(

tests/test_dbgap_doi.py

@@ -0,0 +1,126 @@
+from unittest.mock import patch
+
+import pytest
+
+from gen3.external.nih.dbgap_doi import dbgapDOI
+
+
+VALID_PHSID = "phs000007.v1.p1.c1"
+INVALID_PHSID = "phs002909.v2.p1.c1"
+
+
+@pytest.mark.parametrize("invalid_release_date", [None, "not-a-date"])
+def test_get_metadata_for_ids_skips_invalid_release_dates(
+    invalid_release_date,
+):
+    """
+    Pre BDC-1248 behavior: a malformed upstream record (e.g. missing or
+    unparseable ReleaseDate) raised an exception that aborted the entire batch.
+
+    This test requests one valid accession and one invalid accession together
+    and asserts:
+    - the valid accession's DOI metadata is still returned,
+    - the invalid accession is omitted rather than raising,
+    - a warning naming the accession and the raw bad ReleaseDate is logged.
+
+    Parametrized with a None value and a non-empty value that doesn't parse to a four digit year.
+    """
+    study_registration_metadata = {
+        VALID_PHSID: {"Authority": {"Persons": {"Person": []}}},
+        INVALID_PHSID: {"Authority": {"Persons": {"Person": []}}},
+    }
+    fhir_metadata = {
+        VALID_PHSID: {
+            "Title": "Valid study",
+            "ReleaseDate": "2024-01-01",
+            "Identifier": [VALID_PHSID],
+            "Description": "A valid dbGaP study.",
+            "Sponsor": "NHLBI",
+        },
+        INVALID_PHSID: {
+            "Title": "Invalid release date study",
+            "ReleaseDate": invalid_release_date,
+            "Identifier": [INVALID_PHSID],
+            "Description": "A dbGaP study without a valid release date.",
+            "Sponsor": "NHLBI",
+        },
+    }
+
+    with patch(
+        "gen3.external.nih.dbgap_doi.dbgapStudyRegistration"
+    ) as mock_study_registration, patch(
+        "gen3.external.nih.dbgap_doi.dbgapFHIR"
+    ) as mock_fhir, patch.object(
+        dbgapDOI, "_get_doi_contributors", return_value=[]
+    ), patch(
+        "gen3.external.nih.dbgap_doi.logging.warning"
+    ) as mock_warning:
+        mock_study_registration.return_value.get_metadata_for_ids.return_value = (
+            study_registration_metadata
+        )
+        mock_fhir.return_value.get_metadata_for_ids.return_value = fhir_metadata
+
+        # Request both accessions
+        doi_metadata = dbgapDOI(publisher="Example publisher").get_metadata_for_ids(
+            [VALID_PHSID, INVALID_PHSID]
+        )
+
+    # The invalid accession must be skipped and not raised
+    assert list(doi_metadata) == [VALID_PHSID]
+    assert doi_metadata[VALID_PHSID]["publication_year"] == "2024"
+    # Make sure the the warning names the accession and the raw
+    # bad value so we can trace it back to the dbGaP source record
+    assert any(
+        INVALID_PHSID in call.args[0]
+        and "ReleaseDate" in call.args[0]
+        and repr(invalid_release_date) in call.args[0]
+        for call in mock_warning.call_args_list
+    )
+
+
+def test_get_metadata_for_ids_normalizes_contributors_missing_type():
+    """
+    Pre BDC-1248 behavior: contributors lacking a DataCite-required
+    contributorType caused DataCite to reject the payload with an HTTP 422.
+
+    This test gives one contributor missing contributorType and one with an
+    existing value, and asserts the missing type is defaulted to "Other" while
+    the existing value is untouched.
+    """
+    study_registration_metadata = {
+        VALID_PHSID: {"Authority": {"Persons": {"Person": []}}},
+    }
+    fhir_metadata = {
+        VALID_PHSID: {
+            "Title": "Valid study",
+            "ReleaseDate": "2024-01-01",
+            "Identifier": [VALID_PHSID],
+            "Description": "A valid dbGaP study.",
+            "Sponsor": "NHLBI",
+        },
+    }
+    contributors = [
+        {"name": "Untyped contributor"},
+        {"name": "Typed contributor", "contributorType": "ProjectLeader"},
+    ]
+
+    with patch(
+        "gen3.external.nih.dbgap_doi.dbgapStudyRegistration"
+    ) as mock_study_registration, patch(
+        "gen3.external.nih.dbgap_doi.dbgapFHIR"
+    ) as mock_fhir, patch.object(
+        dbgapDOI, "_get_doi_contributors", return_value=contributors
+    ):
+        mock_study_registration.return_value.get_metadata_for_ids.return_value = (
+            study_registration_metadata
+        )
+        mock_fhir.return_value.get_metadata_for_ids.return_value = fhir_metadata
+
+        doi_metadata = dbgapDOI(publisher="Example publisher").get_metadata_for_ids(
+            [VALID_PHSID]
+        )
+
+    assert doi_metadata[VALID_PHSID]["contributors"] == [
+        {"name": "Untyped contributor", "contributorType": "Other"},
+        {"name": "Typed contributor", "contributorType": "ProjectLeader"},
+    ]