launchpad-reviewers team mailing list archive

[Colin Watson <mp+431753@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:postgresql-connection-uris into launchpad:master.

Commit message:
LaunchpadDatabase: Accept connection URIs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/431753

Use `parse_dsn` and `make_dsn` from `psycopg2.extensions` to manipulate connection strings, rather than mangling them by hand in a way that assumes keyword/value connection strings (see https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING).  This seems like better practice in general, and also starts preparing the way for being able to directly consume connection URIs provided by the PostgreSQL charm.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:postgresql-connection-uris into launchpad:master.

diff --git a/lib/lp/services/webapp/adapter.py b/lib/lp/services/webapp/adapter.py
index 28423d1..b40eed7 100644
--- a/lib/lp/services/webapp/adapter.py
+++ b/lib/lp/services/webapp/adapter.py
@@ -21,6 +21,8 @@ from psycopg2.extensions import (
     ISOLATION_LEVEL_REPEATABLE_READ,
     ISOLATION_LEVEL_SERIALIZABLE,
     QueryCanceledError,
+    make_dsn,
+    parse_dsn,
 )
 from storm.database import register_scheme
 from storm.databases.postgres import Postgres, PostgresTimeoutTracer
@@ -443,22 +445,11 @@ isolation_level_map = {
 
 
 class LaunchpadDatabase(Postgres):
-
-    _dsn_user_re = re.compile("user=[^ ]*")
-
     def __init__(self, uri):
         super().__init__(uri)
         # A unique name for this database connection.
         self.name = uri.database
 
-    @property
-    def dsn_without_user(self):
-        """This database's dsn without the 'user=...' bit."""
-        assert (
-            self._dsn is not None
-        ), "Must not be called before self._dsn has been set."
-        return self._dsn_user_re.sub("", self._dsn)
-
     def raw_connect(self):
         try:
             realm, flavor = self._uri.database.split("-")
@@ -475,7 +466,7 @@ class LaunchpadDatabase(Postgres):
         # is reconnected it pays attention to any config changes.
         config_entry = "%s_%s" % (realm, flavor)
         connection_string = getattr(dbconfig, config_entry)
-        assert "user=" not in connection_string, (
+        assert "user" not in parse_dsn(connection_string), (
             "Database username should not be specified in "
             "connection string (%s)." % connection_string
         )
@@ -484,7 +475,7 @@ class LaunchpadDatabase(Postgres):
         # fallback to the dbuser key.
         dbuser = getattr(dbconfig, "%s_dbuser" % realm, dbconfig.dbuser)
 
-        self._dsn = "%s user=%s" % (connection_string, dbuser)
+        self._dsn = make_dsn(connection_string, user=dbuser)
 
         flags = _get_dirty_commit_flags()
 
diff --git a/lib/lp/services/webapp/tests/test_adapter.py b/lib/lp/services/webapp/tests/test_adapter.py
new file mode 100644
index 0000000..ce03209
--- /dev/null
+++ b/lib/lp/services/webapp/tests/test_adapter.py
@@ -0,0 +1,70 @@
+# Copyright 2022 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from psycopg2.extensions import parse_dsn
+from zope.component import getUtility
+
+from lp.services.config import dbconfig
+from lp.services.database.interfaces import (
+    MAIN_STORE,
+    PRIMARY_FLAVOR,
+    IStoreSelector,
+)
+from lp.services.database.sqlbase import disconnect_stores
+from lp.testing import TestCase
+from lp.testing.layers import DatabaseFunctionalLayer, DatabaseLayer
+
+
+class TestLaunchpadDatabase(TestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_refuses_connection_string_with_user(self):
+        self.addCleanup(dbconfig.reset)
+        connstr = "dbname=%s user=foo" % DatabaseLayer._db_fixture.dbname
+        dbconfig.override(rw_main_primary=connstr)
+        disconnect_stores()
+        self.assertRaisesWithContent(
+            AssertionError,
+            "Database username should not be specified in connection string "
+            "(%s)." % connstr,
+            getUtility(IStoreSelector).get,
+            MAIN_STORE,
+            PRIMARY_FLAVOR,
+        )
+
+    def test_refuses_connection_string_uri_with_user(self):
+        self.addCleanup(dbconfig.reset)
+        connstr = "postgresql://foo@/%s" % DatabaseLayer._db_fixture.dbname
+        dbconfig.override(rw_main_primary=connstr)
+        disconnect_stores()
+        self.assertRaisesWithContent(
+            AssertionError,
+            "Database username should not be specified in connection string "
+            "(%s)." % connstr,
+            getUtility(IStoreSelector).get,
+            MAIN_STORE,
+            PRIMARY_FLAVOR,
+        )
+
+    def test_accepts_connection_string_without_user(self):
+        self.addCleanup(dbconfig.reset)
+        connstr = "dbname=%s" % DatabaseLayer._db_fixture.dbname
+        dbconfig.override(rw_main_primary=connstr, dbuser="ro")
+        disconnect_stores()
+        store = getUtility(IStoreSelector).get(MAIN_STORE, PRIMARY_FLAVOR)
+        self.assertEqual(
+            {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
+            parse_dsn(store.get_database()._dsn),
+        )
+
+    def test_accepts_connection_string_uri_without_user(self):
+        self.addCleanup(dbconfig.reset)
+        connstr = "postgresql:///%s" % DatabaseLayer._db_fixture.dbname
+        dbconfig.override(rw_main_primary=connstr, dbuser="ro")
+        disconnect_stores()
+        store = getUtility(IStoreSelector).get(MAIN_STORE, PRIMARY_FLAVOR)
+        self.assertEqual(
+            {"dbname": DatabaseLayer._db_fixture.dbname, "user": "ro"},
+            parse_dsn(store.get_database()._dsn),
+        )