launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04887
[Merge] lp:~wgrant/launchpad/connectionstring-hates-production into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/connectionstring-hates-production into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #843412 in Launchpad itself: "ConnectionString assumes that values are in \w+"
https://bugs.launchpad.net/launchpad/+bug/843412
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/connectionstring-hates-production/+merge/74321
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
--
https://code.launchpad.net/~wgrant/launchpad/connectionstring-hates-production/+merge/74321
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/connectionstring-hates-production into lp:launchpad.
=== 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:35:23 +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:35:23 +0000
@@ -0,0 +1,39 @@
+# Copyright 2009 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=bar'
+ cs = ConnectionString(initial)
+ cs.user = 'baz'
+ self.assertEqual(expected, str(cs))
+
+ def test_rejects_quoted_strings(self):
+ self.assertRaises(
+ AssertionError, ConnectionString, "dbname='quoted string'")