chore: Fix save behavior for Django 3 and drop Django 2 support (#89)

* fix BoundEndpointManager get_response saves
* use EndpointResourceAttribute on resource
* preserve type arg on ResourceEndpointDefinition
* Merge __init__ methods
* Fix typo
* update fields in memory that changed on save to the database
* Fix getattr when attr doesn't exist
* reset_state
* need to manage Django model state
* check_relationship=False for the in-memory update
* deal with foreign keys in update_or_create
* Guard against type=None
* django3.2 moved an exception
* Add a couple of docstrings
* Don't LYIL
* Don't bother with old import
* Drop Django 2 dependency
* Drop Django 2 testing
* Update CHANGELOG

Co-authored-by: Drew Shafer <[email protected]>

Author: bgrant

.github/workflows/tests.yml

@@ -9,7 +9,6 @@ jobs:
     strategy:
       matrix:
         python-version: ['3.7', '3.8', '3.9', '3.10']
-        django: ['2.2', '3.2']
 
     steps:
     - uses: actions/checkout@v3

CHANGELOG.md

@@ -16,6 +16,7 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 ### Fixed
 - [PR 95](https://github.com/salesforce/django-declarative-apis/pull/95) Fill in missing CHANGELOG entries
 - [PR 90](https://github.com/salesforce/django-declarative-apis/pull/90) Fix `BoundEndpointManager` `get_response` saves
+- [PR 89](https://github.com/salesforce/django-declarative-apis/pull/89) Fix save behavior for Django 3 and drop Django 2 support
 
 ### Changed
 - [PR 93](https://github.com/salesforce/django-declarative-apis/pull/93) Remove spaces in name of `test` GitHub Action

django_declarative_apis/machinery/__init__.py

@@ -14,6 +14,8 @@
 from django.conf import settings
 from django.http import HttpResponse
 
+from dirtyfields.dirtyfields import reset_state
+
 from django_declarative_apis.machinery.filtering import apply_filters_to_object
 from django_declarative_apis.models import BaseConsumer
 from django_declarative_apis.resources.utils import HttpStatusCode
@@ -36,7 +38,7 @@
     ResourceField,
 )
 
-# these imports are unusued in this file but may be used in other projects
+# these imports are unused in this file but may be used in other projects
 # that use `machinery` as an interface
 from .attributes import TypedEndpointAttributeMixin, RequestFieldGroup  # noqa
 from .utils import locate_object, rate_limit_exceeded
@@ -84,7 +86,7 @@ def resource(self):
                 return Todo.objects.get(id=self.resource_id)
     """
 
-    def __init__(self, type, filter=None, returns_list=False, **kwargs):
+    def __init__(self, type=None, filter=None, returns_list=False, **kwargs):
         super().__init__(**kwargs)
         self.type = type
         self.filter = filter
@@ -100,10 +102,12 @@ def get_instance_value(self, owner_instance, owner_class):
             return self
         try:
             value = self.func(owner_instance)
-        except django.core.exceptions.ObjectDoesNotExist:
-            raise errors.ClientErrorNotFound(
-                "{0} instance not found".format(self.type.__name__)
-            )
+        except django.core.exceptions.ObjectDoesNotExist as e:  # noqa: F841
+            try:
+                message = f"{self.type.__name__} instance not found"
+            except AttributeError as e:  # noqa: F841
+                message = "Resource instance not found"
+            raise errors.ClientErrorNotFound(message)
 
         if value.__class__ == dict:
             return value
@@ -173,6 +177,35 @@ def __init__(cls, class_name, bases=None, dict=None):
                 pass
 
 
+def current_dirty_dict(resource):
+    """Get the `current` (in-memory) values for fields that have not yet been written to the database."""
+    new_data = resource.get_dirty_fields(check_relationship=True, verbose=True)
+    field_name_to_att_name = {f.name: f.attname for f in resource._meta.concrete_fields}
+    return {
+        field_name_to_att_name[key]: values["current"]
+        for key, values in new_data.items()
+    }
+
+
+def update_dirty(resource):
+    """Write dirty fields to the database."""
+    dirty_dict = current_dirty_dict(resource)
+    resource_next, created = type(resource).objects.update_or_create(
+        pk=resource.pk, defaults=dirty_dict
+    )
+
+    # update fields in memory that changed on save to the database
+    field_name_to_att_name = {f.name: f.attname for f in resource._meta.concrete_fields}
+    for k, v in resource_next._as_dict(check_relationship=True).items():
+        att_key = field_name_to_att_name[k]
+        if getattr(resource, att_key, None) != v:
+            setattr(resource, att_key, v)
+    resource._state.adding = False
+    resource._state.db = resource_next._state.db
+    resource._state.fields_cache = {}
+    reset_state(type(resource), resource)
+
+
 class EndpointBinder:
     class BoundEndpointManager:
         def __init__(self, manager, bound_endpoint):
@@ -197,7 +230,7 @@ def get_response(self):  # noqa: C901
 
             if hasattr(resource, "is_dirty"):
                 if resource and resource.is_dirty(check_relationship=True):
-                    resource.save()
+                    update_dirty(resource)
 
             endpoint_tasks = sorted(
                 self.manager.endpoint_tasks, key=lambda t: t.priority
@@ -213,13 +246,18 @@ def get_response(self):  # noqa: C901
                     immediate_task.run(self.bound_endpoint)
 
             except errors.ClientError as ce:
-                if ce.save_changes and resource and resource.is_dirty():
-                    resource.save()
+                if (
+                    ce.save_changes
+                    and resource
+                    and hasattr(resource, "is_dirty")
+                    and resource.is_dirty()
+                ):
+                    update_dirty(resource)
                 raise
 
             if hasattr(resource, "is_dirty"):
                 if resource and resource.is_dirty(check_relationship=True):
-                    resource.save()
+                    update_dirty(resource)
 
             for deferred_task in deferred_tasks:
                 deferred_task.run(self.bound_endpoint)
@@ -904,10 +942,10 @@ class ResourceEndpointDefinition(EndpointDefinition):
     """
 
     def __init__(self, *args, **kwargs):
-        super().__init__()
+        super().__init__(*args, **kwargs)
         self._cached_resource = None
 
-    @property
+    @EndpointResourceAttribute()
     def resource(self):
         """Queries the object manager of `self.resource_model` for the given id
         (`self.resource_id`).

django_declarative_apis/machinery/filtering.py

@@ -11,6 +11,7 @@
 
 from django.db import models
 from django.db.models import ManyToOneRel
+from django.core.exceptions import FieldDoesNotExist
 
 NEVER = 0
 ALWAYS = 1
@@ -80,7 +81,7 @@ def _get_filtered_field_value(
     else:
         try:
             val = getattr(inst, field_name)
-        except (AttributeError, models.fields.FieldDoesNotExist) as e:  # noqa
+        except (AttributeError, FieldDoesNotExist) as e:  # noqa
             return None
 
     if isinstance(val, models.Manager):

requirements.txt

@@ -1,4 +1,4 @@
-Django >=2.2, <4
+Django >=3.2, <4
 celery>=4.0.2,!=4.1.0
 cryptography>=2.0,<=3.4.8
 decorator==4.0.11

tests/machinery/test_base.py

@@ -21,7 +21,7 @@
 from django_declarative_apis.machinery import errors, filtering, tasks
 from django_declarative_apis.machinery.tasks import future_task_runner
 from django_declarative_apis.resources.utils import HttpStatusCode
-from tests import testutils
+from tests import testutils, models
 
 _TEST_RESOURCE = {"foo": "bar"}
 
@@ -147,70 +147,96 @@ def resource(self):
         mock_logging.error.assert_called_with("('something bad happened',)\nNone")
 
     def test_get_response_with_dirty_resource(self):
-        class _TestResource:
-            def is_dirty(self, check_relationship=False):
-                return True
+        class _TestEndpoint1(machinery.EndpointDefinition):
+            @machinery.endpoint_resource(type=models.DirtyFieldsModel)
+            def resource(self):
+                result = models.DirtyFieldsModel(field="abcde")
+                result.fk_field = models.TestModel.objects.create(int_field=1)
+                return result
+
+        class _TestEndpoint2(machinery.EndpointDefinition):
+            @machinery.endpoint_resource(type=models.DirtyFieldsModel)
+            def resource(self):
+                result = models.DirtyFieldsModel(field="abcde")
+                result.fk_field = models.TestModel.objects.create(int_field=1)
+                return result
 
-            def save(self):
+            @machinery.task
+            def null_task(self):
                 pass
 
-        class _TestEndpoint(machinery.EndpointDefinition):
-            @machinery.endpoint_resource(type=_TestResource)
+        class _TestEndpoint3(machinery.EndpointDefinition):
+            @machinery.endpoint_resource(type=models.DirtyFieldsModel)
             def resource(self):
-                return _TestResource()
+                result = models.DirtyFieldsModel(field="abcde")
+                result.fk_field = models.TestModel.objects.create(int_field=1)
+                return result
 
-        endpoint = _TestEndpoint()
-        manager = machinery.EndpointBinder.BoundEndpointManager(
-            machinery._EndpointRequestLifecycleManager(endpoint), endpoint
-        )
+            @machinery.task
+            def task(self):
+                self.resource.field = "zyxwv"
+
+        for test_name, endpoint_cls, expected_call_count in (
+            ("No Task", _TestEndpoint1, 1),
+            ("No-op Task", _TestEndpoint2, 1),
+            ("With Task", _TestEndpoint3, 2),
+        ):
+            with self.subTest(test_name):
+                endpoint = endpoint_cls()
+                manager = machinery.EndpointBinder.BoundEndpointManager(
+                    machinery._EndpointRequestLifecycleManager(endpoint), endpoint
+                )
 
-        class _FakeRequest:
-            META = {}
+                class _FakeRequest:
+                    META = {}
 
-        manager.bound_endpoint.request = _FakeRequest()
+                manager.bound_endpoint.request = _FakeRequest()
 
-        with mock.patch.object(_TestResource, "save", return_value=None) as mock_save:
-            manager.get_response()
-            # save is called before and after tasks. since we've hardcoded _TestResource.is_dirty to return True,
-            # both of them should fire
-            self.assertEqual(mock_save.call_count, 2)
+                with mock.patch.object(
+                    models.DirtyFieldsModel.objects,
+                    "update_or_create",
+                    wraps=models.DirtyFieldsModel.objects.update_or_create,
+                ) as mock_uoc:
+                    manager.get_response()
+                    self.assertEqual(mock_uoc.call_count, expected_call_count)
 
     def test_get_response_with_client_error_while_executing_tasks(self):
-        class _TestResource:
-            def is_dirty(self, check_relationship=False):
-                return True
-
-            def save(self):
-                pass
-
         class _TestEndpoint(machinery.EndpointDefinition):
-            @machinery.endpoint_resource(type=_TestResource)
+            @machinery.endpoint_resource(type=models.DirtyFieldsModel)
             def resource(self):
-                return _TestResource()
+                result = models.DirtyFieldsModel(id=1, field="abcde")
+                result.fk_field = models.TestModel.objects.create(int_field=1)
+                return result
 
             @machinery.task
             def raise_an_exception(self):
+                self.resource.field = "zyxwv"
                 raise errors.ClientError(
                     code=http.HTTPStatus.BAD_REQUEST,
                     message="something bad happened",
                     save_changes=error_should_save_changes,
                 )
 
         for error_should_save_changes in (True, False):
-            with mock.patch.object(_TestResource, "save") as mock_save:
-                endpoint = _TestEndpoint()
-                manager = machinery.EndpointBinder.BoundEndpointManager(
-                    machinery._EndpointRequestLifecycleManager(endpoint), endpoint
-                )
-                try:
-                    manager.get_response()
-                    self.fail("This should have failed")
-                except errors.ClientError:
-                    # save should be called twice if the exception says the resource should be saved: once before
-                    # tasks are executed and once during exception handling.
-                    self.assertEqual(
-                        mock_save.call_count, 2 if error_should_save_changes else 1
+            with self.subTest(f"error_should_save_changes={error_should_save_changes}"):
+                with mock.patch.object(
+                    models.DirtyFieldsModel.objects,
+                    "update_or_create",
+                    wraps=models.DirtyFieldsModel.objects.update_or_create,
+                ) as mock_uoc:
+                    endpoint = _TestEndpoint()
+                    manager = machinery.EndpointBinder.BoundEndpointManager(
+                        machinery._EndpointRequestLifecycleManager(endpoint), endpoint
                     )
+                    try:
+                        manager.get_response()
+                        self.fail("This should have failed")
+                    except errors.ClientError:
+                        # save should be called twice if the exception says the resource should be saved: once before
+                        # tasks are executed and once during exception handling.
+                        self.assertEqual(
+                            mock_uoc.call_count, 2 if error_should_save_changes else 1
+                        )
 
     def test_get_response_custom_http_response(self):
         expected_data = {"foo": "bar"}
@@ -280,7 +306,11 @@ class _QuerySet(list):
         data = _QuerySet([_TestResource("foo", "bar"), _TestResource("bar", "baz")])
 
         filter_def = {
-            _TestResource: {"name": filtering.ALWAYS, "secret": filtering.NEVER}
+            _TestResource: {
+                "name": filtering.ALWAYS,
+                "secret": filtering.NEVER,
+                "foo": filtering.ALWAYS,
+            }
         }
 
         class _TestEndpoint(machinery.EndpointDefinition):

tests/models.py

@@ -51,3 +51,14 @@ class ParentModel(models.Model):
 class RootNode(models.Model):
     id = models.IntegerField(primary_key=True)
     parent_field = models.ForeignKey(ParentModel, on_delete=models.CASCADE)
+
+
+try:
+    import dirtyfields
+
+    class DirtyFieldsModel(dirtyfields.DirtyFieldsMixin, models.Model):
+        field = models.CharField(max_length=100)
+        fk_field = models.ForeignKey(TestModel, null=False, on_delete=models.CASCADE)
+
+except Exception:
+    pass