From d103e9174fd1aa6412702761d76c923054d4d2d0 Mon Sep 17 00:00:00 2001 From: Lincoln Puzey Date: Mon, 30 Nov 2020 20:34:34 +0800 Subject: [PATCH 1/4] Add regression test when saving a model using F objects and update_fields arg. --- tests/test_non_regression.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/test_non_regression.py b/tests/test_non_regression.py index 5484788..74ce138 100644 --- a/tests/test_non_regression.py +++ b/tests/test_non_regression.py @@ -1,5 +1,6 @@ import pytest from django.db import IntegrityError +from django.db.models import F from django.test.utils import override_settings from .models import (ModelTest, ModelWithForeignKeyTest, ModelWithNonEditableFieldsTest, @@ -171,3 +172,21 @@ def test_access_deferred_field_doesnt_reset_state(): assert tm_deferred.get_deferred_fields() == set() # previously accessing the deferred field would reset the dirty state. assert tm_deferred.get_dirty_fields() == {"boolean": True} + + +@pytest.mark.django_db +def test_f_objects_and_save_update_fields_works(): + # Non regression test case for bug: + # https://github.com/romgar/django-dirtyfields/issues/118 + tm = ExpressionModelTest.objects.create(counter=0) + assert tm.counter == 0 + + tm.counter = F("counter") + 1 + tm.save() + tm.refresh_from_db() + assert tm.counter == 1 + + tm.counter = F("counter") + 1 + tm.save(update_fields=["counter"]) + tm.refresh_from_db() + assert tm.counter == 2 From 900488049a33621da916175e3192c750ebcaf3e6 Mon Sep 17 00:00:00 2001 From: Lincoln Puzey Date: Sat, 24 Apr 2021 17:01:36 +0800 Subject: [PATCH 2/4] Add comment describing what _as_dict() does. --- src/dirtyfields/dirtyfields.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/dirtyfields/dirtyfields.py b/src/dirtyfields/dirtyfields.py index 1a3ba8c..76cbb0a 100644 --- a/src/dirtyfields/dirtyfields.py +++ b/src/dirtyfields/dirtyfields.py @@ -48,6 +48,12 @@ def _connect_m2m_relations(self): name=self.__class__.__name__)) def _as_dict(self, check_relationship, include_primary_key=True): + """ + Capture the model fields' state as a dictionary. + + Only capture values we are confident are in the database, or would be + saved to the database if self.save() is called. + """ all_field = {} deferred_fields = self.get_deferred_fields() From c29a3ffbb9d102d58f0d6f1bc48019005267e529 Mon Sep 17 00:00:00 2001 From: Lincoln Puzey Date: Sat, 24 Apr 2021 17:20:34 +0800 Subject: [PATCH 3/4] Fixed a `KeyError` that happened when saving a Model with `update_fields` specified Happened after updating a field value with an `F` object (Issue #118). --- ChangeLog.rst | 4 ++++ src/dirtyfields/dirtyfields.py | 13 ++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ChangeLog.rst b/ChangeLog.rst index 1b2f7ce..428622f 100644 --- a/ChangeLog.rst +++ b/ChangeLog.rst @@ -11,6 +11,10 @@ master - Provide programmatically accessible package version number. - Migrate package metadata from setup.py to setup.cfg and specify the PEP-517 build-backend to use with the project. +*Bugfix:* + - Fixed a :code:`KeyError` that happened when saving a Model with :code:`update_fields` specified after updating a + field value with an :code:`F` object (#118). + .. _v1.6.0: 1.6.0 (07/04/2021) diff --git a/src/dirtyfields/dirtyfields.py b/src/dirtyfields/dirtyfields.py index 76cbb0a..0bb175a 100644 --- a/src/dirtyfields/dirtyfields.py +++ b/src/dirtyfields/dirtyfields.py @@ -168,7 +168,18 @@ def reset_state(sender, instance, **kwargs): if field.get_attname() in instance.get_deferred_fields(): continue - instance._original_state[field.name] = new_state[field.name] + if field.name in new_state: + instance._original_state[field.name] = ( + new_state[field.name] + ) + else: + # If we are here it means the field was updated in the DB, + # and we don't know the new value in the database. + # e.g it was updated with an F() expression. + # Because we now don't know the value in the DB, + # we remove it from _original_state, because we can't tell + # if its dirty or not. + del instance._original_state[field.name] else: instance._original_state = new_state From 51389507a51f6f8f5dce193632902a010156b3db Mon Sep 17 00:00:00 2001 From: Lincoln Puzey Date: Sat, 24 Apr 2021 17:45:35 +0800 Subject: [PATCH 4/4] Add tests defining how get_dirty_fields() works when updating fields with F objects. --- tests/test_save_fields.py | 74 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/tests/test_save_fields.py b/tests/test_save_fields.py index 2a08c1e..0e564f5 100644 --- a/tests/test_save_fields.py +++ b/tests/test_save_fields.py @@ -1,6 +1,13 @@ import pytest -from .models import ModelTest, MixedFieldsModelTest, ModelWithForeignKeyTest +from django.db.models import F + +from .models import ( + ExpressionModelTest, + MixedFieldsModelTest, + ModelTest, + ModelWithForeignKeyTest, +) from .utils import assert_number_of_queries_on_regex @@ -139,3 +146,68 @@ def test_save_deferred_field_with_update_fields_behaviour(): tm.save(update_fields=['boolean']) tm.boolean = False assert tm.get_dirty_fields() == {'boolean': True} + + +@pytest.mark.django_db +def test_get_dirty_fields_when_saving_with_f_objects(): + """ + This documents how get_dirty_fields() behaves when updating model fields + with F objects. + """ + + tm = ExpressionModelTest.objects.create(counter=0) + assert tm.counter == 0 + assert tm.get_dirty_fields() == {} + + tm.counter = F("counter") + 1 + # tm.counter field is not considered dirty because it doesn't have a simple + # value in memory we can compare to the original value. + # i.e. we don't know what value it will be in the database after the F + # object is translated into SQL. + assert tm.get_dirty_fields() == {} + + tm.save() + # tm.counter is still an F object after save() - we don't know the new + # value in the database. + assert tm.get_dirty_fields() == {} + + tm.counter = 10 + # even though we have now assigned a literal value to tm.counter, we don't + # know the value in the database, so it is not considered dirty. + assert tm.get_dirty_fields() == {} + + tm.save() + assert tm.get_dirty_fields() == {} + + tm.refresh_from_db() + # if we call refresh_from_db(), we load the database value, + # so we can assign a value and make the field dirty again. + tm.counter = 20 + assert tm.get_dirty_fields() == {"counter": 10} + + +@pytest.mark.django_db +def test_get_dirty_fields_when_saving_with_f_objects_update_fields_specified(): + """ + Same as above but with update_fields specified when saving/refreshing + """ + + tm = ExpressionModelTest.objects.create(counter=0) + assert tm.counter == 0 + assert tm.get_dirty_fields() == {} + + tm.counter = F("counter") + 1 + assert tm.get_dirty_fields() == {} + + tm.save(update_fields={"counter"}) + assert tm.get_dirty_fields() == {} + + tm.counter = 10 + assert tm.get_dirty_fields() == {} + + tm.save(update_fields={"counter"}) + assert tm.get_dirty_fields() == {} + + tm.refresh_from_db(fields={"counter"}) + tm.counter = 20 + assert tm.get_dirty_fields() == {"counter": 10}