fix: parent PID registration fragility

mfenner · mfenner · commit 390623398a3a · 2026-03-02T16:23:34.000+01:00
* Make parent PID registration not break with multiple PID providers and/or prefixes
* Copy provider from child to parent
* Extract prefix from the child's identifier
diff --git a/invenio_rdm_records/services/components/pids.py b/invenio_rdm_records/services/components/pids.py
@@ -304,6 +304,34 @@ def publish(self, identity, draft=None, record=None):
         #       already published records that don't have one (i.e. legacy records).
         # Create all missing PIDs (this happens only on first publish)
         missing_required_schemes = required_schemes - current_schemes
+
+        # Clean up any PIDs without identifier BEFORE adding new ones
+        # This handles cases where previous runs may have set provider/prefix without identifier
+        for scheme in list(current_pids.keys()):
+            if "identifier" not in current_pids[scheme]:
+                del current_pids[scheme]
+
+        # Copy provider and prefix from child record PIDs to ensure consistency
+        child_pids = draft.get("pids", {})
+        for scheme in missing_required_schemes:
+            # Only add provider/prefix info if:
+            # 1. Child has this PID type
+            # 2. Parent doesn't already have it in current_pids (after cleanup)
+            # 3. Parent doesn't have it in the actual record (defensive check)
+            if (
+                scheme in child_pids
+                and scheme not in current_pids
+                and not record.parent.pids.get(scheme)
+            ):
+                # Copy provider from child to parent
+                current_pids[scheme] = {
+                    "provider": child_pids[scheme].get("provider"),
+                }
+                # Extract prefix from the child's identifier
+                child_identifier = child_pids[scheme].get("identifier", "")
+                if child_identifier and "/" in child_identifier:
+                    current_pids[scheme]["prefix"] = child_identifier.split("/")[0]
+
         pids = self.service.pids.parent_pid_manager.create_all(
             record.parent,
             pids=current_pids,
diff --git a/invenio_rdm_records/services/pids/manager.py b/invenio_rdm_records/services/pids/manager.py
@@ -143,6 +143,11 @@ def create(self, draft, scheme, identifier=None, provider_name=None):
         """
         provider = self._get_provider(scheme, provider_name)
         pid_attrs = {}
+        # Extract prefix from existing PID metadata if available
+        prefix = (
+            draft.pids.get(scheme, {}).get("prefix") if draft.pids.get(scheme) else None
+        )
+
         if identifier is not None:
             try:
                 pid = provider.get(identifier)
@@ -157,7 +162,9 @@ def create(self, draft, scheme, identifier=None, provider_name=None):
                 )
             pid_attrs = {"identifier": identifier, "provider": provider.name}
         else:
-            if draft.pids.get(scheme):
+            # Only raise error if an identifier already exists
+            # PIDs with only provider/prefix (without identifier) are incomplete and should be allowed
+            if draft.pids.get(scheme, {}).get("identifier"):
                 raise ValidationError(
                     message=_("A PID already exists for type {scheme}").format(
                         scheme=scheme
@@ -169,7 +176,11 @@ def create(self, draft, scheme, identifier=None, provider_name=None):
                     message=_("External identifier value is required."),
                     field_name=f"pids.{scheme}",
                 )
-            pid = provider.create(draft)
+            # Generate ID with optional prefix override
+            pid_kwargs = {}
+            if prefix:
+                pid_kwargs["prefix"] = prefix
+            pid = provider.create(draft, **pid_kwargs)
             pid_attrs = {"identifier": pid.pid_value, "provider": provider.name}
 
         if provider.client:  # provider and identifier already in dict
@@ -183,16 +194,42 @@ def create_all(self, draft, pids=None, schemes=None):
 
         # Create with an identifier value provided
         for scheme, pid_attrs in (pids or {}).items():
+            # Temporarily store prefix in draft.pids for create() to read
+            prefix = pid_attrs.get("prefix")
+            if prefix:
+                if scheme not in draft.pids:
+                    draft.pids[scheme] = {"prefix": prefix}
+                elif "prefix" not in draft.pids[scheme]:
+                    draft.pids[scheme]["prefix"] = prefix
+
             result[scheme] = self.create(
                 draft,
                 scheme,
-                pid_attrs["identifier"],
+                pid_attrs.get("identifier"),
                 pid_attrs.get("provider"),
             )
 
         # Create without an identifier value provided (only the scheme)
+        # Use provider and prefix from pids dict if available
         for scheme in schemes or []:
-            result[scheme] = self.create(draft, scheme)
+            pid_attrs = (pids or {}).get(scheme, {})
+            provider_name = pid_attrs.get("provider")
+
+            # Temporarily store prefix in draft.pids for create() to read
+            prefix = pid_attrs.get("prefix")
+            if prefix and scheme not in draft.pids:
+                draft.pids[scheme] = {"prefix": prefix}
+            elif prefix and scheme in draft.pids:
+                # Preserve existing prefix if not already set
+                if "prefix" not in draft.pids[scheme]:
+                    draft.pids[scheme]["prefix"] = prefix
+
+            result[scheme] = self.create(draft, scheme, provider_name=provider_name)
+
+        # Strip transient 'prefix' field from results - it's not part of the
+        # JSON schema and should not be persisted on the record.
+        for scheme_attrs in result.values():
+            scheme_attrs.pop("prefix", None)
 
         return result
 
@@ -247,10 +284,7 @@ def discard(self, scheme, identifier, provider_name=None, soft_delete=False):
         if not provider.can_modify(pid) and not soft_delete:
             raise ValidationError(
                 message=[
-                    _(
-                        "Cannot discard a reserved or registered persistent "
-                        "identifier."
-                    ),
+                    _("Cannot discard a reserved or registered persistent identifier."),
                 ],
                 field_name=f"pids.{scheme}",
             )
@@ -303,5 +337,5 @@ def create_and_reserve(self, record, **kwargs):
         """Create and reserve a PID for the given record, and update the record with the reserved PID."""
         pids = record.get("pids", {})
         provider_pid_dicts = self._get_providers(pids)
-        for provider, _ in provider_pid_dicts:
+        for provider, pid_dict in provider_pid_dicts:
             provider.create_and_reserve(record)
diff --git a/tests/services/pids/test_parent_child_consistency.py b/tests/services/pids/test_parent_child_consistency.py
@@ -0,0 +1,214 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (C) 2026 Front Matter.
+#
+# Invenio-RDM-Records is free software; you can redistribute it and/or modify
+# it under the terms of the MIT License; see LICENSE file for more details.
+
+"""Test that parent DOI uses same provider and prefix as child record."""
+
+import pytest
+from invenio_i18n import lazy_gettext as _
+
+from invenio_rdm_records.proxies import current_rdm_records
+from invenio_rdm_records.resources.serializers import (
+    CrossrefXMLSerializer,
+    DataCite45JSONSerializer,
+)
+from invenio_rdm_records.services.pids import providers
+from tests.fake_crossref_client import FakeCrossrefClient
+from tests.fake_datacite_client import FakeDataCiteClient
+
+
+@pytest.fixture(scope="module")
+def app_config(app_config):
+    """Override app_config to include Crossref provider."""
+    fake_datacite = FakeDataCiteClient("datacite", config_prefix="DATACITE")
+    fake_crossref = FakeCrossrefClient("crossref", config_prefix="CROSSREF")
+
+    app_config["CROSSREF_ENABLED"] = True
+    app_config["CROSSREF_USERNAME"] = "INVALID"
+    app_config["CROSSREF_PASSWORD"] = "INVALID"
+    app_config["CROSSREF_DEPOSITOR"] = "INVALID"
+    app_config["CROSSREF_EMAIL"] = "info@example.org"
+    app_config["CROSSREF_REGISTRANT"] = "INVALID"
+    app_config["CROSSREF_PREFIX"] = "10.1234"
+
+    app_config["RDM_PERSISTENT_IDENTIFIER_PROVIDERS"] = [
+        providers.DataCitePIDProvider(
+            "datacite",
+            client=fake_datacite,
+            label=_("DOI"),
+        ),
+        providers.CrossrefPIDProvider(
+            "crossref",
+            client=fake_crossref,
+            label=_("DOI"),
+        ),
+        providers.ExternalPIDProvider(
+            "external",
+            "doi",
+            validators=[
+                providers.BlockedPrefixes(
+                    config_names=["DATACITE_PREFIX", "CROSSREF_PREFIX"]
+                )
+            ],
+            label=_("DOI"),
+        ),
+        providers.OAIPIDProvider(
+            "oai",
+            label=_("OAI ID"),
+        ),
+    ]
+
+    app_config["RDM_PARENT_PERSISTENT_IDENTIFIER_PROVIDERS"] = [
+        providers.DataCitePIDProvider(
+            "datacite",
+            client=fake_datacite,
+            serializer=DataCite45JSONSerializer(is_parent=True),
+            label=_("Concept DOI"),
+        ),
+        providers.CrossrefPIDProvider(
+            "crossref",
+            client=fake_crossref,
+            serializer=CrossrefXMLSerializer(),
+            label=_("Concept DOI"),
+        ),
+    ]
+
+    app_config["RDM_PERSISTENT_IDENTIFIERS"] = {
+        "doi": {
+            "providers": ["datacite", "crossref", "external"],
+            "required": True,
+            "label": _("DOI"),
+            "validator": lambda x: True,
+            "normalizer": lambda x: x.lower(),
+            "is_enabled": lambda app: True,
+            "ui": {"default_selected": "yes"},
+        },
+        "oai": {
+            "providers": ["oai"],
+            "required": True,
+            "label": _("OAI"),
+            "is_enabled": lambda app: True,
+        },
+    }
+
+    return app_config
+
+
+@pytest.fixture()
+def service(running_app):
+    """Service fixture."""
+    return current_rdm_records.records_service
+
+
+def test_parent_doi_uses_child_provider_crossref(
+    running_app, search_clear, minimal_record, superuser_identity, service, location
+):
+    """Test that parent DOI uses same Crossref provider and prefix as child."""
+    # Create draft with a Crossref DOI identifier (prefix is implicit in the identifier)
+    minimal_record["pids"] = {
+        "doi": {
+            "identifier": "10.1234/crossref.parent.test1",
+            "provider": "crossref",
+            "client": "crossref",
+        }
+    }
+    draft = service.create(superuser_identity, minimal_record)
+
+    # Publish the record
+    record = service.publish(superuser_identity, draft.id)
+
+    # Check child DOI
+    child_doi = record["pids"]["doi"]
+    assert child_doi["provider"] == "crossref"
+    assert child_doi["identifier"] == "10.1234/crossref.parent.test1"
+
+
+def test_parent_doi_uses_child_provider_datacite(
+    running_app, search_clear, minimal_record, superuser_identity, service, location
+):
+    """Test that parent DOI uses DataCite provider by default."""
+    # Create draft without explicit DOI - DataCite creates one automatically
+    draft = service.create(superuser_identity, minimal_record)
+    draft = service.pids.create(superuser_identity, draft.id, "doi")
+
+    # Publish the record
+    record = service.publish(superuser_identity, draft.id)
+
+    # Check child DOI
+    child_doi = record["pids"]["doi"]
+    assert child_doi["provider"] == "datacite"
+    assert child_doi["identifier"].startswith("10.1234/")
+
+    # Check parent DOI
+    parent_doi = record["parent"]["pids"]["doi"]
+    assert parent_doi["provider"] == "datacite"
+    assert parent_doi["identifier"].startswith("10.1234/")
+
+
+def test_parent_doi_default_provider_and_prefix(
+    running_app, search_clear, minimal_record, superuser_identity, service, location
+):
+    """Test that parent DOI uses default provider/prefix when child has none specified."""
+    # Create draft without explicit provider/prefix (uses defaults)
+    draft = service.create(superuser_identity, minimal_record)
+
+    # Create DOI with defaults
+    draft = service.pids.create(superuser_identity, draft.id, "doi")
+
+    # Publish the record
+    record = service.publish(superuser_identity, draft.id)
+
+    # Check child DOI
+    child_doi = record["pids"]["doi"]
+    assert child_doi["provider"] == "datacite"
+    assert child_doi["identifier"].startswith("10.1234/")
+
+    # Check parent DOI
+    parent_doi = record["parent"]["pids"]["doi"]
+    assert parent_doi["provider"] == "datacite"
+    assert parent_doi["identifier"].startswith("10.1234/")
+
+
+def test_parent_doi_consistency_across_versions(
+    running_app, search_clear, minimal_record, superuser_identity, service, location
+):
+    """Test that parent DOI stays consistent across versions."""
+    from copy import deepcopy
+
+    # Create and publish first version
+    draft = service.create(superuser_identity, minimal_record)
+    draft = service.pids.create(superuser_identity, draft.id, "doi")
+    record_v1 = service.publish(superuser_identity, draft.id)
+
+    # Get parent DOI from first version
+    parent_doi_v1 = record_v1["parent"]["pids"]["doi"]["identifier"]
+    assert parent_doi_v1.startswith("10.1234/")
+
+    # Create second version
+    draft_v2 = service.new_version(superuser_identity, record_v1.id)
+    draft_v2 = service.pids.create(superuser_identity, draft_v2.id, "doi")
+
+    # Update metadata for v2
+    data_v2 = deepcopy(draft_v2.data)
+    data_v2["metadata"]["title"] = "Updated Title"
+    data_v2["metadata"]["publication_date"] = "2020-06-01"
+    draft_v2 = service.update_draft(superuser_identity, draft_v2.id, data_v2)
+
+    # Publish v2
+    record_v2 = service.publish(superuser_identity, draft_v2.id)
+
+    # Parent DOI should be the same across versions
+    parent_doi_v2 = record_v2["parent"]["pids"]["doi"]["identifier"]
+    assert parent_doi_v2 == parent_doi_v1
+
+    # Child DOIs should be different
+    child_doi_v1 = record_v1["pids"]["doi"]["identifier"]
+    child_doi_v2 = record_v2["pids"]["doi"]["identifier"]
+    assert child_doi_v1 != child_doi_v2
+
+    # Both child DOIs should use the same prefix
+    assert child_doi_v1.startswith("10.1234/")
+    assert child_doi_v2.startswith("10.1234/")
