Invert the Collection-Doi relationship and use DoiFactory directly in tests

Author: brianhelba

isic/conftest.py

@@ -15,7 +15,7 @@
     get_elasticsearch_client,
     maybe_create_index,
 )
-from isic.core.tests.factories import CollectionFactory, DoiFactory, ImageFactory, IsicIdFactory
+from isic.core.tests.factories import CollectionFactory, ImageFactory, IsicIdFactory
 from isic.ingest.tests.factories import (
     AccessionFactory,
     AccessionReviewFactory,
@@ -108,4 +108,3 @@ def s3ff_random_field_value(s3ff_field_value_factory):
 register(IsicIdFactory)
 register(ImageFactory)
 register(CollectionFactory)
-register(DoiFactory)

isic/core/admin.py

@@ -215,7 +215,7 @@ class SupplementalFileInline(admin.TabularInline):
 @admin.register(Doi)
 class DoiAdmin(StaffReadonlyAdmin):
     list_select_related = ["collection"]
-    list_display = ["id", "url", "collection", "bundle", "num_supplemental_files"]
+    list_display = ["id", "external_url", "collection", "bundle", "num_supplemental_files"]
     inlines = [SupplementalFileInline]
 
     autocomplete_fields = ["creator"]

isic/core/api/collection.py

@@ -45,9 +45,10 @@ def query_min_length(cls, v: str):
 class CollectionOut(ModelSchema):
     class Meta:
         model = Collection
-        fields = ["id", "name", "description", "public", "pinned", "locked", "doi"]
+        fields = ["id", "name", "description", "public", "pinned", "locked"]
 
-    doi_url: str | None = Field(alias="doi_url")
+    doi: str | None = Field(None, alias="doi.id")
+    doi_url: str | None = Field(None, alias="doi.external_url")
 
 
 @router.get(

isic/core/api/doi.py

@@ -41,7 +41,9 @@ def parse_s3_file_field_values(cls, v):  # noqa: N805
     include_in_schema=False,
 )
 def create_doi(request, payload: CreateDOIIn):
-    collection = get_object_or_404(Collection, id=payload.collection_id)
+    collection = get_object_or_404(
+        Collection.objects.select_related("doi"), pk=payload.collection_id
+    )
 
     if not request.user.is_staff:
         return 403, {"error": "You do not have permission to create a DOI."}

isic/core/migrations/0026_invert_doi_fk.py

@@ -0,0 +1,76 @@
+import django.core.validators
+from django.db import migrations, models
+from django.db.backends.base.schema import BaseDatabaseSchemaEditor
+from django.db.migrations.state import StateApps
+from django.db.models import OuterRef, Subquery
+import django.db.models.deletion
+
+import isic.core.models.doi
+
+
+def invert_doi_fk(apps: StateApps, schema_editor: BaseDatabaseSchemaEditor):
+    Doi = apps.get_model("core", "Doi")
+    Collection = apps.get_model("core", "Collection")
+
+    Doi.objects.update(
+        collection=Subquery(Collection.objects.filter(doi=OuterRef("pk")).values("pk")[:1])
+    )
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("core", "0025_alter_collection_shares_alter_image_shares"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="collection",
+            name="doi",
+            field=models.OneToOneField(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.PROTECT,
+                related_name="collection_reverse",
+                to="core.doi",
+            ),
+        ),
+        migrations.AddField(
+            model_name="doi",
+            name="collection",
+            field=models.OneToOneField(
+                default=None,
+                null=True,
+                on_delete=django.db.models.deletion.PROTECT,
+                to="core.collection",
+            ),
+            preserve_default=False,
+        ),
+        migrations.RunPython(invert_doi_fk, elidable=True),
+        migrations.RemoveField(
+            model_name="collection",
+            name="doi",
+        ),
+        migrations.AlterField(
+            model_name="doi",
+            name="collection",
+            field=models.OneToOneField(
+                on_delete=django.db.models.deletion.PROTECT,
+                to="core.collection",
+            ),
+        ),
+        migrations.AlterField(
+            model_name="doi",
+            name="id",
+            field=models.CharField(
+                default=isic.core.models.doi._generate_random_doi_id,  # noqa: SLF001
+                max_length=30,
+                primary_key=True,
+                serialize=False,
+                validators=[django.core.validators.RegexValidator("^\\d+\\.\\d+/\\d+$")],
+            ),
+        ),
+        migrations.RemoveField(
+            model_name="doi",
+            name="url",
+        ),
+    ]

isic/core/models/collection.py

@@ -10,7 +10,6 @@
 from django.urls import reverse
 from django_extensions.db.models import TimeStampedModel
 
-from .doi import Doi
 from .image import Image
 
 
@@ -83,8 +82,6 @@ class Meta(TimeStampedModel.Meta):
 
     pinned = models.BooleanField(default=False)
 
-    doi = models.OneToOneField(Doi, on_delete=models.PROTECT, null=True, blank=True)
-
     locked = models.BooleanField(default=False)
 
     objects = CollectionQuerySet.as_manager()
@@ -100,15 +97,6 @@ def is_magic(self) -> bool:
         """Magic collections are collections pointed to by a cohort."""
         return hasattr(self, "cohort")
 
-    @property
-    def has_doi(self) -> bool:
-        return self.doi is not None
-
-    @property
-    def doi_url(self):
-        if self.doi:
-            return f"https://doi.org/{self.doi}"
-
     @property
     def num_lesions(self):
         return (

isic/core/models/doi.py

@@ -1,12 +1,16 @@
 import json
+import random
 
+from django.conf import settings
 from django.contrib.auth.models import User
 from django.core.files.storage import storages
 from django.core.validators import RegexValidator
 from django.db import models
 from django.urls import reverse
 from django_extensions.db.models import TimeStampedModel
 
+from .collection import Collection
+
 
 def doi_upload_to(instance: "Doi", filename: str) -> str:
     return f"dois/{instance.id.replace('/', '-')}/{filename}"
@@ -16,19 +20,26 @@ def doi_storage():
     return storages["sponsored"]
 
 
+def _generate_random_doi_id():
+    # pad DOI with leading zeros so all DOIs are prefix/6 digits
+    return f"{settings.ISIC_DATACITE_DOI_PREFIX}/{random.randint(10_000, 999_999):06}"  # noqa: S311
+
+
 class Doi(TimeStampedModel):
     class Meta:
         verbose_name = "DOI"
         verbose_name_plural = "DOIs"
 
     id = models.CharField(
-        max_length=30, primary_key=True, validators=[RegexValidator(r"^\d+\.\d+/\d+$")]
+        max_length=30,
+        primary_key=True,
+        default=_generate_random_doi_id,
+        validators=[RegexValidator(r"^\d+\.\d+/\d+$")],
     )
     slug = models.SlugField(max_length=150, unique=True)
+    collection = models.OneToOneField(Collection, on_delete=models.PROTECT)
     creator = models.ForeignKey(User, on_delete=models.RESTRICT)
 
-    url = models.CharField(max_length=200)
-
     bundle = models.FileField(upload_to=doi_upload_to, storage=doi_storage, null=True, blank=True)
     bundle_size = models.PositiveBigIntegerField(null=True, blank=True)
 
@@ -38,10 +49,14 @@ class Meta:
     citations = models.JSONField(default=dict, blank=True)
     schema_org_dataset = models.JSONField(default=dict, blank=True)
 
-    def __str__(self):
+    def __str__(self) -> str:
         return self.id
 
-    def get_absolute_url(self):
+    @property
+    def external_url(self) -> str:
+        return f"https://doi.org/{self.id}"
+
+    def get_absolute_url(self) -> str:
         return reverse("core/doi-detail", kwargs={"slug": self.slug})
 
     def get_schema_org_dataset_json(self):

isic/core/services/collection/__init__.py

@@ -58,7 +58,7 @@ def collection_delete(*, collection: Collection, ignore_lock: bool = False) -> N
     if collection.studies.exists():
         raise ValidationError("Collections with derived studies cannot be deleted.")
 
-    if collection.has_doi:
+    if hasattr(collection, "doi"):
         raise ValidationError("Collections with DOIs cannot be deleted.")
 
     collection.delete()
@@ -131,7 +131,7 @@ def collection_merge_magic_collections(
         if hasattr(src_collection, "cohort") and src_collection.cohort != dest_collection.cohort:
             logger.info("Abandoning cohort %s", src_collection.cohort.pk)
 
-        for field in ["public", "pinned", "doi", "locked"]:
+        for field in ["public", "pinned", "locked"]:
             dest_collection_value = getattr(dest_collection, field)
             collection_value = getattr(src_collection, field)
             if dest_collection_value != collection_value:

isic/core/services/collection/doi.py

@@ -1,6 +1,5 @@
 import logging
 from pathlib import Path
-import random
 from typing import TYPE_CHECKING, Any
 from urllib import parse
 
@@ -19,7 +18,6 @@
 from isic.core.services.collection import (
     collection_get_creators_in_attribution_order,
     collection_lock,
-    collection_update,
 )
 from isic.core.services.snapshot import snapshot_images
 from isic.core.tasks import (
@@ -101,17 +99,12 @@ def collection_build_draft_doi(*, doi_id: str) -> dict:
     }
 
 
-def collection_generate_random_doi_id():
-    # pad DOI with leading zeros so all DOIs are prefix/6 digits
-    return f"{settings.ISIC_DATACITE_DOI_PREFIX}/{random.randint(10_000, 999_999):06}"  # noqa: S311
-
-
 def collection_check_create_doi_allowed(
     *, user: User, collection: Collection, supplemental_files=None
 ) -> None:
     if not user.has_perm("core.create_doi", collection):
         raise ValidationError("You don't have permissions to do that.")
-    if collection.doi:
+    if hasattr(collection, "doi"):
         raise ValidationError("This collection already has a DOI.")
     if not collection.public:
         raise ValidationError("A collection must be public to issue a DOI.")
@@ -175,21 +168,13 @@ def collection_create_doi(*, user: User, collection: Collection, supplemental_fi
         user=user, collection=collection, supplemental_files=supplemental_files
     )
 
-    doi_id = collection_generate_random_doi_id()
-    draft_doi_dict = collection_build_draft_doi(doi_id=doi_id)
-    doi_dict = collection_build_doi(collection=collection, doi_id=doi_id)
-
     with transaction.atomic():
         # First, create the local DOI record to validate uniqueness within our known set
-        doi = Doi(
-            id=doi_id, slug=slugify(collection.name), creator=user, url=f"https://doi.org/{doi_id}"
-        )
+        doi = Doi(slug=slugify(collection.name), collection=collection, creator=user)
         doi.full_clean()
         doi.save()
 
-        # Lock the collection, set the DOI on it
         collection_lock(collection=collection)
-        collection_update(collection=collection, doi=doi, ignore_lock=True)
 
         if supplemental_files:
             for supplemental_file in supplemental_files:
@@ -200,17 +185,20 @@ def collection_create_doi(*, user: User, collection: Collection, supplemental_fi
                     size=supplemental_file["blob"].size,
                 )
 
+        draft_doi_dict = collection_build_draft_doi(doi_id=doi.id)
+        doi_dict = collection_build_doi(collection=collection, doi_id=doi.id)
+
         # Reserve the DOI using the draft mechanism.
         # If it fails, transaction will rollback, nothing in our database will change.
         _datacite_create_doi(draft_doi_dict)
 
     # Convert to a published DOI. If this fails, someone will have to come along later and
     # retry to publish it. (May want a django-admin action for this if it ever happens.)
-    _datacite_update_doi(doi_dict, doi_id)
+    _datacite_update_doi(doi_dict, doi.id)
 
-    create_doi_bundle_task.delay_on_commit(doi_id)
-    fetch_doi_citations_task.delay_on_commit(doi_id)
-    fetch_doi_schema_org_dataset_task.delay_on_commit(doi_id)
+    create_doi_bundle_task.delay_on_commit(doi.id)
+    fetch_doi_citations_task.delay_on_commit(doi.id)
+    fetch_doi_schema_org_dataset_task.delay_on_commit(doi.id)
 
     logger.info("User %d created DOI %s for collection %d", user.id, doi.id, collection.id)
 

isic/core/templates/core/collection_detail.html

@@ -38,7 +38,7 @@
         {% if collection.doi %}
           <li>
             <span class="font-bold">DOI:</span>
-            <a href="{{ collection.doi.url }}">{{ collection.doi.url }}</a>
+            <a href="{{ collection.doi.external_url }}">{{ collection.doi.external_url }}</a>
           </li>
         {% endif %}
         <li>