Merge lp:~adeuring/charmworld/better-handling-of-invalid-metadata-1169662 into lp:~juju-jitsu/charmworld/trunk

[Abel Deuring]

ingest: stop processing charms with invalid metadata or config data.

This branch fixes the bugs 1169662 (UnboundLocalError: local variable
'metadata' referenced before assignment) and 1169660 (UnboundLocalError:
local variable 'config' referenced before assignment).

The cause of the bugs is simple: The ingest script just tried to
continue, even if the files metadata.yaml or config.yaml are invalid.

ScanJob now raises the new exception IngestError if these files cannot
be parsed; I changed the function run_job() to log the new exception
at level "info".

I also added a few trivial tests for run_job().

[Richard Harding (rharding)]

thanks

review: Approve

=== modified file 'charmworld/jobs/ingest.py'
--- charmworld/jobs/ingest.py	2013-04-25 16:44:55 +0000
+++ charmworld/jobs/ingest.py	2013-04-26 13:00:39 +0000
@@ -58,6 +58,10 @@
     "%(series)s-%(provider)s-charm-%(charm)s/lastBuild/api/json")
 
 
+class IngestError(Exception):
+    pass
+
+
 class IngestJob(object):
 
     name = 'default'
@@ -190,6 +194,17 @@
             self.update_charm(charm_data, branch_dir, retry)
 
 
+def log(stage, level, exc, charm_data):
+    logger = logging.getLogger("charm.%s" % stage)
+    err_msg = "%s error on %s: %s" % (
+        stage, charm_data, str(exc))
+    logger.log(level, err_msg)
+    charm_error = {'error_stage': stage, 'error': str(exc)}
+    if hasattr(exc, 'output'):
+        charm_error['output'] = str(exc.output)
+    charm_data['error'] = charm_error
+
+
 def run_job(job, charm_data, needs_setup=True, db=None):
     if needs_setup:
         if db is not None:
@@ -197,17 +212,13 @@
         else:
             job.setup()
     stage = job.name
-    log = logging.getLogger("charm.%s" % stage)
     try:
         job.run(charm_data)
+    except IngestError as e:
+        log(stage, logging.INFO, e, charm_data)
+        return False
     except Exception as e:
-        err_msg = "%s error on %s: %s" % (
-            stage, charm_data, str(e))
-        log.exception(err_msg)
-        charm_error = {'error_stage': stage, 'error': str(e)}
-        if hasattr(e, 'output'):
-            charm_error['output'] = str(e.output)
-        charm_data['error'] = charm_error
+        log(stage, logging.ERROR, e, charm_data)
         return False
     return True
 
@@ -577,7 +588,8 @@
             try:
                 metadata = quote_yaml(yaml.load(cfile.read()))
             except Exception, exc:
-                self.log.info('Invalid charm metadata %s: %s' % (
+                raise IngestError(
+                    'Invalid charm metadata %s: %s' % (
                     charm_data['branch_spec'],
                     exc)
                 )
@@ -590,7 +602,7 @@
             try:
                 config = quote_yaml(yaml.load(config_raw))
             except Exception, exc:
-                self.log.info(
+                raise IngestError(
                     'Invalid charm config yaml. %s: %s' % (
                         charm_data['branch_spec'],
                         exc)

=== modified file 'charmworld/jobs/tests/test_ingest.py'
--- charmworld/jobs/tests/test_ingest.py	2013-04-24 20:31:51 +0000
+++ charmworld/jobs/tests/test_ingest.py	2013-04-26 13:00:39 +0000
@@ -19,6 +19,8 @@
 
 from charmworld.charmstore import get_address
 from charmworld.jobs.ingest import (
+    IngestError,
+    IngestJob,
     run_job,
     update_charm,
     UpdateCharmJob,
@@ -171,3 +173,83 @@
         with update_environment(charm_data, self.use_index_client()):
             update_charm(charm_data, self.db)
         self.assertIs(None, self.db.charms.find_one(charm_data['_id']))
+
+
+class TestJob(IngestJob):
+    name = 'test'
+
+    def __init__(self, exc=None):
+        super(TestJob, self).setup()
+        self.setup_called = False
+        self.run_called = False
+        self.exc = exc
+        self.charm_data = None
+
+    def setup(self, db=None):
+        self.setup_called = True
+        self.db = db
+
+    def run(self, charm_data):
+        self.charm_data = charm_data
+        if self.exc is not None:
+            raise self.exc('test error')
+        self.run_called = True
+
+
+class TestRunJob(TestCase):
+
+    def test_run_job_defaults(self):
+        job = TestJob()
+        result = run_job(job, charm_data='foo')
+        self.assertTrue(result)
+        self.assertTrue(job.setup_called)
+        self.assertTrue(job.run_called)
+        self.assertEqual('foo', job.charm_data)
+        self.assertIs(None, job.db)
+
+    def test_run_job_no_setup(self):
+        job = TestJob()
+        run_job(job, 'foo', needs_setup=False)
+        self.assertFalse(job.setup_called)
+
+    def test_run_job_db_specified(self):
+        job = TestJob()
+        run_job(job, 'foo', db='db')
+        self.assertEqual('db', job.db)
+        job = TestJob()
+        run_job(job, 'foo', needs_setup=False, db='db')
+        self.assertIs(None, getattr(job, 'db', None))
+
+    def test_run_job_general_exception(self):
+        handler = self.get_handler('charm.test')
+        job = TestJob(ValueError)
+        charm_data = {'foo': 'bar'}
+        result = run_job(job, charm_data)
+        self.assertFalse(result)
+        expected_error = {
+            'error': 'test error',
+            'error_stage': 'test',
+        }
+        self.assertEqual(expected_error, charm_data['error'])
+        log_messages = [record.getMessage() for record in handler.buffer]
+        expected = ["test error on {'foo': 'bar'}: test error"]
+        self.assertEqual(expected, log_messages)
+        log_level = handler.buffer[0].levelname
+        self.assertEqual('ERROR', log_level)
+
+    def test_run_job_IngestError(self):
+        handler = self.get_handler('charm.test')
+        job = TestJob(IngestError)
+        charm_data = {'foo': 'bar'}
+        result = run_job(job, charm_data)
+        self.assertFalse(result)
+        expected_error = {
+            'error': 'test error',
+            'error_stage': 'test',
+        }
+        self.assertEqual(expected_error, charm_data['error'])
+        log_messages = [record.getMessage() for record in handler.buffer]
+        expected = ["test error on {'foo': 'bar'}: test error"]
+        self.assertEqual(expected, log_messages)
+        log_level = handler.buffer[0].levelname
+        self.assertEqual('INFO', log_level)

=== modified file 'charmworld/jobs/tests/test_scan.py'
--- charmworld/jobs/tests/test_scan.py	2013-04-25 18:08:08 +0000
+++ charmworld/jobs/tests/test_scan.py	2013-04-26 13:00:39 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from charmworld.jobs.ingest import BzrIngestJob
+from charmworld.jobs.ingest import IngestError
 from charmworld.jobs.ingest import ScanIngestJob
 from charmworld.testing import factory
 from charmworld.testing import JobTestBase
@@ -61,3 +62,29 @@
             'website': {'interface': 'http'},
         }
         self.assertEqual(expected, self.charm['provides'])
+
+    def test_invalid_metadata_raises_IngestError(self):
+        metadata = 'invalid_syntax: ['
+        metadata_file = factory.make_charm_file(
+            self.db, self.charm, '/path/to/metadata.yaml', content=metadata)
+        self.charm['files'][quote_key('metadata.yaml')] = {
+            'fileid': metadata_file._id
+        }
+        with self.assertRaises(IngestError) as ctx:
+            self.scan_job.run(self.charm)
+        info = str(ctx.exception)
+        self.assertTrue(info.startswith(
+            'Invalid charm metadata ~charmers/charms/precise/sample'))
+
+    def test_invalid_config_yaml_raises_IngestError(self):
+        config = 'invalid_syntax: ['
+        config_file = factory.make_charm_file(
+            self.db, self.charm, '/path/to/config.yaml', content=config)
+        self.charm['files'][quote_key('config.yaml')] = {
+            'fileid': config_file._id
+        }
+        with self.assertRaises(IngestError) as ctx:
+            self.scan_job.run(self.charm)
+        info = str(ctx.exception)
+        self.assertTrue(info.startswith(
+            'Invalid charm config yaml. ~charmers/charms/precise/sample'))

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-04-18 20:11:19 +0000
+++ charmworld/testing/factory.py	2013-04-26 13:00:39 +0000
@@ -271,10 +271,12 @@
     return record_id, charm
 
 
-def make_charm_file(db, charm, path):
+def make_charm_file(db, charm, path, content=None):
     fs = getfs(db)
     charm_file = CharmFileSet.make_charm_file(fs, charm, path, basename(path))
-    charm_file.save(random_string(10).encode('utf-8'))
+    if content is None:
+        content = random_string(10).encode('utf-8')
+    charm_file.save(content)
     return charm_file