Merge pull request #84 from octue/override-clean

Add uploaded_blob context manager, allow calling of model clean() with overridden settings

Author: thclark

.devcontainer/devcontainer.json

@@ -26,7 +26,6 @@
         "python.testing.pytestEnabled": true,
         "python.testing.unittestEnabled": false,
         "terminal.integrated.defaultProfile.linux": "zsh",
-        "ruff.format.args": ["--line-length", "120"],
         "ruff.importStrategy": "fromEnvironment"
       },
 

django_gcp/storage/__init__.py

@@ -3,7 +3,7 @@
 from .blob_utils import BlobFieldMixin, get_blob, get_blob_name, get_path, get_signed_url
 from .fields import BlobField
 from .gcloud import GoogleCloudFile, GoogleCloudMediaStorage, GoogleCloudStaticStorage, GoogleCloudStorage
-from .operations import upload_blob
+from .operations import upload_blob, uploaded_blob
 
 __all__ = [
     "BlobField",
@@ -18,4 +18,5 @@
     "get_path",
     "get_signed_url",
     "upload_blob",
+    "uploaded_blob",
 ]

django_gcp/storage/fields.py

@@ -199,125 +199,125 @@ def clean(self, value, model_instance, skip_validation=False):
         """Clean the value to ensure correct state and on_commit hooks.
         This method is called during clean_fields on model save.
         """
-        if not skip_validation and not self._validated:
-            self.validate(value, model_instance)
 
-        try:
-            # Cross-field validation: ensure value is not greater than another field
-            add = model_instance._state.adding
+        # An escape hatch allowing you to set the path directly. This is dangerous and should only be done
+        # explicitly for the purpose of data migration and manipulation. You should never allow an untrusted
+        # client to set paths directly, because knowing the path of a pre-existing object allows you to assume
+        # access to it. Tip: You can use django's override_settings context manager to set this temporarily.
+        if getattr(
+            settings,
+            "GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE",
+            DEFAULT_OVERRIDE_BLOBFIELD_VALUE,
+        ):
+            logger.warning(
+                "Overriding %s value to %s",
+                self.__class__.__name__,
+                value,
+            )
+            new_value = value
+
+        else:
+            # Validate the value given
+            if not skip_validation and not self._validated:
+                self.validate(value, model_instance)
 
+            # Clean {} --> None
             value = self._clean_blank_value(getattr(model_instance, self.attname))
 
+            # Get model state
+            add = model_instance._state.adding
             existing_path = None if add else self._get_existing_path(model_instance)
 
-            # An escape hatch allowing you to set the path directly. This is dangerous and should only be done
-            # explicitly for the purpose of data migration and manipulation. You should never allow an untrusted
-            # client to set paths directly, because knowing the path of a pre-existing object allows you to assume
-            # access to it. Tip: You can use django's override_settings context manager to set this temporarily.
-            if getattr(
-                settings,
-                "GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE",
-                DEFAULT_OVERRIDE_BLOBFIELD_VALUE,
-            ):
-                logger.warning(
-                    "Overriding %s value to %s",
-                    self.__class__.__name__,
-                    value,
+            # There are six scenarios to deal with:
+            adding_blank = add and value is None
+            adding_valid = add and value is not None
+            updating_valid_to_valid = not add and self._get_valid_to_valid(model_instance)
+            updating_valid_to_blank = not add and self._get_valid_to_blank(model_instance)
+            updating_blank_to_valid = not add and self._get_blank_to_valid(model_instance)
+            unchanged = not add and self._get_unchanged(model_instance)
+
+            # Branch based on scenarios
+            if unchanged:
+                new_value = {"path": value["path"]} if value is not None else None
+
+            elif adding_blank or updating_valid_to_blank:
+                new_value = None
+
+                # Trigger the on_change callback at the end of the commit when we know the
+                # database transaction will work
+                def on_commit_blank():
+                    if self.on_change is not None:
+                        self.on_change(new_value, instance=model_instance)
+
+                # clean may happen outside transations so we must
+                # actually register the transaction during pre-save,
+                # store the callback here temporarily
+                self._on_commit_blank = on_commit_blank
+
+            elif adding_valid or updating_blank_to_valid or updating_valid_to_valid:
+                new_value = {}
+
+                allow_overwrite = self._get_allow_overwrite(add)
+
+                attributes = self._update_attributes(
+                    getattr(value, "attributes", {}),
+                    instance=model_instance,
+                    original_name=value["name"],
+                    existing_path=existing_path,
+                    temporary_path=value["_tmp_path"],
+                    adding=add,
+                    bucket=self.storage.bucket,
                 )
-                new_value = value
-            else:
-                # There are six scenarios to deal with:
-                adding_blank = add and value is None
-                adding_valid = add and value is not None
-                updating_valid_to_valid = not add and self._get_valid_to_valid(model_instance)
-                updating_valid_to_blank = not add and self._get_valid_to_blank(model_instance)
-                updating_blank_to_valid = not add and self._get_blank_to_valid(model_instance)
-                unchanged = not add and self._get_unchanged(model_instance)
-
-                if unchanged:
-                    new_value = {"path": value["path"]} if value is not None else None
-
-                elif adding_blank or updating_valid_to_blank:
-                    new_value = None
-
-                    # Trigger the on_change callback at the end of the commit when we know the
-                    # database transaction will work
-                    def on_commit_blank():
-                        if self.on_change is not None:
-                            self.on_change(new_value, instance=model_instance)
-
-                    # clean may happen outside transations so we must
-                    # actually register the transaction during pre-save,
-                    # store the callback here temporarily
-                    self._on_commit_blank = on_commit_blank
-
-                elif adding_valid or updating_blank_to_valid or updating_valid_to_valid:
-                    new_value = {}
-
-                    allow_overwrite = self._get_allow_overwrite(add)
-
-                    attributes = self._update_attributes(
-                        getattr(value, "attributes", {}),
-                        instance=model_instance,
-                        original_name=value["name"],
-                        existing_path=existing_path,
-                        temporary_path=value["_tmp_path"],
-                        adding=add,
-                        bucket=self.storage.bucket,
-                    )
 
-                    new_value["path"], allow_overwrite = self._get_destination_path(
-                        instance=model_instance,
-                        original_name=value["name"],
-                        attributes=attributes,
-                        allow_overwrite=self._get_allow_overwrite(add),
-                        existing_path=existing_path,
-                        temporary_path=value["_tmp_path"],
-                        bucket=self.storage.bucket,
-                    )
+                new_value["path"], allow_overwrite = self._get_destination_path(
+                    instance=model_instance,
+                    original_name=value["name"],
+                    attributes=attributes,
+                    allow_overwrite=self._get_allow_overwrite(add),
+                    existing_path=existing_path,
+                    temporary_path=value["_tmp_path"],
+                    bucket=self.storage.bucket,
+                )
 
-                    logger.info(
-                        "Adding/updating cloud object via temporary ingress at %s to %s",
-                        value["_tmp_path"],
-                        new_value["path"],
-                    )
+                logger.info(
+                    "Adding/updating cloud object via temporary ingress at %s to %s",
+                    value["_tmp_path"],
+                    new_value["path"],
+                )
 
-                    # Trigger the copy only on successful commit of the transaction. We have to
-                    # capture the dual edge cases of the file not moving correctly, and the database
-                    # row not saving (eg due to validation errors in other model fields).
-                    # https://stackoverflow.com/questions/33180727/trigering-post-save-signal-only-after-transaction-has-completed
-                    def on_commit_valid():
-                        copy_blob(
-                            self.storage.bucket,
-                            value["_tmp_path"],
-                            self.storage.bucket,
-                            new_value["path"],
-                            move=True,
-                            overwrite=allow_overwrite,
-                            attributes=attributes,
-                        )
-                        if self.on_change is not None:
-                            self.on_change(new_value, instance=model_instance)
-
-                    logger.info(
-                        "Registered move of %s to %s to happen on transaction commit",
+                # Trigger the copy only on successful commit of the transaction. We have to
+                # capture the dual edge cases of the file not moving correctly, and the database
+                # row not saving (eg due to validation errors in other model fields).
+                # https://stackoverflow.com/questions/33180727/trigering-post-save-signal-only-after-transaction-has-completed
+                def on_commit_valid():
+                    copy_blob(
+                        self.storage.bucket,
                         value["_tmp_path"],
+                        self.storage.bucket,
                         new_value["path"],
+                        move=True,
+                        overwrite=allow_overwrite,
+                        attributes=attributes,
                     )
-                    self._on_commit_valid = on_commit_valid
+                    if self.on_change is not None:
+                        self.on_change(new_value, instance=model_instance)
 
-                else:
-                    # Raise unknown edge cases rather than failing silently
-                    raise ValueError(
-                        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)}"
-                    )
+                logger.info(
+                    "Registered move of %s to %s to happen on transaction commit",
+                    value["_tmp_path"],
+                    new_value["path"],
+                )
+                self._on_commit_valid = on_commit_valid
 
-            # Cache DB values in the instance so you can reuse it without multiple DB queries
-            # pylint: disable-next=protected-access
-            model_instance._state.fields_cache[self.attname] = new_value
+            else:
+                # Raise unknown edge cases rather than failing silently
+                raise ValueError(
+                    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)}"
+                )
 
-        except ValidationError as e:
-            raise e
+        # Cache DB values in the instance so you can reuse it without multiple DB queries
+        # pylint: disable-next=protected-access
+        model_instance._state.fields_cache[self.attname] = new_value
 
         self._cleaned = True
 

django_gcp/storage/operations.py

@@ -1,6 +1,8 @@
+from contextlib import contextmanager
 import datetime
 import logging
 
+from django.test.utils import override_settings
 from django.utils import timezone
 from google.cloud.exceptions import NotFound, PreconditionFailed
 from google.cloud.storage.blob import Blob
@@ -54,8 +56,9 @@ def upload_blob(
             instance,
             original_name=local_path,
             attributes=attributes,
-            allow_overwrite=allow_overwrite,
             existing_path=existing_path,
+            temporary_path=None,
+            allow_overwrite=allow_overwrite,
             bucket=field.storage.bucket,
         )
 
@@ -195,6 +198,38 @@ def get_signed_upload_url(bucket, blob_name, timedelta=None, max_size_bytes=None
     if max_size_bytes is not None:
         content_length_range = f"0,{max_size_bytes}"
         headers = kwargs.pop("headers", {})
-        headers["X-Goog-Content-Length-Range"]: content_length_range
+        headers["X-Goog-Content-Length-Range"] = content_length_range
+        kwargs["headers"] = headers
 
     return blob.generate_signed_url(expiration=timezone.now() + timedelta, method="PUT", **kwargs)
+
+
+@contextmanager
+def uploaded_blob(instance, field_name, local_path, destination_path=None, allow_overwrite=False, delete_on_exit=False):
+    """A context manager enabling the preparation of an instance with a blob uploaded from a local path.
+    Optionally override the destination path (the ultimate path of the blob within the destination bucket) and allow_overwrite parameter.
+
+    Usage:
+
+    ```py
+    my_instance = MyModel()
+    with uploaded_blob(my_instance, 'my_field', '/path/to/local/file') as field_value
+        my_instance.my_field = field_value
+        my_instance.save()
+    ```
+
+    """
+
+    with override_settings(GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True):
+        field_value = upload_blob(
+            instance,
+            field_name=field_name,
+            local_path=local_path,
+            destination_path=destination_path,
+            allow_overwrite=allow_overwrite,
+        )
+        try:
+            yield field_value
+        finally:
+            if delete_on_exit:
+                raise NotImplementedError("Deletion on exit is not yet implemented")

pyproject.toml

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

tests/test_storage_fields.py

@@ -111,14 +111,25 @@ def test_add_view_has_presigned_url(self):
             widget.signed_ingress_url.startswith("https://storage.googleapis.com/example-media-assets/_tmp")
         )
 
+    def test_full_clean_executes_in_overridden_context(self):
+        """Ensure that full_clean() can be legitimately called
+        on a model while in an overridden context
+        """
+
+        blob_name = self._prefix_blob_name("test_full_clean_executes_in_overridden_context.txt")
+        self._create_test_blob(self.bucket, blob_name, "")
+        with override_settings(GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True):
+            obj = ExampleBlobFieldModel(blob={"path": blob_name})
+            obj.full_clean()
+            obj.save()
+
     @override_settings(GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True)
     def test_change_view_loads_normally(self):
         """Ensure we can load a change view"""
 
         blob_name = self._prefix_blob_name("test_change_view_loads_normally.txt")
         self._create_test_blob(self.bucket, blob_name, "")
-        with override_settings(GCP_STORAGE_OVERRIDE_BLOBFIELD_VALUE=True):
-            obj = ExampleBlobFieldModel.objects.create(blob={"path": blob_name})
+        obj = ExampleBlobFieldModel.objects.create(blob={"path": blob_name})
 
         # Assert that the view loads
         response = self.client.get(get_admin_change_view_url(obj))