From 1298ea2cb1a0c35631069757e47b7a76dfa4cf39 Mon Sep 17 00:00:00 2001
From: Jacob Nesbitt <jjnesbitt2@gmail.com>
Date: Fri, 29 Dec 2023 18:10:17 -0500
Subject: [PATCH] Don't use .save() in validate_version_metadata

---
 dandiapi/api/services/metadata/__init__.py | 41 ++++++++++++----------
 dandiapi/api/tests/test_tasks.py           | 33 ++++++++++++++++-
 2 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/dandiapi/api/services/metadata/__init__.py b/dandiapi/api/services/metadata/__init__.py
index b6a4c3409..77f5ea642 100644
--- a/dandiapi/api/services/metadata/__init__.py
+++ b/dandiapi/api/services/metadata/__init__.py
@@ -129,36 +129,39 @@ def _build_validatable_version_metadata(version: Version) -> dict:
     with transaction.atomic():
         # validating version metadata needs to lock the version to avoid racing with
         # other modifications e.g. aggregate_assets_summary.
-        version = (
-            Version.objects.filter(id=version.id, status=Version.Status.PENDING)
-            .select_for_update()
-            .first()
-        )
-        version.status = Version.Status.VALIDATING
-        version.save()
+        version_qs = Version.objects.filter(id=version.id).select_for_update()
+        if not version_qs.filter(status=Version.Status.PENDING).exists():
+            # TODO: Should raise an error here?
+            return
 
+        # Set to validating and continue
+        version_qs.update(status=Version.Status.VALIDATING)
         try:
             validate(
-                _build_validatable_version_metadata(version),
+                _build_validatable_version_metadata(version_qs.first()),
                 schema_key='PublishedDandiset',
                 json_validation=True,
             )
         except dandischema.exceptions.ValidationError as e:
             logger.info('Error while validating version %s', version.id)
-            version.status = Version.Status.INVALID
-
-            validation_errors = _collect_validation_errors(e)
-            version.validation_errors = validation_errors
-            version.save()
+            version_qs.update(
+                status=Version.Status.INVALID,
+                validation_errors=_collect_validation_errors(e),
+                modified=timezone.now(),
+            )
             return
         except ValueError as e:
             # A bare ValueError is thrown when dandischema generates its own exceptions, like a
             # mismatched schemaVersion.
-            version.status = Version.Status.INVALID
-            version.validation_errors = [{'field': '', 'message': str(e)}]
-            version.save()
+            logger.info('Error while validating version %s', version.id)
+            version_qs.update(
+                status=Version.Status.INVALID,
+                validation_errors=[{'field': '', 'message': str(e)}],
+                modified=timezone.now(),
+            )
             return
+
         logger.info('Successfully validated version %s', version.id)
-        version.status = Version.Status.VALID
-        version.validation_errors = []
-        version.save()
+        version_qs.update(
+            status=Version.Status.VALID, validation_errors=[], modified=timezone.now()
+        )
diff --git a/dandiapi/api/tests/test_tasks.py b/dandiapi/api/tests/test_tasks.py
index d5dcb3056..cd9066ea1 100644
--- a/dandiapi/api/tests/test_tasks.py
+++ b/dandiapi/api/tests/test_tasks.py
@@ -4,6 +4,8 @@
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core.files.storage import Storage
+from django.forms.models import model_to_dict
+from django.utils import timezone
 from guardian.shortcuts import assign_perm
 import pytest
 from rest_framework.test import APIClient
@@ -185,13 +187,42 @@ def test_validate_asset_metadata_saves_version(draft_asset: Asset, draft_version
 def test_validate_version_metadata(draft_version: Version, asset: Asset):
     draft_version.assets.add(asset)
 
-    tasks.validate_version_metadata_task(draft_version.id)
+    # Bypass .save to manually set an older timestamp
+    old_modified = timezone.now() - datetime.timedelta(minutes=10)
+    updated = Version.objects.filter(id=draft_version.id).update(modified=old_modified)
+    assert updated == 1
 
+    tasks.validate_version_metadata_task(draft_version.id)
     draft_version.refresh_from_db()
 
     assert draft_version.status == Version.Status.VALID
     assert draft_version.validation_errors == []
 
+    # Ensure modified field was updated
+    assert draft_version.modified > old_modified
+
+
+@pytest.mark.django_db()
+def test_validate_version_metadata_no_side_effects(draft_version: Version, asset: Asset):
+    draft_version.assets.add(asset)
+
+    # Set the version `status` and `validation_errors` fields to something
+    # which can't be a result of validate_version_metadata
+    draft_version.status = Version.Status.PENDING
+    draft_version.validation_errors = [{'foo': 'bar'}]
+    draft_version.save()
+
+    # Validate version metadata, storing the model data before and after
+    old_data = model_to_dict(draft_version)
+    tasks.validate_version_metadata_task(draft_version.id)
+    draft_version.refresh_from_db()
+    new_data = model_to_dict(draft_version)
+
+    # Check that change is isolated to these two fields
+    assert old_data.pop('status') != new_data.pop('status')
+    assert old_data.pop('validation_errors') != new_data.pop('validation_errors')
+    assert old_data == new_data
+
 
 @pytest.mark.django_db()
 def test_validate_version_metadata_malformed_schema_version(draft_version: Version, asset: Asset):