From cca19df73ce7c328de05b174f3b9b899dc71be35 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 9 Jul 2019 11:47:40 +0100
Subject: [PATCH] Stop `JSONModel` hiding attribute errors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`__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              | 27 ++++++++---
 tests/app/models/test_base_model.py | 70 +++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 6 deletions(-)
 create mode 100644 tests/app/models/test_base_model.py

diff --git a/app/models/__init__.py b/app/models/__init__.py
index 7675d4a81..2574495e2 100644
--- a/app/models/__init__.py
+++ b/app/models/__init__.py
@@ -21,12 +21,27 @@ class JSONModel():
     def __eq__(self, other):
         return self.id == other.id
 
-    def __getattr__(self, attr):
-        if attr in self.ALLOWED_PROPERTIES:
-            return self._dict[attr]
-        raise AttributeError('`{}` is not a {} attribute'.format(
-            attr,
-            self.__class__.__name__.lower(),
+    def __getattribute__(self, attr):
+
+        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):
diff --git a/tests/app/models/test_base_model.py b/tests/app/models/test_base_model.py
new file mode 100644
index 000000000..0c3ccf2f7
--- /dev/null
+++ b/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'