diff --git a/docs/howto/discoveryMetadataTools.md b/docs/howto/discoveryMetadataTools.md index 95d76789..94631297 100644 --- a/docs/howto/discoveryMetadataTools.md +++ b/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 diff --git a/gen3/discovery_dois.py b/gen3/discovery_dois.py index 769ec71c..7f64260c 100644 --- a/gen3/discovery_dois.py +++ b/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 diff --git a/gen3/external/nih/dbgap_doi.py b/gen3/external/nih/dbgap_doi.py index fbbab279..f2427c76 100644 --- a/gen3/external/nih/dbgap_doi.py +++ b/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( diff --git a/tests/test_dbgap_doi.py b/tests/test_dbgap_doi.py new file mode 100644 index 00000000..28033515 --- /dev/null +++ b/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"}, + ] diff --git a/tests/test_discovery.py b/tests/test_discovery.py index a4386a9e..8b0ee378 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -1,8 +1,9 @@ import asyncio import csv import json +import os import tempfile -from unittest.mock import patch +from unittest.mock import call, patch import pytest from gen3.tools.metadata.discovery import ( @@ -208,6 +209,69 @@ def test_discovery_read( ) +@patch("gen3.tools.metadata.discovery._create_metadata_output_filename") +@patch("gen3.metadata.Gen3Metadata.query") +def test_discovery_tsv_export_with_large_limit_reads_multiple_pages( + metadata_query_patch, metadata_file_patch, gen3_auth, tmp_path +): + """ + This test proves the exporter actually makes paginated MDS + queries when given a larger limit. + """ + output_filename = tmp_path / "discovery_metadata.tsv" + metadata_file_patch.return_value = str(output_filename) + metadata_query_patch.side_effect = [ + { + "guid_one": { + "_guid_type": "discovery_metadata", + "gen3_discovery": {"dbgap_accession": "phs004546.v1.p1.c1"}, + } + }, + { + "guid_two": { + "_guid_type": "discovery_metadata", + "gen3_discovery": {"dbgap_accession": "phs000007.v1.p1.c1"}, + } + }, + ] + + loop = asyncio.new_event_loop() + try: + loop.run_until_complete( + output_expanded_discovery_metadata( + gen3_auth, + endpoint="excommons.org", + limit=4000, + ) + ) + finally: + loop.close() + + with open(output_filename, encoding="utf-8") as output_file: + csv_rows = list(csv.DictReader(output_file, **BASE_CSV_PARSER_SETTINGS)) + + assert csv_rows == [ + {"guid": "guid_one", "dbgap_accession": "phs004546.v1.p1.c1"}, + {"guid": "guid_two", "dbgap_accession": "phs000007.v1.p1.c1"}, + ] + assert metadata_query_patch.call_args_list == [ + call( + "_guid_type=discovery_metadata", + return_full_metadata=True, + limit=2000, + offset=0, + use_agg_mds=False, + ), + call( + "_guid_type=discovery_metadata", + return_full_metadata=True, + limit=2000, + offset=2000, + use_agg_mds=False, + ), + ] + + @patch("gen3.metadata.Gen3Metadata.async_create") @pytest.mark.parametrize("ignore_empty_columns", [True, False]) def test_discovery_publish_omit_empty_columns( diff --git a/tests/test_doi_discovery.py b/tests/test_doi_discovery.py index a8024eb5..48a3fb63 100644 --- a/tests/test_doi_discovery.py +++ b/tests/test_doi_discovery.py @@ -5,7 +5,12 @@ from unittest.mock import MagicMock, patch from gen3.auth import Gen3Auth -from gen3.discovery_dois import mint_dois_for_discovery_datasets, GetMetadataInterface +from gen3.discovery_dois import ( + DOI_DISCOVERY_METADATA_EXPORT_LIMIT, + GetMetadataInterface, + get_alternate_id_to_guid_mapping, + mint_dois_for_discovery_datasets, +) from gen3.utils import get_random_alphanumeric from gen3.doi import DigitalObjectIdentifier @@ -20,6 +25,48 @@ DOI_CONTACT = "DOI_CONTACT" +@patch("gen3.discovery_dois.output_expanded_discovery_metadata") +def test_get_alternate_id_to_guid_mapping_uses_increased_export_limit( + mock_output_expanded_discovery_metadata, gen3_auth, tmp_path +): + """ + Test to ensure DOI mapping requests an export with DOI_DISCOVERY_METADATA_EXPORT_LIMIT. + """ + output_filename = tmp_path / "discovery_metadata.tsv" + output_filename.write_text( + "guid\tdbgap_accession\tdoi_identifier\n" + "guid_one\tphs004546.v1.p1.c1\t10.12345/EXISTING-ONE\n" + "guid_two\tphs000007.v1.p1.c1\t\n", + encoding="utf-8", + ) + mock_output_expanded_discovery_metadata.return_value = str(output_filename) + + alternate_id_to_guid, all_discovery_metadata = get_alternate_id_to_guid_mapping( + auth=gen3_auth, + metadata_field_for_alternate_id=METADATA_FIELD_FOR_ALTERNATE_ID, + ) + + assert alternate_id_to_guid == { + "phs004546.v1.p1.c1": "guid_one", + "phs000007.v1.p1.c1": "guid_two", + } + assert all_discovery_metadata["guid_one"] == { + "dbgap_accession": "phs004546.v1.p1.c1", + "doi_identifier": "10.12345/EXISTING-ONE", + "guid": "guid_one", + } + assert all_discovery_metadata["guid_two"] == { + "dbgap_accession": "phs000007.v1.p1.c1", + "doi_identifier": "", + "guid": "guid_two", + } + mock_output_expanded_discovery_metadata.assert_called_once_with( + gen3_auth, + endpoint=gen3_auth.endpoint, + limit=DOI_DISCOVERY_METADATA_EXPORT_LIMIT, + ) + + @pytest.mark.parametrize("exclude_datasets", [["guid_W"], ["alternate_id_0"]]) @pytest.mark.parametrize("does_datacite_have_dois_minted_already", [True, False]) @patch("gen3.discovery_dois._raise_exception_on_collision")