Fix validation code and tests

2 files changed, 72 insertions, 10 deletions

diff --git a/lorrycontroller/readconf.py b/lorrycontroller/readconf.py
index ba0f463..ec0ae6d 100644
--- a/lorrycontroller/readconf.py
+++ b/lorrycontroller/readconf.py
@@ -108,9 +108,11 @@ class ReadConfiguration(lorrycontroller.LorryControllerRoute):
     def git_clone_confgit(self, confdir):
         url = self.app_settings['confgit-url']
         branch = self.app_settings['confgit-branch']
+        logging.info('Cloning %s to %s', url, confdir)
         cliapp.runcmd(['git', 'clone', '-b', branch, url, confdir])
 
     def git_pull_confgit(self, confdir):
+        logging.info('Updating CONFGIT in %s', confdir)
         cliapp.runcmd(['git', 'pull'], cwd=confdir)
 
     @property
@@ -123,16 +125,19 @@ class ReadConfiguration(lorrycontroller.LorryControllerRoute):
         '''Read the configuration file, return as Python object.'''
 
         filename = self.config_file_name
+        logging.debug('Reading configuration file %s', filename)
 
         try:
             with open(filename) as f:
                 return json.load(f)
         except IOError as e:
             if e.errno == errno.ENOENT:
-                # File doesn't exist. Return an empty configuration.
+                logging.debug(
+                    '%s: does not exist, returning empty config', filename)
                 return []
             bottle.abort(500, 'Error reading %s: %s' % (filename, e))
         except ValueError as e:
+            logging.error('Error parsing configuration: %s', e)
             raise LorryControllerConfParseError(filename, e)
 
     def validate_config(self, obj):
@@ -165,12 +170,16 @@ class ReadConfiguration(lorrycontroller.LorryControllerRoute):
         return int(number) * factor
 
     def add_matching_lorries_to_statedb(self, statedb, section):
+        logging.debug('Adding matching lorries to STATEDB')
+
         added_paths = set()
 
         filenames = self.find_lorry_files_for_section(section)
+        logging.debug('filenames=%r', filenames)
         lorry_specs = []
         for filename in sorted(filenames):
-            for lorry_spec in self.get_lorry_specs(filename):
+            logging.debug('Reading .lorry: %s', filename)
+            for lorry_spec in self.get_valid_lorry_specs(filename):
                 self.add_refspecs_if_missing(lorry_spec)
                 lorry_specs.append(lorry_spec)
 
@@ -200,10 +209,40 @@ class ReadConfiguration(lorrycontroller.LorryControllerRoute):
             result.extend(glob.glob(pattern))
         return result
     
-    def get_lorry_specs(self, filename):
-        with open(filename) as f:
-            obj = json.load(f)
-        return obj.items()
+    def get_valid_lorry_specs(self, filename):
+        # We do some basic validation of the .lorry file and the Lorry
+        # specs contained within it. We silently ignore anything that
+        # doesn't look OK. We don't have a reasonable mechanism to
+        # communicate any problems to the user, but we do log them to
+        # the log file.
+
+        try:
+            with open(filename) as f:
+                obj = json.load(f)
+        except ValueError as e:
+            logging.error('JSON problem in %s', filename)
+            return []
+
+        if type(obj) != dict:
+            logging.error('%s: does not contain a dict', filename)
+            return []
+
+        items = []
+        for key in obj:
+            if type(obj[key]) != dict:
+                logging.error(
+                    '%s: key %s does not map to a dict', filename, key)
+                continue
+
+            if 'type' not in obj[key]:
+                logging.error(
+                    '%s: key %s does not have type field', filename, key)
+                continue
+
+            logging.debug('Happy with Lorry spec %r: %r', key, obj[key])
+            items.append((key, obj[key]))
+
+        return items
 
     def add_refspecs_if_missing(self, lorry_spec):
         base_path, details = lorry_spec
diff --git a/yarns.webapp/060-validation.yarn b/yarns.webapp/060-validation.yarn
index 588d875..989c80b 100644
--- a/yarns.webapp/060-validation.yarn
+++ b/yarns.webapp/060-validation.yarn
@@ -137,30 +137,53 @@ Make sure WEBAPP handles there not being any `.lorry` files.
     WHEN admin makes request POST /1.0/read-configuration with dummy=value
     THEN response matches "has been updated"
     AND STATEDB is empty
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to []
 
 Add a `.lorry` file that contains broken JSON.
 
-    GIVEN Lorry file notjson.lorry with THIS IS NOT JSON
+    GIVEN Lorry file CONFGIT/notjson.lorry with THIS IS NOT JSON
+    WHEN admin makes request POST /1.0/read-configuration with dummy=value
     THEN response matches "has been updated"
     AND STATEDB is empty
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to []
 
 Add a `.lorry` file that is valid JSON, but is not a dict.
 
-    GIVEN Lorry file notadict.lorry with [1,2,3]
+    GIVEN Lorry file CONFGIT/notadict.lorry with [1,2,3]
+    WHEN admin makes request POST /1.0/read-configuration with dummy=value
     THEN response matches "has been updated"
     AND STATEDB is empty
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to []
 
 Add a `.lorry` that is a dict, but doesn't map keys to dicts.
 
-    GIVEN Lorry file notadictofdicts.lorry with { "foo": 1 }
+    GIVEN Lorry file CONFGIT/notadictofdicts.lorry with { "foo": 1 }
+    WHEN admin makes request POST /1.0/read-configuration with dummy=value
     THEN response matches "has been updated"
     AND STATEDB is empty
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to []
 
 Add a `.lorry` whose inner dict does not have a `type` field.
 
-    GIVEN Lorry file notadictofdicts.lorry with { "foo": { "bar": "yo" }}
+    GIVEN Lorry file CONFGIT/notype.lorry with { "foo": { "bar": "yo" }}
+    WHEN admin makes request POST /1.0/read-configuration with dummy=value
     THEN response matches "has been updated"
     AND STATEDB is empty
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to []
+
+Add a `.lorry` that is A-OK. This should work even when there are some
+broken ones too.
+
+    GIVEN Lorry file CONFGIT/a-ok.lorry with { "foo": { "type": "git", "url": "git://example.com/foo" }}
+    WHEN admin makes request POST /1.0/read-configuration with dummy=value
+    THEN response matches "has been updated"
+    WHEN admin makes request GET /1.0/list-queue
+    THEN response has queue set to ["upstream/foo"]
 
 Clean up at the end.