Improve failure mode handling in enginefacade

Check explicitly for the cases where no "sql_connection"
attribute was set when running _start(), so that the
lack of this parameter is documented by the
exception rather than failing into create_engine()
with an unclear failure mode.
If _start() fails as it will here, make sure _started
stays False so that repeated calls to _start() continue
to raise the same exception, rather than raising
attribute errors.  When accessing the "session" or
"connection" attributes of the context when these
attributes were not requested by the decorator or
context manager, raise explicit exceptions
for each, rather than returning None which leads to
hard-to-debug NoneType errors.

Change-Id: Iadfbf4707daed4140285a3a472009f6863b18275
Closes-bug: 1477080

doc/source/usage.rst

@@ -76,6 +76,12 @@ positional argument:
         results = some_reader_api_function(context)
         some_writer_api_function(context, 5, 10)
 
+.. note::  The ``context.session`` and ``context.connection`` attributes
+   must be accessed within the scope of an appropriate writer/reader block
+   (either the decorator or contextmanager approach). An AttributeError is
+   raised otherwise.
+
+
 The scope of transaction and connectivity for both approaches is managed
 transparently.   The configuration for the connection comes from the standard
 :obj:`oslo_config.cfg.CONF` collection.  Additional configurations can be

oslo_db/exception.py

@@ -208,12 +208,12 @@ class RetryRequest(Exception):
 
 
 class NoEngineContextEstablished(AttributeError):
-    """Error raised for non-present enginefacade attribute access.
+    """Error raised for enginefacade attribute access with no context.
 
 
     This applies to the ``session`` and ``connection`` attributes
     of a user-defined context and/or RequestContext object, when they
-    are accessed outside of the scope of an enginefacade decorator
+    are accessed *outside* of the scope of an enginefacade decorator
     or context manager.
 
     The exception is a subclass of AttributeError so that
@@ -224,6 +224,23 @@ class NoEngineContextEstablished(AttributeError):
     """
 
 
+class ContextNotRequestedError(AttributeError):
+    """Error raised when requesting a not-setup enginefacade attribute.
+
+    This applies to the ``session`` and ``connection`` attributes
+    of a user-defined context and/or RequestContext object, when they
+    are accessed *within* the scope of an enginefacade decorator
+    or context manager, but the context has not requested that
+    attribute (e.g. like "with enginefacade.connection.using(context)"
+    and "context.session" is requested).
+
+    """
+
+
+class CantStartEngineError(Exception):
+    """Error raised when the enginefacade cannot start up correctly."""
+
+
 class NotSupportedWarning(Warning):
     """Warn that an argument or call that was passed is not supported.
 

oslo_db/sqlalchemy/enginefacade.py

@@ -314,7 +314,6 @@ class _TransactionFactory(object):
             # for the lock.
             if self._started:
                 return
-            self._started = True
             if conf is False:
                 conf = cfg.CONF
 
@@ -349,8 +348,16 @@ class _TransactionFactory(object):
 
             self.synchronous_reader = self._facade_cfg['synchronous_reader']
 
+            # set up _started last, so that in case of exceptions
+            # we try the whole thing again and report errors
+            # correctly
+            self._started = True
+
     def _setup_for_connection(
             self, sql_connection, engine_kwargs, maker_kwargs):
+        if sql_connection is None:
+            raise exception.CantStartEngineError(
+                "No sql_connection parameter is established")
         engine = engines.create_engine(
             sql_connection=sql_connection, **engine_kwargs)
         sessionmaker = orm.get_maker(engine=engine, **maker_kwargs)
@@ -736,7 +743,13 @@ def _context_descriptor(attr=None):
                 % (context, attr)
             )
         else:
-            return getter(transaction_context)
+            result = getter(transaction_context)
+            if result is None:
+                raise exception.ContextNotRequestedError(
+                    "The '%s' context attribute was requested but "
+                    "it has not been established for this context." % attr
+                )
+            return result
     return property(_property_for_context)
 
 

oslo_db/tests/sqlalchemy/test_enginefacade.py

@@ -770,6 +770,36 @@ class MockFacadeTest(oslo_test_base.BaseTestCase):
                 conn.execute("test1")
                 conn.execute("test2")
 
+    def test_session_context_notrequested_exception(self):
+        context = oslo_context.RequestContext()
+
+        with enginefacade.reader.connection.using(context):
+            exc = self.assertRaises(
+                exception.ContextNotRequestedError,
+                getattr, context, 'session'
+            )
+
+            self.assertRegexpMatches(
+                exc.args[0],
+                "The 'session' context attribute was requested but it has "
+                "not been established for this context."
+            )
+
+    def test_connection_context_notrequested_exception(self):
+        context = oslo_context.RequestContext()
+
+        with enginefacade.reader.using(context):
+            exc = self.assertRaises(
+                exception.ContextNotRequestedError,
+                getattr, context, 'connection'
+            )
+
+            self.assertRegexpMatches(
+                exc.args[0],
+                "The 'connection' context attribute was requested but it has "
+                "not been established for this context."
+            )
+
     def test_session_context_exception(self):
         context = oslo_context.RequestContext()
         exc = self.assertRaises(
@@ -1052,19 +1082,28 @@ class ThreadingTest(test_base.DbTestCase):
 
         @enginefacade.writer.connection
         def go_one(context):
-            self.assertIsNone(context.session)
+            self.assertRaises(
+                exception.ContextNotRequestedError,
+                getattr, context, "session"
+            )
             self.assertIsNotNone(context.connection)
 
             self.ident = 2
             go_two(context)
 
             self.ident = 1
-            self.assertIsNone(context.session)
+            self.assertRaises(
+                exception.ContextNotRequestedError,
+                getattr, context, "session"
+            )
             self.assertIsNotNone(context.connection)
 
         @enginefacade.reader
         def go_two(context):
-            self.assertIsNone(context.connection)
+            self.assertRaises(
+                exception.ContextNotRequestedError,
+                getattr, context, "connection"
+            )
             self.assertIsNotNone(context.session)
 
         context = oslo_context.RequestContext()
@@ -1652,5 +1691,19 @@ class ConfigOptionsTest(oslo_test_base.BaseTestCase):
             str(w[-1].message)
         )
 
+    def test_no_engine(self):
+        factory = enginefacade._TransactionFactory()
+
+        self.assertRaises(
+            exception.CantStartEngineError,
+            factory._create_session, enginefacade._WRITER
+        )
+
+        self.assertRaises(
+            exception.CantStartEngineError,
+            factory._create_session, enginefacade._WRITER
+        )
+
+
 # TODO(zzzeek): test configuration options, e.g. like
 # test_sqlalchemy->test_creation_from_config