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

dandiapi/api/services/asset/__init__.py

@@ -44,6 +44,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.status = Version.Status.PENDING
+    # Save the version so that the modified field is updated
+    version.save()
+
+    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.status = Version.Status.PENDING
+    # Save the version so that the modified field is updated
+    version.save()
+
+
 def change_asset(  # noqa: PLR0913
     *,
     user,
@@ -85,16 +118,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 +135,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:
@@ -142,57 +172,39 @@ def add_asset_to_version(  # noqa: PLR0913, C901
 
     # Creating an asset in an OPEN dandiset that points to an embargoed blob results in that
     # blob being unembargoed
-    unembargo_asset_blob = (
+    should_unembargo = (
         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:
+        if should_unembargo:
             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