Stop JSONModel hiding attribute errors

`__getattr__` is called whenever an attribute error is raised.

This means that if something deep inside a property on a model raised
an attribute error, that error would be caught by `__getattr__`, which
would then raise an exception that looked like the property itself
didn’t exist. Very confusing.

The solution seems to be to override `__getattribute__` instead, which
handles _all_ attributes, not just those that aren’t explicitly defined.
We then only intervene if the desired attribute is one of the
`ALLOWED_PROPERTIES`, otherwise falling through to the built in methods
of the underlying `object`.

app/models/__init__.py

@@ -21,12 +21,27 @@ class JSONModel():
     def __eq__(self, other):
         return self.id == other.id
 
-    def __getattr__(self, attr):
+    def __getattribute__(self, attr):
-        if attr in self.ALLOWED_PROPERTIES:
+
-            return self._dict[attr]
+        try:
-        raise AttributeError('`{}` is not a {} attribute'.format(
+            return super().__getattribute__(attr)
-            attr,
+        except AttributeError as e:
-            self.__class__.__name__.lower(),
+            # 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):

tests/app/models/test_base_model.py

@@ -0,0 +1,70 @@
+import pytest
+
+from app.models import JSONModel
+
+
+def test_looks_up_from_dict():
+
+    class Custom(JSONModel):
+        ALLOWED_PROPERTIES = {'foo'}
+
+    assert Custom({'foo': 'bar'}).foo == 'bar'
+
+
+def test_prefers_property_to_dict():
+
+    class Custom(JSONModel):
+
+        ALLOWED_PROPERTIES = {'foo'}
+
+        @property
+        def foo(self):
+            return 'bar'
+
+    assert Custom({'foo': 'NOPE'}).foo == 'bar'
+
+
+@pytest.mark.parametrize('json_response', (
+    {},
+    {'foo': 'bar'},  # Should still raise an exception
+))
+def test_model_raises_for_unknown_attributes(json_response):
+
+    model = JSONModel(json_response)
+    assert model.ALLOWED_PROPERTIES == set()
+
+    with pytest.raises(AttributeError) as e:
+        model.foo
+
+    assert str(e.value) == (
+        "'JSONModel' 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():
+
+    class Custom(JSONModel):
+        ALLOWED_PROPERTIES = {'foo'}
+
+    with pytest.raises(KeyError) as e:
+        Custom({}).foo
+
+    assert str(e.value) == "'foo'"
+
+
+@pytest.mark.parametrize('json_response', (
+    {},
+    {'foo': 'bar'},  # Should be ignored
+))
+def test_model_doesnt_swallow_attribute_errors(json_response):
+
+    class Custom(JSONModel):
+        @property
+        def foo(self):
+            raise AttributeError('Something has gone wrong')
+
+    with pytest.raises(AttributeError) as e:
+        Custom(json_response).foo
+
+    assert str(e.value) == 'Something has gone wrong'