launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/overlayless-ztm into lp:launchpad with lp:~wgrant/launchpad/overridable-dbconfig as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/overlayless-ztm/+merge/76333

Part of my continuing campaign to exterminate ZopelessTransactionManager, this branch turns it into a reasonably small wrapper around DatabaseConfig.override (added in the prereq branch), rather than using a lazr.config overlay on top of the 'launchpad' database config section.

test_initZopelessTwice is gone -- only infrastructure code is calls initZopeless now, and overriding twice is harmless, so not much point keeping the warning and hideous test.

The behaviour of reconnect_stores() (used only by tests) has changed. It previously defaulted to overriding to the values in the 'launchpad' config section, but that no longer works properly -- initZopeless sets overrides directly, rather than altering the base config, so reconnect_stores() ended up clobbering settings. So now it defaults to preserving the existing overrides, and callsites that want the old reset behaviour call reconnect_stores('launchpad') instead. This will all be cleaned up more thoroughly when database config sections are abolished next week.
-- 
https://code.launchpad.net/~wgrant/launchpad/overlayless-ztm/+merge/76333
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/overlayless-ztm into lp:launchpad.

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2011-09-18 08:55:07 +0000
+++ lib/canonical/database/sqlbase.py	2011-09-21 06:11:21 +0000
@@ -3,11 +3,9 @@
 
 __metaclass__ = type
 __all__ = [
-    'alreadyInstalledMsg',
     'block_implicit_flushes',
     'clear_current_connection_cache',
     'commit',
-    'ConflictingTransactionManagerError',
     'connect',
     'convert_storm_clause_to_string',
     'cursor',
@@ -32,8 +30,6 @@
 
 
 from datetime import datetime
-from textwrap import dedent
-import warnings
 
 from lazr.restful.interfaces import IRepresentationCache
 import psycopg2
@@ -61,10 +57,7 @@
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.config import (
-    config,
-    dbconfig,
-    )
+from canonical.config import dbconfig
 from canonical.database.interfaces import ISQLBase
 from lp.services.propertycache import clear_property_cache
 
@@ -269,19 +262,10 @@
         clear_property_cache(self)
 
 
-alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
-"already installed.  This is probably caused by calling initZopeless twice.")
-
-
-class ConflictingTransactionManagerError(Exception):
-    pass
-
-
 class ZopelessTransactionManager(object):
     """Compatibility shim for initZopeless()"""
 
     _installed = None
-    _CONFIG_OVERLAY_NAME = 'initZopeless config overlay'
 
     def __init__(self):
         raise AssertionError("ZopelessTransactionManager should not be "
@@ -299,32 +283,12 @@
             ISOLATION_LEVEL_READ_COMMITTED: 'read_committed',
             ISOLATION_LEVEL_SERIALIZABLE: 'serializable'}[isolation]
 
-        # Construct a config fragment:
-        overlay = dedent("""\
-            [database]
-            isolation_level: %(isolation_level)s
-
-            [launchpad]
-            dbuser: %(dbuser)s
-            """ % dict(
-                isolation_level=isolation_level,
-                dbuser=dbuser))
-
-        if cls._installed is not None:
-            if cls._config_overlay != overlay:
-                raise ConflictingTransactionManagerError(
-                        "A ZopelessTransactionManager with different "
-                        "settings is already installed")
-            # There's an identical ZopelessTransactionManager already
-            # installed, so return that one, but also emit a warning.
-            warnings.warn(alreadyInstalledMsg, stacklevel=3)
-        else:
-            config.push(cls._CONFIG_OVERLAY_NAME, overlay)
-            cls._config_overlay = overlay
-            cls._dbuser = dbuser
-            cls._isolation = isolation
-            cls._reset_stores()
-            cls._installed = cls
+        dbconfig.override(dbuser=dbuser, isolation_level=isolation_level)
+
+        cls._dbuser = dbuser
+        cls._isolation = isolation
+        cls._reset_stores()
+        cls._installed = cls
 
     @staticmethod
     def _reset_stores():
@@ -362,7 +326,7 @@
         """
         assert cls._installed is not None, (
             "ZopelessTransactionManager not installed")
-        config.pop(cls._CONFIG_OVERLAY_NAME)
+        dbconfig.override(dbuser=None, isolation_level=None)
         cls._reset_stores()
         cls._installed = None
 

=== modified file 'lib/canonical/database/tests/test_zopeless_transaction_manager.py'
--- lib/canonical/database/tests/test_zopeless_transaction_manager.py	2011-09-20 01:55:36 +0000
+++ lib/canonical/database/tests/test_zopeless_transaction_manager.py	2011-09-21 06:11:21 +0000
@@ -1,19 +1,11 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-import warnings
-
 from storm.zope.interfaces import IZStorm
 from zope.component import getUtility
 
-from canonical.database.sqlbase import (
-    alreadyInstalledMsg,
-    ZopelessTransactionManager,
-    )
-from canonical.testing.layers import (
-    LaunchpadZopelessLayer,
-    ZopelessDatabaseLayer,
-    )
+from canonical.database.sqlbase import ZopelessTransactionManager
+from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.testing import TestCase
 
 
@@ -30,40 +22,3 @@
         new_active_stores = [
             item[0] for item in getUtility(IZStorm).iterstores()]
         self.assertContentEqual(active_stores, new_active_stores)
-
-
-class TestInitZopeless(TestCase):
-
-    layer = ZopelessDatabaseLayer
-
-    def test_initZopelessTwice(self):
-        # Hook the warnings module, so we can verify that we get the expected
-        # warning.  The warnings module has two key functions, warn and
-        # warn_explicit, the first calling the second. You might, therefore,
-        # think that we should hook the second, to catch all warnings in one
-        # place.  However, from Python 2.6, both of these are replaced with
-        # entries into a C extension if available, and the C implementation of
-        # the first will not call a monkeypatched Python implementation of the
-        # second.  Therefore, we hook warn, as is the one actually called by
-        # the particular code we are interested in testing.
-        original_warn = warnings.warn
-        warnings.warn = self.warn_hooked
-        self.warned = False
-        try:
-            # Calling initZopeless with the same arguments twice should emit
-            # a warning.
-            try:
-                ZopelessTransactionManager.initZopeless(
-                    dbuser='launchpad')
-                ZopelessTransactionManager.initZopeless(
-                    dbuser='launchpad')
-                self.failUnless(self.warned)
-            finally:
-                ZopelessTransactionManager.uninstall()
-        finally:
-            # Put the warnings module back the way we found it.
-            warnings.warn = original_warn
-
-    def warn_hooked(self, message, category=None, stacklevel=1):
-        self.failUnlessEqual(alreadyInstalledMsg, str(message))
-        self.warned = True

=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py	2011-09-21 06:11:21 +0000
+++ lib/canonical/testing/layers.py	2011-09-21 06:11:21 +0000
@@ -210,17 +210,18 @@
             store.close()
 
 
-def reconnect_stores(database_config_section='launchpad'):
+def reconnect_stores(database_config_section=None):
     """Reconnect Storm stores, resetting the dbconfig to its defaults.
 
     After reconnecting, the database revision will be checked to make
     sure the right data is available.
     """
     disconnect_stores()
-    section = getattr(config, database_config_section)
-    dbconfig.override(
-        dbuser=getattr(section, 'dbuser', None),
-        isolation_level=getattr(section, 'isolation_level', None))
+    if database_config_section:
+        section = getattr(config, database_config_section)
+        dbconfig.override(
+            dbuser=getattr(section, 'dbuser', None),
+            isolation_level=getattr(section, 'isolation_level', None))
 
     main_store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
     assert main_store is not None, 'Failed to reconnect'
@@ -1352,7 +1353,7 @@
     @profiled
     def testSetUp(cls):
         # Connect Storm
-        reconnect_stores()
+        reconnect_stores('launchpad')
 
     @classmethod
     @profiled
@@ -1383,7 +1384,7 @@
         OpStats.resetStats()
 
         # Connect Storm
-        reconnect_stores()
+        reconnect_stores('launchpad')
 
     @classmethod
     @profiled
@@ -1450,7 +1451,7 @@
     def testSetUp(cls):
         # LaunchpadZopelessLayer takes care of reconnecting the stores
         if not LaunchpadZopelessLayer.isSetUp:
-            reconnect_stores()
+            reconnect_stores('launchpad')
 
     @classmethod
     @profiled
@@ -1488,7 +1489,7 @@
     def testSetUp(cls):
         # LaunchpadZopelessLayer takes care of reconnecting the stores
         if not LaunchpadZopelessLayer.isSetUp:
-            reconnect_stores()
+            reconnect_stores('launchpad')
 
     @classmethod
     @profiled