Merge pull request #33896 from dimagi/dm/dump-load-lookup-table

Fix jsonattrs conformance to the model deserialization protocol

corehq/apps/dump_reload/tests/test_sql_dump_load.py

@@ -735,41 +735,36 @@ def test_zapier_subscription(self):
         self._dump_and_load(Counter({CreateCaseRepeater: 1, ConnectionSettings: 1, ZapierSubscription: 1}))
 
     def test_lookup_table(self):
-        from corehq.apps.fixtures.models import LookupTable, LookupTableRow, LookupTableRowOwner, OwnerType
+        from corehq.apps.fixtures.models import (
+            Field,
+            LookupTable,
+            LookupTableRow,
+            LookupTableRowOwner,
+            OwnerType,
+            TypeField,
+        )
         table = LookupTable.objects.create(
             domain=self.domain_name,
             tag="dump-load",
             fields=[
-                {
-                    "name": "country",
-                    "properties": [],
-                    "is_indexed": True,
-                },
-                {
-                    "name": "state_name",
-                    "properties": ["lang"],
-                    "is_indexed": False,
-                },
-                {
-                    "name": "state_id",
-                    "properties": [],
-                    "is_indexed": False,
-                },
+                TypeField("country", is_indexed=True),
+                TypeField("state_name", properties=["lang"]),
+                TypeField("state_id"),
             ]
         )
         row = LookupTableRow.objects.create(
             domain=self.domain_name,
             table_id=table.id,
             fields={
                 "country": [
-                    {"value": "India", "properties": {}},
+                    Field("India"),
                 ],
                 "state_name": [
-                    {"value": "Delhi_IN_ENG", "properties": {"lang": "eng"}},
-                    {"value": "Delhi_IN_HIN", "properties": {"lang": "hin"}},
+                    Field("Delhi_IN_ENG", properties={"lang": "eng"}),
+                    Field("Delhi_IN_HIN", properties={"lang": "hin"}),
                 ],
                 "state_id": [
-                    {"value": "DEL", "properties": {}}
+                    Field("DEL"),
                 ],
             },
             sort_key=0,

corehq/util/jsonattrs.py

@@ -81,32 +81,43 @@ class Plane(models.Model):
 - Support for migrating/converting the outer collection of `AttrsDict` and
   `AttrsList`.
 """
-from attr.exceptions import NotAnAttrsClassError
 from attrs import asdict, define, field
 
+from django.core.exceptions import ValidationError
 from django.db.models import JSONField
+from django.utils.translation import gettext_lazy as _
 
 __all__ = ["AttrsDict", "AttrsList", "dict_of", "list_of"]
 
 
 class JsonAttrsField(JSONField):
+    default_error_messages = {
+        'invalid': _("'%(field)s' field value has an invalid format: %(exc)s"),
+    }
 
     def __init__(self, *args, builder, **kw):
         super().__init__(*args, **kw)
         self.builder = builder
 
     def get_prep_value(self, value):
+        return super().get_prep_value(self.builder.jsonify(value))
+
+    def to_python(self, value):
         try:
-            prep_value = super().get_prep_value(self.builder.jsonify(value))
-        except NotAnAttrsClassError:
-            # `self.builder.jsonify()` expected a subclass of `attrs`
-            # but `value` is not. e.g. It is loaded from a data dump.
-            prep_value = super().get_prep_value(value)
-        return prep_value
+            return self.builder.attrify(value)
+        except Exception as exc:
+            raise ValidationError(
+                self.error_messages['invalid'],
+                code='invalid',
+                params={
+                    'field': self.name,
+                    'exc': BadValue.format(value, exc),
+                },
+            )
 
     def from_db_value(self, value, expression, connection):
         value = super().from_db_value(value, expression, connection)
-        return self.builder.attrify(value)
+        return self.to_python(value)
 
     def value_to_string(self, obj):
         # Returns a JSON-serializable object for compatibility with
@@ -164,10 +175,8 @@ def attrify(self, items):
         attrs_type = self.attrs_type
         if items is None:
             return items
-        if hasattr(attrs_type, "__jsonattrs_from_json__"):
-            from_json = attrs_type.__jsonattrs_from_json__
-            return [from_json(item) for item in items]
-        return [attrs_type(**item) for item in items]
+        from_json = make_from_json(attrs_type)
+        return [from_json(item) for item in items]
 
     def jsonify(self, value):
         if not value:
@@ -187,10 +196,8 @@ def attrify(self, values):
         attrs_type = self.attrs_type
         if values is None:
             return values
-        if hasattr(attrs_type, "__jsonattrs_from_json__"):
-            from_json = attrs_type.__jsonattrs_from_json__
-            return {key: from_json(value) for key, value in values.items()}
-        return {key: attrs_type(**value) for key, value in values.items()}
+        from_json = make_from_json(attrs_type)
+        return {key: from_json(value) for key, value in values.items()}
 
     def jsonify(self, value):
         if not value:
@@ -202,6 +209,36 @@ def jsonify(self, value):
         return {k: to_json(v) for k, v in value.items()}
 
 
+def make_from_json(attrs_type):
+    def from_json(value):
+        try:
+            return transform(value)
+        except BadValue:
+            raise
+        except Exception as exc:
+            msg = _("Cannot construct {type_name} with {cause}")
+            raise BadValue(msg.format(
+                type_name=getattr(attrs_type, "__name__", attrs_type),
+                cause=BadValue.format(value, exc),
+            ))
+
+    if hasattr(attrs_type, "__jsonattrs_from_json__"):
+        transform = attrs_type.__jsonattrs_from_json__
+    else:
+        def transform(value):
+            return attrs_type(**value)
+    return from_json
+
+
+class BadValue(ValueError):
+
+    @classmethod
+    def format(cls, value, exc):
+        if isinstance(exc, cls):
+            return str(exc)
+        return f"{value!r} -> {type(exc).__name__}: {exc}"
+
+
 @define
 class dict_of:
     value_type = field()
@@ -225,6 +262,12 @@ def _check_none(self, value):
             typename = self.value_type.__name__
             raise ValueError(f"expected dict with {typename} values, got None")
 
+    @property
+    def __name__(self):
+        value_name = self.value_type.__name__
+        key = f", {self.key_type.__name__}" if self.key_type != str else ''
+        return f"{type(self).__name__}({value_name}{key})"
+
 
 @define
 class list_of:
@@ -247,3 +290,7 @@ def _check_none(self, value):
         if value is None:
             typename = self.item_type.__name__
             raise ValueError(f"expected list of {typename}, got None")
+
+    @property
+    def __name__(self):
+        return f"{type(self).__name__}({self.item_type.__name__})"

corehq/util/tests/test_jsonattrs.py

@@ -2,6 +2,8 @@
 from datetime import date
 
 from attrs import asdict, define, field
+from attrs.exceptions import NotAnAttrsClassError
+from django.core.exceptions import ValidationError
 from django.db import models
 from testil import assert_raises, eq
 
@@ -25,6 +27,15 @@ class Check(models.Model):
     assert check.points is not xydict, xydict
     eq(check.points, {"north": Point(0, 1)})
 
+    with assert_raises(ValidationError, msg=(
+        '["'
+        "'points' field value has an invalid format: "
+        "Cannot construct Point with {} -> "
+        "TypeError: __init__() missing 2 required positional arguments: 'x' and 'y'"
+        '"]'
+    )):
+        set_json_value(check, "points", {"north": {}})
+
 
 def test_attrsdict_list_of():
     @unregistered_django_model
@@ -46,6 +57,15 @@ class Check(models.Model):
     with assert_raises(ValueError, msg="expected list of Point, got None"):
         get_json_value(check, "point_lists")
 
+    with assert_raises(ValidationError, msg=(
+        '["'
+        "'point_lists' field value has an invalid format: "
+        "Cannot construct list_of(Point) with 500 -> "
+        "TypeError: 'int' object is not iterable"
+        '"]'
+    )):
+        set_json_value(check, "point_lists", {"north": 500})
+
 
 def test_attrslist():
     @unregistered_django_model
@@ -63,6 +83,16 @@ class Check(models.Model):
     assert check.values is not abbylist, abbylist
     eq(check.values, [Value("abby")])
 
+    with assert_raises(ValidationError, msg=(
+        '["'
+        "'values' field value has an invalid format: "
+        "Cannot construct Value with 'bad value' -> "
+        "TypeError: corehq.util.tests.test_jsonattrs.Value() "
+        "argument after ** must be a mapping, not str"
+        '"]'
+    )):
+        set_json_value(check, "values", ["bad value"])
+
 
 def test_attrslist_dict_of():
     @unregistered_django_model
@@ -84,6 +114,15 @@ class Check(models.Model):
     with assert_raises(ValueError, msg="expected dict with Value values, got None"):
         get_json_value(check, "value_items")
 
+    with assert_raises(ValidationError, msg=(
+        '["'
+        "'value_items' field value has an invalid format: "
+        "Cannot construct dict_of(Value) with 'bad' -> "
+        "AttributeError: 'str' object has no attribute 'items'"
+        '"]'
+    )):
+        set_json_value(check, "value_items", {"bad": "value"})
+
 
 def test_jsonattrs_to_json():
     @unregistered_django_model
@@ -116,6 +155,18 @@ class Check(models.Model):
     )
 
 
+def test_invalid_value_does_not_save():
+    @unregistered_django_model
+    class Check(models.Model):
+        events = AttrsList(Event)
+
+    check = Check(events=[{"monday": "2022-07-20"}])
+    with assert_raises(NotAnAttrsClassError, msg=(
+        "<class 'dict'> is not an attrs-decorated class."
+    )):
+        get_json_value(check, "events")
+
+
 def get_json_value(model, field_name):
     """Get the JSON value of a field as it would be stored in the database