launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/fix-dbuser-override into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #842284 in Launchpad itself: "connect_string can't override the username if already specified"
  https://bugs.launchpad.net/launchpad/+bug/842284

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/fix-dbuser-override/+merge/74154

The long-deprecated canonical.database.sqlbase.connect() function is still used by a few scripts around the place. It takes user and DB names and returns a psycopg2 connection, bypassing Storm. Most of its dirty work is done by connect_string(), which merges the requested DB and usernames with rw_main_master.

c.l.scripts.db_options() does a similar thing with script commandline options, but merges them into the script's main config. It was changed in r13862 to also inject dbuser into the connection string, when it had previously just stored it for connection code to use manually.

Now, most sqlbase.connect() callsites call it with options.dbuser, because db_options() didn't originally inject it into the connection string. But connect_string() has an assertion to confirm that it isn't inserting a second dbuser field into the string -- this is obsolete since it now uses ConnectionString to *replace* the field, so there is no risk of duplication. So if db_options() sees a custom username (specified with the -U option), it will insert it into the connection string, causing scripts that call connect(options.dbuser) to crash with the obsolete assertion.

This has only manifested itself in staging updates so far, where we run "full-update.py -U postgres". The eventual solution is to fix the deprecated callers to not specify options.dbuser, but we need a quick fix for now.

This removal is tested on staging, and it unbreaks full-update.py. And it pretty clearly isn't going to break anything.
-- 
https://code.launchpad.net/~wgrant/launchpad/fix-dbuser-override/+merge/74154
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/fix-dbuser-override into lp:launchpad.

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2011-09-02 14:02:57 +0000
+++ lib/canonical/database/sqlbase.py	2011-09-06 02:43:23 +0000
@@ -808,8 +808,6 @@
     # directly.
     from canonical.database.postgresql import ConnectionString
     con_str = ConnectionString(dbconfig.rw_main_master)
-    assert con_str.user is None, (
-        'Connection string already contains username')
     if user is not None:
         con_str.user = user
     if lp.dbhost is not None: