Merge pull request #42 from octue/destroy-ganon-rescue-princess

Destroy Ganon, Rescue Princess

Author: thclark

django_gcp/storage/fields.py

@@ -279,8 +279,8 @@ def on_commit_valid():
                     f"Unable to determine field state for {self._get_fieldname(model_instance)}. The most likely cause of this doing an operation (like migration) without setting GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True. Otherwise, please contact the django_gcp developers and describe what you're doing along with this exception stacktrace. Value was: {json.dumps(value)}"
                 )
 
-        # Pop cached DB values so you can re-use an instantiated form after saving it
-        self._existing_value = {"_cached": new_value}
+        # Cache DB values in the instance so you can reuse it without multiple DB queries
+        model_instance._state.fields_cache[self.attname] = new_value
 
         return new_value
 
@@ -478,14 +478,14 @@ def _get_existing_path(self, instance):
         """Gets the existing (saved in db) path; will raise an error if the instance is unsaved.
         Caches value on the instance to avoid duplicate database calls within a transaction.
         """
-        if self._existing_value is None:
-            # pylint: disable-next=protected-access
+        # pylint: disable=protected-access
+        if self.attname not in instance._state.fields_cache.keys():
             pk_name = instance._meta.pk.name
             # pylint: disable-next=protected-access
             existing = instance.__class__._default_manager.get(**{pk_name: getattr(instance, pk_name)})
             # Cache value in a dict because None is a valid value
-            self._existing_value = {"_cached": getattr(existing, self.attname)}
-        value_or_dict = self._existing_value["_cached"] or {}
+            instance._state.fields_cache[self.attname] = getattr(existing, self.attname)
+        value_or_dict = instance._state.fields_cache.get(self.attname, None) or {}
         return value_or_dict.get("path", None)
 
     def _get_fieldname(self, instance):

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "django-gcp"
-version = "0.10.1"
+version = "0.10.2"
 description = "Utilities to run Django on Google Cloud Platform"
 authors = ["Tom Clark"]
 license = "MIT"

tests/test_storage_fields.py

@@ -314,8 +314,8 @@ def test_update_valid_unchanged(self):
 
 
 class TestCallableBlobField(BlobModelFactoryMixin, StorageOperationsMixin, TestCase):
-    """Inherits from transaction test case, because we use an on_commit
-    hook to move ingressed files once a database save has been made.
+    """Test the callbacks functionality.
+    Use normal test case, allowing on_commit hook to execute.
     """
 
     def test_on_change_callback_execution(self):
@@ -366,3 +366,37 @@ def test_on_change_callback_execution(self):
                 )
                 instance = form.save()
                 self.assertEqual(mock_on_commit.call_count, 4)
+
+
+class TestModelBlobField(BlobModelFactoryMixin, StorageOperationsMixin, TestCase):
+    """Extra tests for corner cases and model behaviour"""
+
+    def test_no_corruption_with_multiple_models(self):
+        """Tests for the presence of a nasty corner case where
+        multiple models loaded into memory at once would conflict in
+        their behaviour when determining the existing_path
+        """
+
+        blob_name_1 = self._prefix_blob_name("no_corruption_1.txt")
+        blob_name_2 = self._prefix_blob_name("no_corruption_2.txt")
+        # self._create_test_blob(self.bucket, blob_name_1, "")
+        # self._create_test_blob(self.bucket, blob_name_2, "")
+
+        # Create two instances
+        with override_settings(GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True):
+            obj_1 = ExampleBlobFieldModel.objects.create(blob={"path": blob_name_1})
+            obj_1.save()
+            # In the captured behaviour, the cache of the second instance overwrite the first
+            obj_2 = ExampleBlobFieldModel.objects.create(blob={"path": blob_name_2})
+            obj_2.save()
+
+        # Edit the first, without changing the KMZ field (and outside the settings override)
+        new_obj_1 = ExampleBlobFieldModel.objects.get(id=obj_1.id)
+        new_obj_1.category = "test-change"
+
+        # Note this is not a blankable model, and the observed flaw in flushing
+        # the cache in this case manifests in passing a None value instead of
+        # the correct existing path being passed through. This save() thus
+        # resulted in an IntegrityError.
+        new_obj_1.save()
+        self.assertEqual(new_obj_1.blob["path"], blob_name_1)