From 89c63e3243f0d3f5dfd9799768796ed2f3bd6fa1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Oct 2020 15:36:50 +0100 Subject: [PATCH] Remove custom `getattr` implementation from JSONModel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We wrote custom `__getattr__` and `__getitem__` implementation to make it easier to find code that was accessing fields in the dictionary that we weren’t aware of. See this commit for details: https://github.com/alphagov/notifications-admin/commit/b48305c50d0d6b1674a39e0fd5a3c4768d281a66 Now that no view-layer code accesses the service dictionary directly we don’t need to support this behaviour any more. By removing it we can make the model code simpler, and closer to the `SerialisedModel` it inherits from. It still needs some custom implementation because: - a lot of our test fixtures are lazy and don’t set up all the expected fields, so we need to account for fields sometimes being present in the underlying dictionary and sometimes not - we often implement a property that has the same name as one of the fields in the JSON, so we have to be careful not to try to override this property with the value from the underlying JSON --- app/models/__init__.py | 31 +++-------------------------- app/models/job.py | 1 - app/models/service.py | 2 -- app/models/user.py | 1 - tests/app/models/test_base_model.py | 13 ++++++------ 5 files changed, 9 insertions(+), 39 deletions(-) diff --git a/app/models/__init__.py b/app/models/__init__.py index f73531f12..c42bb028a 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -12,6 +12,9 @@ class JSONModel(SerialisedModel): def __init__(self, _dict): # in the case of a bad request _dict may be `None` self._dict = _dict or {} + for property in self.ALLOWED_PROPERTIES: + if property in self._dict and not hasattr(self, property): + setattr(self, property, self._dict[property]) def __bool__(self): return self._dict != {} @@ -19,37 +22,9 @@ class JSONModel(SerialisedModel): def __hash__(self): return hash(self.id) - def __dir__(self): - return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) - def __eq__(self, other): return self.id == other.id - def __getattribute__(self, attr): - # Eventually we should remove this custom implementation in - # favour of looping over self.ALLOWED_PROPERTIES in __init__ - - try: - return super().__getattribute__(attr) - except AttributeError as e: - # Re-raise any `AttributeError`s that are not directly on - # this object because they indicate an underlying exception - # that we don’t want to swallow - if str(e) != "'{}' object has no attribute '{}'".format( - self.__class__.__name__, attr - ): - raise e - - if attr in super().__getattribute__('ALLOWED_PROPERTIES'): - return super().__getattribute__('_dict')[attr] - - raise AttributeError(( - "'{}' object has no attribute '{}' and '{}' is not a field " - "in the underlying JSON" - ).format( - self.__class__.__name__, attr, attr - )) - def _get_by_id(self, things, id): try: return next(thing for thing in things if thing['id'] == str(id)) diff --git a/app/models/job.py b/app/models/job.py index adda565d5..e08a8a87a 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -25,7 +25,6 @@ class Job(JSONModel): ALLOWED_PROPERTIES = { 'id', 'service', - 'template', 'template_version', 'original_file_name', 'created_at', diff --git a/app/models/service.py b/app/models/service.py index a88b580e6..0367a130a 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -31,11 +31,9 @@ class Service(JSONModel): ALLOWED_PROPERTIES = { 'active', 'contact_link', - 'email_branding', 'email_from', 'id', 'inbound_api', - 'letter_branding', 'message_limit', 'name', 'prefix_sms', diff --git a/app/models/user.py b/app/models/user.py index 0c27aff2e..d3fd5a3aa 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -39,7 +39,6 @@ class User(JSONModel, UserMixin): 'mobile_number', 'password_changed_at', 'permissions', - 'platform_admin', 'state', } diff --git a/tests/app/models/test_base_model.py b/tests/app/models/test_base_model.py index 2be06759e..48500b0c7 100644 --- a/tests/app/models/test_base_model.py +++ b/tests/app/models/test_base_model.py @@ -39,10 +39,7 @@ def test_model_raises_for_unknown_attributes(json_response): with pytest.raises(AttributeError) as e: model.foo - assert str(e.value) == ( - "'Custom' object has no attribute 'foo' and 'foo' is not a " - "field in the underlying JSON" - ) + assert str(e.value) == "'Custom' object has no attribute 'foo'" def test_model_raises_keyerror_if_item_missing_from_dict(): @@ -50,10 +47,10 @@ def test_model_raises_keyerror_if_item_missing_from_dict(): class Custom(JSONModel): ALLOWED_PROPERTIES = {'foo'} - with pytest.raises(KeyError) as e: + with pytest.raises(AttributeError) as e: Custom({}).foo - assert str(e.value) == "'foo'" + assert str(e.value) == "'Custom' object has no attribute 'foo'" @pytest.mark.parametrize('json_response', ( @@ -79,4 +76,6 @@ def test_dynamic_properties_are_introspectable(): class Custom(JSONModel): ALLOWED_PROPERTIES = {'foo', 'bar', 'baz'} - assert dir(Custom({}))[-3:] == ['bar', 'baz', 'foo'] + model = Custom({'foo': None, 'bar': None, 'baz': None}) + + assert dir(model)[-3:] == ['bar', 'baz', 'foo']