Merge pull request #1991 from dandi/refactor-asset-service-layer

Separate core model logic from top-level asset service layer functions

dandiapi/api/services/asset/__init__.py

@@ -3,6 +3,7 @@
 from typing import TYPE_CHECKING
 
 from django.db import transaction
+from django.utils import timezone
 
 from dandiapi.api.asset_paths import add_asset_paths, delete_asset_paths, get_conflicting_paths
 from dandiapi.api.models.asset import Asset, AssetBlob
@@ -44,6 +45,39 @@ def _create_asset(
     return asset
 
 
+def _add_asset_to_version(
+    *,
+    version: Version,
+    asset_blob: AssetBlob | None,
+    zarr_archive: ZarrArchive | None,
+    metadata: dict,
+) -> Asset:
+    path = metadata['path']
+    asset = _create_asset(
+        path=path, asset_blob=asset_blob, zarr_archive=zarr_archive, metadata=metadata
+    )
+    version.assets.add(asset)
+    add_asset_paths(asset, version)
+
+    # Trigger a version metadata validation, as saving the version might change the metadata
+    Version.objects.filter(id=version.id).update(
+        status=Version.Status.PENDING, modified=timezone.now()
+    )
+
+    return asset
+
+
+def _remove_asset_from_version(*, asset: Asset, version: Version):
+    # Remove asset paths and asset itself from version
+    delete_asset_paths(asset, version)
+    version.assets.remove(asset)
+
+    # Trigger a version metadata validation, as saving the version might change the metadata
+    Version.objects.filter(id=version.id).update(
+        status=Version.Status.PENDING, modified=timezone.now()
+    )
+
+
 def change_asset(  # noqa: PLR0913
     *,
     user,
@@ -85,16 +119,14 @@ def change_asset(  # noqa: PLR0913
         raise AssetAlreadyExistsError
 
     with transaction.atomic():
-        remove_asset_from_version(user=user, asset=asset, version=version, do_audit=False)
-
-        new_asset = add_asset_to_version(
-            user=user,
+        _remove_asset_from_version(asset=asset, version=version)
+        new_asset = _add_asset_to_version(
             version=version,
             asset_blob=new_asset_blob,
             zarr_archive=new_zarr_archive,
             metadata=new_metadata,
-            do_audit=False,
         )
+
         # Set previous asset and save
         new_asset.previous = asset
         new_asset.save()
@@ -104,14 +136,13 @@ def change_asset(  # noqa: PLR0913
     return new_asset, True
 
 
-def add_asset_to_version(  # noqa: PLR0913, C901
+def add_asset_to_version(
     *,
     user,
     version: Version,
     asset_blob: AssetBlob | None = None,
     zarr_archive: ZarrArchive | None = None,
     metadata: dict,
-    do_audit: bool = True,
 ) -> Asset:
     """Create an asset, adding it to a version."""
     if not asset_blob and not zarr_archive:
@@ -140,59 +171,39 @@ def add_asset_to_version(  # noqa: PLR0913, C901
     if zarr_archive and zarr_archive.dandiset != version.dandiset:
         raise ZarrArchiveBelongsToDifferentDandisetError
 
-    # Creating an asset in an OPEN dandiset that points to an embargoed blob results in that
-    # blob being unembargoed
-    unembargo_asset_blob = (
-        asset_blob is not None
-        and asset_blob.embargoed
-        and version.dandiset.embargo_status == Dandiset.EmbargoStatus.OPEN
-    )
     with transaction.atomic():
-        if asset_blob and unembargo_asset_blob:
+        # Creating an asset in an OPEN dandiset that points to an embargoed blob results in that
+        # blob being unembargoed
+        if (
+            asset_blob is not None
+            and asset_blob.embargoed
+            and version.dandiset.embargo_status == Dandiset.EmbargoStatus.OPEN
+        ):
             asset_blob.embargoed = False
             asset_blob.save()
+            transaction.on_commit(
+                lambda: remove_asset_blob_embargoed_tag_task.delay(blob_id=asset_blob.blob_id)
+            )
 
-        asset = _create_asset(
-            path=path, asset_blob=asset_blob, zarr_archive=zarr_archive, metadata=metadata
+        asset = _add_asset_to_version(
+            version=version,
+            asset_blob=asset_blob,
+            zarr_archive=zarr_archive,
+            metadata=metadata,
         )
-        version.assets.add(asset)
-        add_asset_paths(asset, version)
-
-        # Trigger a version metadata validation, as saving the version might change the metadata
-        version.status = Version.Status.PENDING
-        # Save the version so that the modified field is updated
-        version.save()
-
-        if do_audit:
-            audit.add_asset(dandiset=version.dandiset, user=user, asset=asset)
-
-    # Perform this after the above transaction has finished, to ensure we only
-    # operate on unembargoed asset blobs
-    if asset_blob and unembargo_asset_blob:
-        remove_asset_blob_embargoed_tag_task.delay(blob_id=asset_blob.blob_id)
+        audit.add_asset(dandiset=version.dandiset, user=user, asset=asset)
 
     return asset
 
 
-def remove_asset_from_version(
-    *, user, asset: Asset, version: Version, do_audit: bool = True
-) -> Version:
+def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Version:
     if not user.has_perm('owner', version.dandiset):
         raise DandisetOwnerRequiredError
     if version.version != 'draft':
         raise DraftDandisetNotModifiableError
 
     with transaction.atomic():
-        # Remove asset paths and asset itself from version
-        delete_asset_paths(asset, version)
-        version.assets.remove(asset)
-
-        # Trigger a version metadata validation, as saving the version might change the metadata
-        version.status = Version.Status.PENDING
-        # Save the version so that the modified field is updated
-        version.save()
-
-        if do_audit:
-            audit.remove_asset(dandiset=version.dandiset, user=user, asset=asset)
+        _remove_asset_from_version(asset=asset, version=version)
+        audit.remove_asset(dandiset=version.dandiset, user=user, asset=asset)
 
     return version

dandiapi/api/tests/test_asset.py

@@ -665,7 +665,7 @@ def test_asset_create_unembargo_in_progress(
     assert resp.status_code == 400
 
 
-@pytest.mark.django_db()
+@pytest.mark.django_db(transaction=True)
 def test_asset_create_embargoed_asset_blob_open_dandiset(
     api_client, user, draft_version, embargoed_asset_blob, mocker
 ):