Skip to content

Commit

Permalink
FIX: Cache values on model instead of common field instance
Browse files Browse the repository at this point in the history
  • Loading branch information
thclark committed Jun 20, 2023
1 parent 67e6324 commit 7ce2633
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 9 deletions.
12 changes: 6 additions & 6 deletions django_gcp/storage/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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, {})
return value_or_dict.get("path", None)

def _get_fieldname(self, instance):
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
38 changes: 36 additions & 2 deletions tests/test_storage_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)

0 comments on commit 7ce2633

Please sign in to comment.