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

4 files changed, 96 insertions, 7 deletions

diff --git a/doc/source/usage.rst b/doc/source/usage.rst
index 11c8c2a..cc4f2fd 100644
--- a/doc/source/usage.rst
+++ b/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
diff --git a/oslo_db/exception.py b/oslo_db/exception.py
index c1620e1..2b2df30 100644
--- a/oslo_db/exception.py
+++ b/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.
 
diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py
index 4497a50..cf7f149 100644
--- a/oslo_db/sqlalchemy/enginefacade.py
+++ b/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)
 
 
diff --git a/oslo_db/tests/sqlalchemy/test_enginefacade.py b/oslo_db/tests/sqlalchemy/test_enginefacade.py
index fe8030c..6893a93 100644
--- a/oslo_db/tests/sqlalchemy/test_enginefacade.py
+++ b/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