Fix: Occasional IntegrityError when saving 'new' assets (#30)

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - BYNDER_MAX_DOCUMENT_FILE_SIZE and BYNDER_MAX_IMAGE_FILE_SIZE settings to guard against memory spikes when downloading asset files ([#31]https://github.com/torchbox/wagtail-bynder/pull/31) @ababic
 - Automatic reformatting and downsizing of source images ([#32](https://github.com/torchbox/wagtail-bynder/pull/32)) @ababic
+- Improved clash detection/handling in choosen views ([#30](https://github.com/torchbox/wagtail-bynder/pull/30)) @ababic
 
 ## [0.5.1] - 2024-07-29
 

src/wagtail_bynder/views/document.py

@@ -56,7 +56,7 @@ def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
 
-class DocumentChosenView(chooser_views.DocumentChosenView, BynderAssetCopyMixin):
+class DocumentChosenView(BynderAssetCopyMixin, chooser_views.DocumentChosenView):
     model = get_document_model()
 
     def get(self, request: "HttpRequest", pk: str) -> "JsonResponse":

src/wagtail_bynder/views/image.py

@@ -54,7 +54,7 @@ def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
 
-class ImageChosenView(chooser_views.ImageChosenView, BynderAssetCopyMixin):
+class ImageChosenView(BynderAssetCopyMixin, chooser_views.ImageChosenView):
     model = get_image_model()
 
     def get(self, request: "HttpRequest", pk: str) -> "JsonResponse":

src/wagtail_bynder/views/mixins.py

@@ -1,6 +1,9 @@
-from typing import TYPE_CHECKING
+import contextlib
+
+from typing import TYPE_CHECKING, Any
 
 from django.conf import settings
+from django.db import IntegrityError
 from django.shortcuts import redirect
 
 from wagtail_bynder.models import BynderAssetMixin
@@ -12,17 +15,48 @@
 
 
 class BynderAssetCopyMixin:
+    model = type[BynderAssetMixin]
+
+    def setup(self, *args, **kwargs):
+        super().setup(*args, **kwargs)
+        bynder_client = get_bynder_client()
+        self.asset_client = bynder_client.asset_bank_client
+
+    def build_object_from_data(self, asset_data: dict[str, Any]) -> BynderAssetMixin:
+        obj = self.model(bynder_id=asset_data["id"])
+        obj.update_from_asset_data(asset_data)
+        return obj
+
     def create_object(self, asset_id: str) -> BynderAssetMixin:
-        client = get_bynder_client()
-        obj: BynderAssetMixin = self.model(bynder_id=asset_id)
-        data = client.asset_bank_client.media_info(asset_id)
-        obj.update_from_asset_data(data)
-        obj.save()
+        data = self.asset_client.media_info(asset_id)
+        obj = self.build_object_from_data(data)
+        try:
+            # If the asset finished saving in a different thread during the download/update process,
+            # return the pre-existing object
+            return self.model.objects.get(bynder_id=asset_id)
+        except self.model.DoesNotExist:
+            try:
+                # Save the new object, triggering transfer of the file to media storage
+                obj.save()
+            except IntegrityError as integrity_error:
+                # It's likely the asset finished saving in a different thread while the file was
+                # being transferred to media storage
+                try:
+                    # Lookup the existing object
+                    pre_existing = self.model.objects.get(bynder_id=asset_id)
+                except self.model.DoesNotExist:
+                    # The IntegrityError must have been caused by a custom field, so reraise
+                    raise integrity_error from None
+                else:
+                    # If the newly-downloaded file was successfully copied to storage, delete it
+                    with contextlib.suppress(ValueError, FileNotFoundError):
+                        if obj.file.path != pre_existing.file.path:
+                            obj.file.delete()
+                return pre_existing
         return obj
 
     def update_object(self, asset_id: str, obj: BynderAssetMixin) -> BynderAssetMixin:
-        client = get_bynder_client()
-        data = client.asset_bank_client.media_info(asset_id)
+        data = self.asset_client.media_info(asset_id)
         if not obj.is_up_to_date(data):
             obj.update_from_asset_data(data)
             obj.save()

src/wagtail_bynder/views/video.py

@@ -31,7 +31,7 @@ def page_subtitle(self):
         return self.model._meta.verbose_name
 
 
-class VideoChosenView(SnippetChosenView, BynderAssetCopyMixin):
+class VideoChosenView(BynderAssetCopyMixin, SnippetChosenView):
     def get(self, request: "HttpRequest", pk: str) -> "JsonResponse":
         try:
             obj = self.model.objects.get(bynder_id=pk)

tests/test_bynderassetcopymixin.py

@@ -2,8 +2,13 @@
 
 import responses
 
+from django.db import IntegrityError
+from django.http import HttpRequest
 from django.test import TestCase
-from wagtail.images import get_image_model
+from django.utils.functional import cached_property
+from django.views.generic.base import View
+from testapp.factories import CustomImageFactory
+from testapp.models import CustomImage
 from wagtail_factories import ImageFactory
 
 from wagtail_bynder.views.mixins import BynderAssetCopyMixin
@@ -17,8 +22,13 @@
 class BynderAssetCopyMixinTests(TestCase):
     def setUp(self):
         super().setUp()
-        self.view = BynderAssetCopyMixin()
-        self.view.model = get_image_model()
+        request = HttpRequest()
+        request.method = ""
+        request.path = "/"
+
+        self.view = self.view_class()
+        self.view.setup(request)
+
         # Mock out Bynder API calls
         responses.add(
             responses.GET,
@@ -27,6 +37,20 @@ def setUp(self):
             status=200,
         )
 
+    @cached_property
+    def view_class(self) -> type[BynderAssetCopyMixin]:
+        """
+        Use the mixin to create a functioning view class
+        that can be used in tests for this class.
+        """
+
+        class TestViewClass(BynderAssetCopyMixin, View):
+            """A basic view class utilising BynderAssetCopyMixin"""
+
+            model = CustomImage
+
+        return TestViewClass
+
     @responses.activate
     def test_create_object(self):
         # After fetching the data from bynder, the method should create a
@@ -94,3 +118,74 @@ def test_update_object_when_object_is_outdated(self):
         is_up_to_date_mock.assert_called_once_with(TEST_ASSET_DATA)
         update_from_asset_data_mock.assert_called_once_with(TEST_ASSET_DATA)
         save_mock.assert_called_once()
+
+    @responses.activate
+    def test_create_object_clash_handling_after_update(self):
+        # Create an image with a matching bynder_id
+        existing = CustomImageFactory.create(bynder_id=TEST_ASSET_ID)
+
+        # Trigger the test target directly
+        with mock.patch.object(
+            self.view.model, "update_from_asset_data"
+        ) as update_from_asset_data_mock:
+            result = self.view.create_object(TEST_ASSET_ID)
+
+        # Check behavior and result
+        update_from_asset_data_mock.assert_called_once_with(TEST_ASSET_DATA)
+        self.assertEqual(result, existing)
+
+    @responses.activate
+    def test_create_object_clash_handling_on_save(self):
+        def create_dupe_and_throw_integrity_error():
+            CustomImageFactory.create(bynder_id=TEST_ASSET_ID)
+            raise IntegrityError("bynder_id must be unique")
+
+        fake_obj = mock.MagicMock()
+        fake_obj.save = create_dupe_and_throw_integrity_error
+
+        with (
+            # Patch update_from_asset_data() to prevent download attempts
+            # and other unnecessary work
+            mock.patch.object(
+                self.view.model,
+                "update_from_asset_data",
+            ),
+            # Patch build_object_from_data to return a mock with a patched save() method
+            mock.patch.object(
+                self.view,
+                "build_object_from_data",
+                return_value=fake_obj,
+            ),
+        ):
+            # The IntegrityError should be captured, and the object
+            # that was saved previously should be found and returned
+            result = self.view.create_object(TEST_ASSET_ID)
+
+        self.assertEqual(result.bynder_id, TEST_ASSET_ID)
+
+    @responses.activate
+    def test_create_object_clash_handling_on_save_when_bynder_id_match_not_found(self):
+        def throw_integrity_error():
+            raise IntegrityError("Some other field must be unique")
+
+        fake_obj = mock.MagicMock()
+        fake_obj.save = throw_integrity_error
+
+        with (
+            # Patch update_from_asset_data() to prevent download attempts
+            # and other unnecessary work
+            mock.patch.object(
+                self.view.model,
+                "update_from_asset_data",
+            ),
+            # Patch build_object_from_data to return a mock with a patched save() method
+            mock.patch.object(
+                self.view,
+                "build_object_from_data",
+                return_value=fake_obj,
+            ),
+            self.assertRaises(IntegrityError),
+        ):
+            # With no 'bynder_id' match to be found, the error is allowed
+            # to bubble up
+            self.view.create_object(TEST_ASSET_ID)