Remove custom getattr implementation from JSONModel

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:
b48305c50d

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

@@ -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))

app/models/job.py

@@ -25,7 +25,6 @@ class Job(JSONModel):
     ALLOWED_PROPERTIES = {
         'id',
         'service',
-        'template',
         'template_version',
         'original_file_name',
         'created_at',

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

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

@@ -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']