Add more validation

2 files changed, 56 insertions, 29 deletions

diff --git a/lorrycontroller/readconf.py b/lorrycontroller/readconf.py
index cd89a49..5e15dd5 100644
--- a/lorrycontroller/readconf.py
+++ b/lorrycontroller/readconf.py
@@ -234,32 +234,57 @@ class ReadConfiguration(lorrycontroller.LorryControllerRoute):
 class LorryControllerConfValidator(object):
 
     def validate_config(self, conf_obj):
-        for check in self._find_checks():
-            error = check(conf_obj)
-            if error:
-                return error
-        return None
+        try:
+            self._check_is_list(conf_obj)
+            self._check_is_list_of_dicts(conf_obj)
+
+            for section in conf_obj:
+                if 'type' not in section:
+                    raise ValidationError(
+                        'section without type: %r' % section)
+                elif section['type'] in ('trove', 'troves'):
+                    self._check_troves_section(section)
+                elif section['type'] == 'lorries':
+                    self._check_lorries_section(section)
+                else:
+                    raise ValidationError(
+                        'unknown section type %r' % section['type'])
+        except ValidationError as e:
+            return str(e)
 
-    def _find_checks(self):
-        return [
-            getattr(self, name)
-            for name in dir(self)
-            if name.startswith('_check_')]
+        return None
 
     def _check_is_list(self, conf_obj):
         if type(conf_obj) is not list:
-            return 'is not a JSON list'
-
+            raise ValidationError(
+                'type %r is not a JSON list' % type(conf_obj))
+        
     def _check_is_list_of_dicts(self, conf_obj):
         for item in conf_obj:
             if type(item) is not dict:
-                return 'all items must be dicts'
+                raise ValidationError('all items must be dicts')
 
-    def _check_sections_have_known_type(self, conf_obj):
-        allowed_types = ['lorries', 'trove', 'troves']
+    def _check_troves_section(self, section):
+        self._check_has_required_fields(
+            section, ['trovehost', 'interval', 'ls-interval', 'prefixmap'])
+        self._check_prefixmap(section)
 
-        for item in conf_obj:
-            if 'type' not in conf_obj:
-                return 'must have a type field'
-            if conf_obj['type'] not in allowed_types:
-                return ('must have a known type, not %r' % conf_obj['type'])
+    def _check_prefixmap(self, section):
+        pass
+
+    def _check_lorries_section(self, section):
+        self._check_has_required_fields(
+            section, ['interval', 'prefix', 'globs'])
+
+    def _check_has_required_fields(self, section, fields):
+        for field in fields:
+            if field not in section:
+                raise ValidationError(
+                    'mandatory field %s missing in section %r' %
+                    (field, section))
+
+
+class ValidationError(Exception):
+
+    def __init__(self, msg):
+        Exception.__init__(self, msg)
diff --git a/yarns.webapp/060-validation.yarn b/yarns.webapp/060-validation.yarn
index 4d2ce3b..a66e889 100644
--- a/yarns.webapp/060-validation.yarn
+++ b/yarns.webapp/060-validation.yarn
@@ -85,15 +85,17 @@ non-sensical value?
 
 Now we're getting to real sections. A `troves` section must have
 `trovehost`, `interval`, `ls-interval`, and `prefixmap` set, and may
-optionally have `ignore` set. Test each of those with good and bad
-values.
-
-    GIVEN an empty lorry-controller.conf in CONFGIT
-    AND lorry-controller.conf in CONFGIT adds trove example-trove
-    AND lorry-controller.conf in CONFGIT sets interval to "blah" for trove example-trove
-    WHEN admin makes request POST /1.0/read-configuration with dummy=value
-    THEN response matches "ERROR"
-    AND STATEDB is empty
+optionally have `ignore` set. The `trovehost` field can't really be
+checked, and `interval` and `ls-interval` don't need much checking: if
+they don't parse as sensible intervals, Lorry Controller will just use
+a default value.
+
+`prefixmap`, however, can have a reasonable check: it shouldn't map
+something to be under the Trove ID of the local Trove, otherwise Lorry
+won't be able to push the repositories. However, at this time, we do
+not have a reasonable way to get the Trove ID of the local Trove, so
+we're skipping implementing that test for now. (FIXME: fix this lack
+of testing.)
 
 Clean up at the end.