Merge pull request #3687 from alphagov/remove-custom-getattr

Remove custom `getattr` implementation from JSONModel

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:
+                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))

app/models/job.py

@@ -25,11 +25,9 @@ class Job(JSONModel):
     ALLOWED_PROPERTIES = {
         'id',
         'service',
-        'template',
         'template_version',
         'original_file_name',
         'created_at',
-        'processing_started',
         'notification_count',
         'created_by',
         'template_type',

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',
         'rate_limit',
         'name',

app/models/user.py

@@ -39,7 +39,6 @@ class User(JSONModel, UserMixin):
         'mobile_number',
         'password_changed_at',
         'permissions',
-        'platform_admin',
         'state',
     }
 

tests/app/models/test_base_model.py

@@ -11,7 +11,7 @@ def test_looks_up_from_dict():
     assert Custom({'foo': 'bar'}).foo == 'bar'
 
 
-def test_prefers_property_to_dict():
+def test_raises_when_overriding_custom_properties():
 
     class Custom(JSONModel):
 
@@ -19,9 +19,12 @@ def test_prefers_property_to_dict():
 
         @property
         def foo(self):
-            return 'bar'
+            pass
 
-    assert Custom({'foo': 'NOPE'}).foo == 'bar'
+    with pytest.raises(AttributeError) as e:
+        Custom({'foo': 'NOPE'})
+
+    assert str(e.value) == "can't set attribute"
 
 
 @pytest.mark.parametrize('json_response', (
@@ -39,10 +42,7 @@ def test_model_raises_for_unknown_attributes(json_response):
     with pytest.raises(AttributeError) as e:
         model.foo
 
-    assert str(e.value) == (
+    assert str(e.value) == "'Custom' object has no attribute 'foo'"
-        "'Custom' object has no attribute 'foo' and 'foo' is not a "
-        "field in the underlying JSON"
-    )
 
 
 def test_model_raises_keyerror_if_item_missing_from_dict():
@@ -50,10 +50,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 +79,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']