From b291d8509fae235e1596dbfcc246fc1fae1674b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20L=C3=BCbcke?= Date: Mon, 31 Oct 2022 14:10:14 +0100 Subject: [PATCH 1/5] Introducing IommiModel --- iommi/form.py | 4 +- iommi/model.py | 45 ++++++++++++++++++ iommi/model__tests.py | 103 ++++++++++++++++++++++++++++++++++++++++++ tests/models.py | 26 ++++++++++- 4 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 iommi/model.py create mode 100644 iommi/model__tests.py diff --git a/iommi/form.py b/iommi/form.py index 3642f255..4d382d77 100644 --- a/iommi/form.py +++ b/iommi/form.py @@ -332,10 +332,10 @@ def create_q_objects(suffix): when_clauses = [When(q, then=rank) for rank, q in enumerate(q_objects)] choices = field.choices.annotate( - iommi_ranking=Case(*when_clauses, default=len(q_objects) + 1, output_field=IntegerField()) + _iommi_ranking=Case(*when_clauses, default=len(q_objects) + 1, output_field=IntegerField()) ) - return choices.filter(reduce(or_, q_objects)).order_by('iommi_ranking', *field.search_fields) + return choices.filter(reduce(or_, q_objects)).order_by('_iommi_ranking', *field.search_fields) def choice_queryset__parse(field, string_value, **_): diff --git a/iommi/model.py b/iommi/model.py new file mode 100644 index 00000000..cfd4fd7d --- /dev/null +++ b/iommi/model.py @@ -0,0 +1,45 @@ +from django.core.exceptions import FieldDoesNotExist +from django.db.models import Model + + +class IommiModel(Model): + class Meta: + abstract = True + + def __setattr__(self, name, value): + if not name.startswith('_') and name != 'pk': + try: + field = self._meta.get_field(name) + + if getattr(field, 'primary_key', False): + return object.__setattr__(self, name, value) + + except FieldDoesNotExist as e: + if name not in self.get_annotated_attributes(): + raise TypeError(f'There is no field {name}') from e + + self.get_updated_fields().add(name) + + return object.__setattr__(self, name, value) + + def get_updated_fields(self): + return self.__dict__.setdefault('_updated_fields', set()) + + def get_annotated_attributes(self): + return () + + @classmethod + def from_db(cls, db, field_names, values): + result = super().from_db(db, field_names, values) + result.get_updated_fields().clear() + return result + + def save(self, force_insert=False, force_update=False, using=None, update_fields=None): + if self.pk is not None and not force_insert: + update_fields = self.get_updated_fields() + + super(IommiModel, self).save( + force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields + ) + + self.get_updated_fields().clear() diff --git a/iommi/model__tests.py b/iommi/model__tests.py new file mode 100644 index 00000000..70caa6d2 --- /dev/null +++ b/iommi/model__tests.py @@ -0,0 +1,103 @@ +import pytest +from django.db.models import ( + AutoField, + CASCADE, + Max, + OneToOneField, +) + +from iommi.model import IommiModel +from tests.models import ( + MyAnnotatedIommiModel, + MyIommiModel, + NoRaceConditionModel, + RaceConditionModel, +) + + +def test_model(): + m = MyIommiModel(foo=17) + assert m.foo == 17 + + assert m.get_updated_fields() == {'foo'} + + +def test_constructor_exception(): + with pytest.raises(TypeError): + MyIommiModel(bar=17) + + +def test_attribute_exception(): + m = MyIommiModel() + m._bar = 17 + with pytest.raises(TypeError): + m.bar = 17 + + +def test_reversed(): + class MyOtherModel(IommiModel): + bar = OneToOneField(MyIommiModel, related_name='other', on_delete=CASCADE) + + o = MyOtherModel(bar=MyIommiModel()) + o.bar = MyIommiModel() + + MyIommiModel().other = MyOtherModel() + + +def test_updated_fields(): + m = MyIommiModel() + m.foo = 17 + + assert m.get_updated_fields() == {'foo'} + + +def test_ignore_pk_field(): + class WeirdPKNameModel(IommiModel): + this_is_a_pk = AutoField(primary_key=True) + + m = WeirdPKNameModel() + + assert m.get_updated_fields() == set() + + +@pytest.mark.django_db +def test_race_condition_on_save(): + m = RaceConditionModel.objects.create(a=1, b=2) + m2 = RaceConditionModel.objects.get(pk=m.pk) + m2.b = 7 + m2.save() + + m.a = 17 + m.save() # This save() overwrites the value of b + assert RaceConditionModel.objects.get(pk=m.pk).b == 2 + + +@pytest.mark.django_db +def test_no_race_condition_on_save(): + m = NoRaceConditionModel.objects.create(a=1, b=2) + m2 = NoRaceConditionModel.objects.get(pk=m.pk) + assert m2.get_updated_fields() == set() + m2.b = 7 + assert m2.get_updated_fields() == {'b'} + m2.save() + + m.a = 17 + assert m.get_updated_fields() == {'a'} + m.save() # This save() does NOT overwrite b! + assert NoRaceConditionModel.objects.get(pk=m.pk).b == 7 + assert not m.get_updated_fields() + + +@pytest.mark.django_db +def test_annotation(): + MyIommiModel.objects.create(foo=2) + with pytest.raises(TypeError): + MyIommiModel.objects.annotate(fisk=Max('foo')).get() + + MyAnnotatedIommiModel.objects.create(foo=2) + MyAnnotatedIommiModel.objects.annotate(fisk=Max('foo')).get() + + +@pytest.mark.django_db +def test_force_insert(): + MyIommiModel.objects.create(pk=3, foo=1) diff --git a/tests/models.py b/tests/models.py index 8a3f1964..addfe317 100644 --- a/tests/models.py +++ b/tests/models.py @@ -14,6 +14,7 @@ ) from iommi import register_search_fields +from iommi.model import IommiModel class FormFromModelTest(Model): @@ -153,8 +154,8 @@ class TBaz(Model): class Meta: ordering = ('pk',) - - + + class AdminUnique(Model): foo = IntegerField() unique = IntegerField(unique=True) @@ -271,3 +272,24 @@ class CustomField(models.Field): class NotRegisteredCustomFieldModel(models.Model): custom_field = CustomField() + + +class RaceConditionModel(models.Model): + a = IntegerField() + b = IntegerField() + + +class NoRaceConditionModel(IommiModel): + a = IntegerField() + b = IntegerField() + + +class MyIommiModel(IommiModel): + foo = IntegerField() + + +class MyAnnotatedIommiModel(IommiModel): + foo = IntegerField() + + def get_annotated_attributes(self): + return ['fisk'] From 20bf5ab93f00b20ae284a62af0513a02f5808d4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20Hovm=C3=B6ller?= Date: Tue, 1 Nov 2022 10:39:02 +0100 Subject: [PATCH 2/5] Added a good default repr, and improved error message for TypeError --- iommi/model.py | 5 ++++- iommi/model__tests.py | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/iommi/model.py b/iommi/model.py index cfd4fd7d..7f716314 100644 --- a/iommi/model.py +++ b/iommi/model.py @@ -16,7 +16,7 @@ def __setattr__(self, name, value): except FieldDoesNotExist as e: if name not in self.get_annotated_attributes(): - raise TypeError(f'There is no field {name}') from e + raise TypeError(f'There is no field {name} on the model {self.__class__.__name__}. You can assign arbitrary attributes if they start with `_`. If this is an annotation, please add a method `get_annotated_attributes` that returns a list of valid annotated attributes that should not trigger this message.') from e self.get_updated_fields().add(name) @@ -43,3 +43,6 @@ def save(self, force_insert=False, force_update=False, using=None, update_fields ) self.get_updated_fields().clear() + + def __repr__(self): + return f'<{self.__class__.__name__} pk={self.pk}>' diff --git a/iommi/model__tests.py b/iommi/model__tests.py index 70caa6d2..067ca3d7 100644 --- a/iommi/model__tests.py +++ b/iommi/model__tests.py @@ -101,3 +101,8 @@ def test_annotation(): @pytest.mark.django_db def test_force_insert(): MyIommiModel.objects.create(pk=3, foo=1) + + +def test_repr(): + assert repr(MyIommiModel(pk=None, foo=1)) == '' + assert repr(MyIommiModel(pk=7, foo=1)) == '' From 0f53b2db7e023110c61b3014bb2f6c9e0675ce96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20L=C3=BCbcke?= Date: Thu, 17 Nov 2022 09:35:31 +0100 Subject: [PATCH 3/5] Change protocol for ignored attributes --- iommi/model.py | 14 +++++++++----- tests/models.py | 3 +-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/iommi/model.py b/iommi/model.py index 7f716314..3a6a51d8 100644 --- a/iommi/model.py +++ b/iommi/model.py @@ -6,6 +6,8 @@ class IommiModel(Model): class Meta: abstract = True + iommi_ignored_attributes = () + def __setattr__(self, name, value): if not name.startswith('_') and name != 'pk': try: @@ -15,8 +17,13 @@ def __setattr__(self, name, value): return object.__setattr__(self, name, value) except FieldDoesNotExist as e: - if name not in self.get_annotated_attributes(): - raise TypeError(f'There is no field {name} on the model {self.__class__.__name__}. You can assign arbitrary attributes if they start with `_`. If this is an annotation, please add a method `get_annotated_attributes` that returns a list of valid annotated attributes that should not trigger this message.') from e + if name not in self.iommi_ignored_attributes: + raise TypeError( + f'There is no field {name} on the model {self.__class__.__name__}. ' + f'You can assign arbitrary attributes if they start with `_`. ' + f'If this is an annotation, please add a method `get_annotated_attributes` that returns a list ' + f'of valid annotated attributes that should not trigger this message.' + ) from e self.get_updated_fields().add(name) @@ -25,9 +32,6 @@ def __setattr__(self, name, value): def get_updated_fields(self): return self.__dict__.setdefault('_updated_fields', set()) - def get_annotated_attributes(self): - return () - @classmethod def from_db(cls, db, field_names, values): result = super().from_db(db, field_names, values) diff --git a/tests/models.py b/tests/models.py index addfe317..759c4762 100644 --- a/tests/models.py +++ b/tests/models.py @@ -291,5 +291,4 @@ class MyIommiModel(IommiModel): class MyAnnotatedIommiModel(IommiModel): foo = IntegerField() - def get_annotated_attributes(self): - return ['fisk'] + iommi_ignored_attributes = ['fisk'] From afc2b9709d92abf9ad435cb16ec2b1cc77c115d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20L=C3=BCbcke?= Date: Thu, 17 Nov 2022 11:16:40 +0100 Subject: [PATCH 4/5] Cleanup --- iommi/model.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/iommi/model.py b/iommi/model.py index 3a6a51d8..a1ea3b33 100644 --- a/iommi/model.py +++ b/iommi/model.py @@ -42,9 +42,7 @@ def save(self, force_insert=False, force_update=False, using=None, update_fields if self.pk is not None and not force_insert: update_fields = self.get_updated_fields() - super(IommiModel, self).save( - force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields - ) + super().save(force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields) self.get_updated_fields().clear() From 97d5b98ada60ce0abaa451a55d4cd9f6267502d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20Hovm=C3=B6ller?= Date: Mon, 28 Nov 2022 20:15:02 +0100 Subject: [PATCH 5/5] Fixed error message --- iommi/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iommi/model.py b/iommi/model.py index a1ea3b33..ac9f0ae3 100644 --- a/iommi/model.py +++ b/iommi/model.py @@ -21,7 +21,7 @@ def __setattr__(self, name, value): raise TypeError( f'There is no field {name} on the model {self.__class__.__name__}. ' f'You can assign arbitrary attributes if they start with `_`. ' - f'If this is an annotation, please add a method `get_annotated_attributes` that returns a list ' + f'If this is an annotation, please add a tuple on the class named `iommi_ignored_attributes`' f'of valid annotated attributes that should not trigger this message.' ) from e