Support facade arguments, unstarted facade for patch_engine()

Fixed two issues in patch_engine() as used in test suites
where facade-level arguments would not be propagated to the
internal _TestTransactionFactory, and also if the parent
factory weren't "started" the patch operation would fail
as it assumes a started factory.

This removes the "synchronous_reader" argument from
_TestTransactionFactory as this is propagated from the parent
_TransactionFactory, where synchronous_reader defaults to True.
However, arbitrary keyword arguments are currently allowed to
allow for outside projects calling _TestTransactionFactory
(currently Neutron).

Change-Id: I15f85a0ebfd732c09d7cb92d885c1773c393aabd

3 files changed, 68 insertions, 10 deletions

diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py
index 2b51ec3..057e89e 100644
--- a/oslo_db/sqlalchemy/enginefacade.py
+++ b/oslo_db/sqlalchemy/enginefacade.py
@@ -18,6 +18,7 @@ import operator
 import threading
 import warnings
 
+import debtcollector.removals as removals
 from oslo_config import cfg
 
 from oslo_db import exception
@@ -516,16 +517,25 @@ class _TestTransactionFactory(_TransactionFactory):
     within the global :class:`._TransactionContextManager`.
 
     """
-    def __init__(self, engine, maker, apply_global, synchronous_reader):
+
+    @removals.removed_kwarg(
+        'synchronous_reader',
+        'argument value is propagated from the parent _TransactionFactory')
+    def __init__(self, engine, maker, apply_global, from_factory=None, **kw):
+        # NOTE(zzzeek): **kw needed for backwards compability
         self._reader_engine = self._writer_engine = engine
         self._reader_maker = self._writer_maker = maker
         self._started = True
         self._legacy_facade = None
-        self.synchronous_reader = synchronous_reader
 
-        self._facade_cfg = _context_manager._factory._facade_cfg
-        self._transaction_ctx_cfg = \
-            _context_manager._factory._transaction_ctx_cfg
+        if from_factory is None:
+            from_factory = _context_manager._factory
+
+        self._facade_cfg = from_factory._facade_cfg
+        self._transaction_ctx_cfg = from_factory._transaction_ctx_cfg
+
+        self.synchronous_reader = self._facade_cfg['synchronous_reader']
+
         if apply_global:
             self.existing_factory = _context_manager._factory
             _context_manager._root_factory = self
@@ -852,6 +862,8 @@ class _TransactionContextManager(object):
         """
 
         existing_factory = self._factory
+        if not existing_factory._started:
+            existing_factory._start()
         maker = existing_factory._writer_maker
         maker_kwargs = existing_factory._maker_args_for_conf(cfg.CONF)
         maker = orm.get_maker(engine=engine, **maker_kwargs)
@@ -859,8 +871,7 @@ class _TransactionContextManager(object):
         factory = _TestTransactionFactory(
             engine, maker,
             apply_global=False,
-            synchronous_reader=existing_factory.
-            _facade_cfg['synchronous_reader']
+            from_factory=existing_factory
         )
         return self.patch_factory(factory)
 
diff --git a/oslo_db/sqlalchemy/test_base.py b/oslo_db/sqlalchemy/test_base.py
index ab7dea2..4c9c2c7 100644
--- a/oslo_db/sqlalchemy/test_base.py
+++ b/oslo_db/sqlalchemy/test_base.py
@@ -86,8 +86,7 @@ class DbFixture(fixtures.Fixture):
         self.addCleanup(setattr, self.test, 'engine', None)
 
         self.test.enginefacade = enginefacade._TestTransactionFactory(
-            self.test.engine, self.test.sessionmaker, apply_global=True,
-            synchronous_reader=True)
+            self.test.engine, self.test.sessionmaker, apply_global=True)
         self.addCleanup(self.test.enginefacade.dispose_global)
 
 
diff --git a/oslo_db/tests/sqlalchemy/test_enginefacade.py b/oslo_db/tests/sqlalchemy/test_enginefacade.py
index 9d84c1a..a606fae 100644
--- a/oslo_db/tests/sqlalchemy/test_enginefacade.py
+++ b/oslo_db/tests/sqlalchemy/test_enginefacade.py
@@ -1160,7 +1160,10 @@ class PatchFactoryTest(oslo_test_base.BaseTestCase):
 
     def test_patch_engine(self):
         normal_mgr = enginefacade.transaction_context()
-        normal_mgr.configure(connection="sqlite:///foo.db")
+        normal_mgr.configure(
+            connection="sqlite:///foo.db",
+            rollback_reader_sessions=True
+        )
 
         @normal_mgr.writer
         def go1(context):
@@ -1179,6 +1182,17 @@ class PatchFactoryTest(oslo_test_base.BaseTestCase):
                 s1.bind.url,
                 "sqlite:///bar.db")
 
+            self.assertTrue(
+                enginefacade._transaction_ctx_for_context(
+                    context).rollback_reader_sessions
+            )
+
+            # ensure this defaults to True
+            self.assertTrue(
+                enginefacade._transaction_ctx_for_context(
+                    context).factory.synchronous_reader
+            )
+
         def create_engine(sql_connection, **kw):
             return mock.Mock(url=sql_connection)
 
@@ -1195,6 +1209,40 @@ class PatchFactoryTest(oslo_test_base.BaseTestCase):
             reset()
             go1(context)
 
+    def test_patch_not_started(self):
+        normal_mgr = enginefacade.transaction_context()
+        normal_mgr.configure(
+            connection="sqlite:///foo.db",
+            rollback_reader_sessions=True
+        )
+
+        @normal_mgr.writer
+        def go1(context):
+            s1 = context.session
+
+            self.assertEqual(
+                s1.bind.url,
+                "sqlite:///bar.db")
+
+            self.assertTrue(
+                enginefacade._transaction_ctx_for_context(
+                    context).rollback_reader_sessions
+            )
+
+        def create_engine(sql_connection, **kw):
+            return mock.Mock(url=sql_connection)
+
+        with mock.patch(
+                "oslo_db.sqlalchemy.engines.create_engine", create_engine):
+            mock_engine = create_engine("sqlite:///bar.db")
+
+            context = oslo_context.RequestContext()
+            reset = normal_mgr.patch_engine(mock_engine)
+            go1(context)
+            self.assertIs(
+                normal_mgr._factory._writer_engine, mock_engine)
+            reset()
+
     def test_new_manager_from_config(self):
         normal_mgr = enginefacade.transaction_context()
         normal_mgr.configure(