Repository: jbradberry/universe

Issue:
Fields need to have more validation
Currently the only validation we do is on whether a field is required.

- [x] validate that the type of data is consistent with the field type
- [x] validate any new additional field arguments, e.g. IntField.max
- [x] perform validation upon Entity initialization, as well as serialization
- [x] add a scheme to do per-component validation, not just per-field, in a manner similar to how Django forms can have a custom `Form.clean()` method
- [x] allow customization of field validation for a component, like with Django's `Form.clean_<fieldname>()` methods

Discussion:

Patch:
diff --git a/universe/components.py b/universe/components.py
index f454a10..f39c57f 100644
--- a/universe/components.py
+++ b/universe/components.py
@@ -1,11 +1,7 @@
 import math
 import random
 
-from . import fields
-
-
-class ValidationError(Exception):
-    pass
+from . import fields, exceptions
 
 
 class MetaComponent(type):
@@ -16,6 +12,7 @@ class MetaComponent(type):
         for name, f in attrs.items():
             if isinstance(f, fields.Field):
                 new_attrs['_fields'][name] = f
+                f.name = name
             else:
                 new_attrs[name] = f
 
@@ -24,17 +21,23 @@ class MetaComponent(type):
 
 
 class Component(metaclass=MetaComponent):
+    def validate(self, data):
+        for name, field in self._fields.items():
+            field.validate(data)
+            if hasattr(self, f'validate_{name}'):
+                getattr(self, f'validate_{name}')(data)
+
     def serialize(self, data):
         output = {}
+        self.validate(data)
         for name, field in self._fields.items():
             if name in data:
                 output[name] = data[name]
-            elif getattr(field, 'required', True):
-                raise ValidationError(f"{name} is required.")
         return output
 
     def display(self, data):
         output = {}
+        self.validate(data)
         for name, field in self._fields.items():
             if name in data:
                 value = data[name]
@@ -43,8 +46,6 @@ class Component(metaclass=MetaComponent):
                 else:
                     value = str(value)
                 output[name] = value
-            elif getattr(field, 'required', True):
-                raise ValidationError(f"{name} is required.")
         return output
 
 
diff --git a/universe/engine.py b/universe/engine.py
index f1d4246..08765e2 100644
--- a/universe/engine.py
+++ b/universe/engine.py
@@ -8,6 +8,9 @@ class Entity:
         self._data = data
         self._components = registry[data['type']]
 
+        for _type, component in self._components.items():
+            component.validate(data)
+
     def __getattr__(self, name):
         for component in self.__dict__.get('_components', {}).values():
             if name in component._fields:
diff --git a/universe/exceptions.py b/universe/exceptions.py
new file mode 100644
index 0000000..ead42d5
--- /dev/null
+++ b/universe/exceptions.py
@@ -0,0 +1,4 @@
+
+
+class ValidationError(Exception):
+    pass
diff --git a/universe/fields.py b/universe/fields.py
index d058137..669aaa4 100644
--- a/universe/fields.py
+++ b/universe/fields.py
@@ -1,20 +1,49 @@
+from . import exceptions
 
 
 class Field:
-    pass
+    def __init__(self, required=True):
+        self.required = required
+
+    def validate(self, data):
+        if self.required and self.name not in data:
+            raise exceptions.ValidationError(f"{self.name!r} is required.")
 
 
 class IntField(Field):
-    def __init__(self, min=None, max=None, required=True):
+    def __init__(self, min=None, max=None, **kwargs):
+        super().__init__(**kwargs)
         self.min = min
         self.max = max
-        self.required = required
+
+    def validate(self, data):
+        super().validate(data)
+        if self.name not in data:
+            return
+        value = data[self.name]
+        if not isinstance(value, int):
+            raise exceptions.ValidationError(f"{self.name!r} must be an integer.")
+        if self.min is not None and value < self.min:
+            raise exceptions.ValidationError(f"{self.name!r} must be greater than or equal to {self.min}.")
+        if self.max is not None and value > self.max:
+            raise exceptions.ValidationError(f"{self.name!r} must be less than or equal to {self.max}.")
 
 
 class ListField(Field):
-    pass
+    def validate(self, data):
+        super().validate(data)
+        if self.name not in data:
+            return
+        value = data[self.name]
+        if not isinstance(value, list):
+            raise exceptions.ValidationError(f"{self.name!r} must be a list.")
 
 
 class CharField(Field):
-    def __init__(self, required=True):
-        self.required = required
+    def validate(self, data):
+        super().validate(data)
+        if self.name not in data:
+            return
+        value = data[self.name]
+        if not isinstance(value, str):
+            raise exceptions.ValidationError(f"{self.name!r} must be a string.")


Test patch:
diff --git a/tests/test_components.py b/tests/test_components.py
index 7cfddbc..37e90c2 100644
--- a/tests/test_components.py
+++ b/tests/test_components.py
@@ -1,7 +1,52 @@
 # -*- coding: utf-8 -*-
 import unittest
 
-from universe import components
+from universe import components, fields, exceptions
+
+
+class ComponentTestCase(unittest.TestCase):
+    class SpecialComponent(components.Component):
+        foo = fields.IntField(required=False)
+        bar = fields.IntField(required=False)
+
+        def validate(self, data):
+            super().validate(data)
+            if ('foo' in data) == ('bar' in data):
+                raise exceptions.ValidationError("Only one of 'foo' or 'bar' can be set.")
+
+        def validate_bar(self, data):
+            if 'bar' in data and data['bar'] % 2 != 0:
+                raise exceptions.ValidationError("'bar' must be even.")
+
+    def test_invalid_type_foo(self):
+        with self.assertRaises(exceptions.ValidationError) as e:
+            self.SpecialComponent().validate({'foo': 'a'})
+        self.assertEqual(str(e.exception), "'foo' must be an integer.")
+
+    def test_invalid_type_bar(self):
+        with self.assertRaises(exceptions.ValidationError) as e:
+            self.SpecialComponent().validate({'bar': 'a'})
+        self.assertEqual(str(e.exception), "'bar' must be an integer.")
+
+    def test_valid_foo(self):
+        self.assertIsNone(self.SpecialComponent().validate({'foo': 42}))
+
+    def test_valid_bar(self):
+        self.assertIsNone(self.SpecialComponent().validate({'bar': 42}))
+
+    def test_custom_field_validation(self):
+        with self.assertRaises(exceptions.ValidationError) as e:
+            self.SpecialComponent().validate({'bar': 3})
+        self.assertEqual(str(e.exception), "'bar' must be even.")
+
+    def test_custom_component_validation(self):
+        with self.assertRaises(exceptions.ValidationError) as e:
+            self.SpecialComponent().validate({})
+        self.assertEqual(str(e.exception), "Only one of 'foo' or 'bar' can be set.")
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            self.SpecialComponent().validate({'foo': 42, 'bar': 12})
+        self.assertEqual(str(e.exception), "Only one of 'foo' or 'bar' can be set.")
 
 
 class MetadataComponentTestCase(unittest.TestCase):
@@ -13,7 +58,7 @@ class MetadataComponentTestCase(unittest.TestCase):
     def test_missing_required_field(self):
         data = {}
         component = components.MetadataComponent()
-        with self.assertRaises(components.ValidationError):
+        with self.assertRaises(exceptions.ValidationError):
             component.serialize(data)
 
 
diff --git a/tests/test_fields.py b/tests/test_fields.py
new file mode 100644
index 0000000..185e01c
--- /dev/null
+++ b/tests/test_fields.py
@@ -0,0 +1,140 @@
+# -*- coding: utf-8 -*-
+import unittest
+
+from universe import fields, exceptions
+
+
+class FieldTestCase(unittest.TestCase):
+    def test_implicit(self):
+        field = fields.Field()
+        field.name = 'foo'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({})
+        self.assertEqual(str(e.exception), "'foo' is required.")
+
+    def test_not_required(self):
+        field = fields.Field(required=False)
+        field.name = 'foo'
+
+        self.assertIsNone(field.validate({}))
+
+    def test_explicit_required(self):
+        field = fields.Field(required=True)
+        field.name = 'foo'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({})
+        self.assertEqual(str(e.exception), "'foo' is required.")
+
+
+class IntFieldTestCase(unittest.TestCase):
+    def test_not_required(self):
+        field = fields.IntField(required=False)
+        field.name = 'widgets'
+
+        self.assertIsNone(field.validate({}))
+
+    def test_explicit_required(self):
+        field = fields.IntField(required=True)
+        field.name = 'widgets'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({})
+        self.assertEqual(str(e.exception), "'widgets' is required.")
+
+    def test_bad_type(self):
+        field = fields.IntField(required=True)
+        field.name = 'widgets'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({'widgets': 'a'})
+        self.assertEqual(str(e.exception), "'widgets' must be an integer.")
+
+    def test_in_range(self):
+        field = fields.IntField(min=0, max=100, required=True)
+        field.name = 'widgets'
+
+        self.assertIsNone(field.validate({'widgets': 42}))
+
+    def test_under_min(self):
+        field = fields.IntField(min=0, max=100, required=True)
+        field.name = 'widgets'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({'widgets': -10})
+        self.assertEqual(str(e.exception), "'widgets' must be greater than or equal to 0.")
+
+    def test_over_max(self):
+        field = fields.IntField(min=0, max=100, required=True)
+        field.name = 'widgets'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({'widgets': 200})
+        self.assertEqual(str(e.exception), "'widgets' must be less than or equal to 100.")
+
+
+class ListFieldTestCase(unittest.TestCase):
+    def test_not_required(self):
+        field = fields.ListField(required=False)
+        field.name = 'queue'
+
+        self.assertIsNone(field.validate({}))
+
+    def test_explicit_required(self):
+        field = fields.ListField(required=True)
+        field.name = 'queue'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({})
+        self.assertEqual(str(e.exception), "'queue' is required.")
+
+    def test_bad_type(self):
+        field = fields.ListField(required=True)
+        field.name = 'queue'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({'queue': 0})
+        self.assertEqual(str(e.exception), "'queue' must be a list.")
+
+    def test_empty_list_is_valid(self):
+        field = fields.ListField(required=True)
+        field.name = 'queue'
+
+        self.assertIsNone(field.validate({'queue': []}))
+
+
+class CharFieldTestCase(unittest.TestCase):
+    def test_not_required(self):
+        field = fields.CharField(required=False)
+        field.name = 'name'
+
+        self.assertIsNone(field.validate({}))
+
+    def test_explicit_required(self):
+        field = fields.CharField(required=True)
+        field.name = 'name'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({})
+        self.assertEqual(str(e.exception), "'name' is required.")
+
+    def test_bad_type(self):
+        field = fields.CharField(required=True)
+        field.name = 'name'
+
+        with self.assertRaises(exceptions.ValidationError) as e:
+            field.validate({'name': 0})
+        self.assertEqual(str(e.exception), "'name' must be a string.")
+
+    def test_empty_string_is_valid(self):
+        field = fields.CharField(required=True)
+        field.name = 'name'
+
+        self.assertIsNone(field.validate({'name': ""}))
+
+    def test_valid_string(self):
+        field = fields.CharField(required=True)
+        field.name = 'name'
+
+        self.assertIsNone(field.validate({'name': "Human"}))