Merge lp:~wgrant/launchpad/connectionstring-hates-production into lp:launchpad

[William Grant]

ConnectionString now assumes values continue until a space character, and rejects values that seem to be quoted or escaped. Also now has better tests.

This branch fixes ConnectionString to not truncate production DB hostnames. See bug #843412 for details of the problem.

As I suggested in the bug, I've not implemented escaping or quoting since we don't need them in production, and they would greatly increase complexity and time to fix. I have, however, added assertions so we crash instead of misparse.

I used gina against the production configs to verify the fix:

$ LPCONFIG=gina scripts/gina.py -vv lenny # before
2011-09-07 00:30:15 DEBUG Cronscript control file not found at file:cronscripts.ini
2011-09-07 00:30:15 INFO Creating lockfile: /var/lock/launchpad-gina.lock
2011-09-07 00:30:21 INFO
2011-09-07 00:30:21 INFO === Processing debian/lenny/release ===
2011-09-07 00:30:21 DEBUG Packages read from: /srv/debian-import.launchpad.net/mirror/
2011-09-07 00:30:21 DEBUG Keyrings read from: /usr/share/keyrings
2011-09-07 00:30:21 INFO Components to import: main, contrib, non-free
2011-09-07 00:30:21 INFO Architectures to import: i386, powerpc, amd64
2011-09-07 00:30:21 DEBUG Launchpad database: launchpad_prod_3
2011-09-07 00:30:21 DEBUG Launchpad database host: lp
2011-09-07 00:30:21 DEBUG Launchpad database user: gina

$ LPCONFIG=gina scripts/gina.py -vv lenny # after
2011-09-07 00:29:36 DEBUG Cronscript control file not found at file:cronscripts.ini
2011-09-07 00:29:36 INFO Creating lockfile: /var/lock/launchpad-gina.lock
2011-09-07 00:29:41 INFO
2011-09-07 00:29:41 INFO === Processing debian/lenny/release ===
2011-09-07 00:29:41 DEBUG Packages read from: /srv/debian-import.launchpad.net/mirror/
2011-09-07 00:29:41 DEBUG Keyrings read from: /usr/share/keyrings
2011-09-07 00:29:41 INFO Components to import: main, contrib, non-free
2011-09-07 00:29:41 INFO Architectures to import: i386, powerpc, amd64
2011-09-07 00:29:41 DEBUG Launchpad database: launchpad_prod_3
2011-09-07 00:29:41 DEBUG Launchpad database host: lp-db-master.canonical.com
2011-09-07 00:29:41 DEBUG Launchpad database user: gina

[Ian Booth (wallyworld)]

Looks good

review: Approve (code)

=== modified file 'lib/canonical/database/postgresql.py'
--- lib/canonical/database/postgresql.py	2009-09-21 08:13:40 +0000
+++ lib/canonical/database/postgresql.py	2011-09-07 00:46:24 +0000
@@ -482,6 +482,8 @@
     need the components seperated out (such as pg_dump command line
     arguments). This class allows you to switch easily between formats.
 
+    Quoted or escaped values are not supported.
+
     >>> cs = ConnectionString('user=foo dbname=launchpad_dev')
     >>> cs.dbname
     'launchpad_dev'
@@ -496,6 +498,9 @@
         'dbname', 'user', 'host', 'port', 'connect_timeout', 'sslmode']
 
     def __init__(self, conn_str):
+        if "'" in conn_str or "\\" in conn_str:
+            raise AssertionError("quoted or escaped values are not supported")
+
         if '=' not in conn_str:
             # Just a dbname
             for key in self.CONNECTION_KEYS:
@@ -507,7 +512,7 @@
             # be added after construction or not actually required
             # at all in some instances.
             for key in self.CONNECTION_KEYS:
-                match = re.search(r'%s=(\w+)' % key, conn_str)
+                match = re.search(r'%s=([^ ]+)' % key, conn_str)
                 if match is None:
                     setattr(self, key, None)
                 else:

=== added file 'lib/canonical/database/tests/test_connectionstring.py'
--- lib/canonical/database/tests/test_connectionstring.py	1970-01-01 00:00:00 +0000
+++ lib/canonical/database/tests/test_connectionstring.py	2011-09-07 00:46:24 +0000
@@ -0,0 +1,40 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from canonical.database.postgresql import ConnectionString
+from lp.testing import TestCase
+
+
+class TestConnectionString(TestCase):
+
+    def test_relevant_fields_parsed(self):
+        s = ('dbname=dbname user=user host=host port=port '
+             'connect_timeout=timeout sslmode=mode')
+        cs = ConnectionString(s)
+        self.assertEqual('dbname', cs.dbname)
+        self.assertEqual('user', cs.user)
+        self.assertEqual('host', cs.host)
+        self.assertEqual('port', cs.port)
+        self.assertEqual('timeout', cs.connect_timeout)
+        self.assertEqual('mode', cs.sslmode)
+
+        # and check that str/repr have the same keys and values.
+        self.assertContentEqual(s.split(), str(cs).split())
+        self.assertContentEqual(s.split(), repr(cs).split())
+
+    def test_hyphens_in_values(self):
+        cs = ConnectionString('user=foo-bar host=foo.bar-baz.quux')
+        self.assertEqual('foo-bar', cs.user)
+        self.assertEqual('foo.bar-baz.quux', cs.host)
+
+    def test_str_with_changes(self):
+        initial = 'dbname=foo host=bar'
+        expected = 'dbname=foo user=baz host=blah'
+        cs = ConnectionString(initial)
+        cs.host = 'blah'
+        cs.user = 'baz'
+        self.assertEqual(expected, str(cs))
+
+    def test_rejects_quoted_strings(self):
+        self.assertRaises(
+            AssertionError, ConnectionString, "dbname='quoted string'")