Update connection docs for migrating off of nesting

Change-Id: I3a81140f00a4a9945121bfb8ec4c0e3953b4085f

doc/build/core/connections.rst

@@ -131,9 +131,11 @@ as a best practice.
 Nesting of Transaction Blocks
 -----------------------------
 
-.. note:: The "transaction nesting" feature of SQLAlchemy is a legacy feature
-   that will be deprecated in an upcoming release.  New usage paradigms will
-   eliminate the need for it to be present.
+.. deprecated:: 1.4 The "transaction nesting" feature of SQLAlchemy is a legacy feature
+   that is deprecated in the 1.4 release and will be removed in SQLAlchemy 2.0.
+   The pattern has proven to be a little too awkward and complicated, unless an
+   application makes more of a first-class framework around the behavior.  See
+   the following subsection :ref:`connections_avoid_nesting`.
 
 The :class:`.Transaction` object also handles "nested" behavior by keeping
 track of the outermost begin/commit pair. In this example, two functions both
@@ -168,8 +170,144 @@ which "guarantee" that a transaction will be used if one was not already
 available, but will automatically participate in an enclosing transaction if
 one exists.
 
-.. index::
-   single: thread safety; transactions
+.. _connections_avoid_nesting:
+
+Arbitrary Transaction Nesting as an Antipattern
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+With many years of experience, the above "nesting" pattern has not proven to
+be very popular, and where it has been observed in large projects such
+as Openstack, it tends to be complicated.
+
+The most ideal way to organize an application would have a single, or at
+least very few, points at which the "beginning" and "commit" of all
+database transactions is demarcated.   This is also the general
+idea discussed in terms of the ORM at :ref:`session_faq_whentocreate`.  To
+adapt the example from the previous section to this practice looks like::
+
+
+    # method_a calls method_b
+    def method_a(connection):
+        method_b(connection)
+
+    # method_b uses the connection and assumes the transaction
+    # is external
+    def method_b(connection):
+        connection.execute(text("insert into mytable values ('bat', 'lala')"))
+        connection.execute(mytable.insert(), {"col1": "bat", "col2": "lala"})
+
+    # open a Connection inside of a transaction and call method_a
+    with engine.begin() as conn:
+        method_a(conn)
+
+That is, ``method_a()`` and ``method_b()`` do not deal with the details
+of the transaction at all; the transactional scope of the connection is
+defined **externally** to the functions that have a SQL dialogue with the
+connection.
+
+It may be observed that the above code has fewer lines, and less indentation
+which tends to correlate with lower :term:`cyclomatic complexity`.   The
+above code is organized such that ``method_a()`` and ``method_b()`` are always
+invoked from a point at which a transaction is begun.  The previous
+version of the example features a ``method_a()`` and a ``method_b()`` that are
+trying to be agnostic of this fact, which suggests they are prepared for
+at least twice as many potential codepaths through them.
+
+.. _connections_subtransactions:
+
+Migrating from the "nesting" pattern
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+As SQLAlchemy's intrinsic-nested pattern is considered legacy, an application
+that for either legacy or novel reasons still seeks to have a context that
+automatically frames transactions should seek to maintain this functionality
+through the use of a custom Python context manager.  A similar example is also
+provided in terms of the ORM in the "seealso" section below.
+
+To provide backwards compatibility for applications that make use of this
+pattern, the following context manager or a similar implementation based on
+a decorator may be used::
+
+    import contextlib
+
+    @contextlib.contextmanager
+    def transaction(connection):
+        if not connection.in_transaction():
+            with connection.begin():
+                yield connection
+        else:
+            yield connection
+
+The above contextmanager would be used as::
+
+    # method_a starts a transaction and calls method_b
+    def method_a(connection):
+        with transaction(connection):  # open a transaction
+            method_b(connection)
+
+    # method_b either starts a transaction, or uses the one already
+    # present
+    def method_b(connection):
+        with transaction(connection):  # open a transaction
+            connection.execute(text("insert into mytable values ('bat', 'lala')"))
+            connection.execute(mytable.insert(), {"col1": "bat", "col2": "lala"})
+
+    # open a Connection and call method_a
+    with engine.connect() as conn:
+        method_a(conn)
+
+A similar approach may be taken such that connectivity is established
+on demand as well; the below approach features a single-use context manager
+that accesses an enclosing state in order to test if connectivity is already
+present::
+
+    import contextlib
+
+    def connectivity(engine):
+        connection = None
+
+        @contextlib.contextmanager
+        def connect():
+            nonlocal connection
+
+            if connection is None:
+                connection = engine.connect()
+                with connection:
+                    with connection.begin():
+                        yield connection
+            else:
+                yield connection
+
+        return connect
+
+Using the above would look like::
+
+    # method_a passes along connectivity context, at the same time
+    # it chooses to establish a connection by calling "with"
+    def method_a(connectivity):
+        with connectivity():
+            method_b(connectivity)
+
+    # method_b also wants to use a connection from the context, so it
+    # also calls "with:", but also it actually uses the connection.
+    def method_b(connectivity):
+        with connectivity() as connection:
+            connection.execute(text("insert into mytable values ('bat', 'lala')"))
+            connection.execute(mytable.insert(), {"col1": "bat", "col2": "lala"})
+
+    # create a new connection/transaction context object and call
+    # method_a
+    method_a(connectivity(engine))
+
+The above context manager acts not only as a "transaction" context but also
+as a context that manages having an open connection against a particular
+:class:`_engine.Engine`.   When using the ORM :class:`_orm.Session`, this
+connectivty management is provided by the :class:`_orm.Session` itself.
+An overview of ORM connectivity patterns is at :ref:`unitofwork_transaction`.
+
+.. seealso::
+
+  :ref:`session_subtransactions` - ORM version
 
 .. _autocommit:
 

doc/build/glossary.rst

@@ -81,6 +81,14 @@ Glossary
 
             `Relational Algebra (via Wikipedia) <https://en.wikipedia.org/wiki/Relational_algebra>`_
 
+    cyclomatic complexity
+        A measure of code complexity based on the number of possible paths
+        through a program's source code.
+
+        .. seealso::
+
+            `Cyclomatic Complexity <https://en.wikipedia.org/wiki/Cyclomatic_complexity>`_
+
     selectable
         A term used in SQLAlchemy to describe a SQL construct that represents
         a collection of rows.   It's largely similar to the concept of a

doc/build/orm/session_transaction.rst

@@ -325,13 +325,16 @@ Explicit Begin
 
 .. versionchanged:: 1.4
     SQLAlchemy 1.4 deprecates "autocommit mode", which is historically enabled
-    by using the :paramref:`_orm.Session.autocommit` flag.    This flag allows
-    the :class:`_orm.Session` to invoke SQL statements within individual,
-    ad-hoc transactions and has been recommended against for many years.
-    Instead, the :meth:`_orm.Session.begin` method may now be called when
+    by using the :paramref:`_orm.Session.autocommit` flag.    Going forward,
+    a new approach to allowing usage of the :meth:`_orm.Session.begin` method
+    is new "autobegin" behavior so that the method may now be called when
     a :class:`_orm.Session` is first constructed, or after the previous
     transaction has ended and before it begins a new one.
 
+    For background on migrating away from the "subtransaction" pattern for
+    frameworks that rely upon nesting of begin()/commit() pairs, see the
+    next section :ref:`session_subtransactions`.
+
 The :class:`_orm.Session` features "autobegin" behavior, meaning that as soon
 as operations begin to take place, it ensures a :class:`_orm.SessionTransaction`
 is present to track ongoing operations.   This transaction is completed
@@ -405,22 +408,11 @@ a decorator may be used::
 
     @contextlib.contextmanager
     def transaction(session):
-
-        if session.in_transaction():
-            outermost = False
+        if not session.in_transaction():
+            with session.begin():
+                yield
         else:
-            outermost = True
-            session.begin()
-
-        try:
             yield
-        except:
-            if session.in_transaction():
-                session.rollback()
-            raise
-        else:
-            if outermost and session.in_transaction():
-                session.commit()
 
 
 The above context manager may be used in the same way the
@@ -439,6 +431,8 @@ The above context manager may be used in the same way the
         with transaction(session):
             session.add(SomeObject('bat', 'lala'))
 
+    Session = sessionmaker(engine)
+
     # create a Session and call method_a
     with Session() as session:
         method_a(session)
@@ -453,11 +447,16 @@ or methods to be concerned with the details of transaction demarcation::
     def method_b(session):
         session.add(SomeObject('bat', 'lala'))
 
+    Session = sessionmaker(engine)
+
     # create a Session and call method_a
     with Session() as session:
         with session.begin():
             method_a(session)
 
+.. seealso::
+
+    :ref:`connections_subtransactions` - similar pattern based on Core only
 
 .. _session_twophase:
 

lib/sqlalchemy/orm/session.py

@@ -1085,11 +1085,11 @@ class Session(_SessionClassMethods):
 
     @property
     @util.deprecated_20(
-        "The :attr:`_orm.Session.transaction` accessor is deprecated and "
-        "will be removed in SQLAlchemy version 2.0.  "
-        "For context manager use, use :meth:`_orm.Session.begin`.  To access "
+        ":attr:`_orm.Session.transaction`",
+        alternative="For context manager use, use "
+        ":meth:`_orm.Session.begin`.  To access "
         "the current root transaction, use "
-        ":meth:`_orm.Session.get_transaction`"
+        ":meth:`_orm.Session.get_transaction`.",
     )
     def transaction(self):
         """The current active or inactive :class:`.SessionTransaction`.

test/orm/test_transaction.py

@@ -1089,34 +1089,80 @@ class _LocalFixture(FixtureTest):
         mapper(Address, addresses)
 
 
-class SubtransactionRecipeTest(FixtureTest):
-    run_inserts = None
-    __backend__ = True
+def subtransaction_recipe_one(self):
+    @contextlib.contextmanager
+    def transaction(session):
 
-    future = False
-
-    @testing.fixture
-    def subtransaction_recipe(self):
-        @contextlib.contextmanager
-        def transaction(session):
+        if session.in_transaction():
+            outermost = False
+        else:
+            outermost = True
+            session.begin()
 
+        try:
+            yield
+        except:
             if session.in_transaction():
-                outermost = False
-            else:
-                outermost = True
-                session.begin()
+                session.rollback()
+            raise
+        else:
+            if outermost and session.in_transaction():
+                session.commit()
 
+    return transaction
+
+
+def subtransaction_recipe_two(self):
+    # shorter recipe
+    @contextlib.contextmanager
+    def transaction(session):
+        if not session.in_transaction():
+            with session.begin():
+                yield
+        else:
+            yield
+
+    return transaction
+
+
+def subtransaction_recipe_three(self):
+    @contextlib.contextmanager
+    def transaction(session):
+        if not session.in_transaction():
+            session.begin()
+            try:
+                yield
+            except:
+                if session.in_transaction():
+                    session.rollback()
+            else:
+                session.commit()
+        else:
             try:
                 yield
             except:
                 if session.in_transaction():
                     session.rollback()
                 raise
-            else:
-                if outermost and session.in_transaction():
-                    session.commit()
 
-        return transaction
+    return transaction
+
+
+@testing.combinations(
+    (subtransaction_recipe_one, True),
+    (subtransaction_recipe_two, False),
+    (subtransaction_recipe_three, True),
+    argnames="target_recipe,recipe_rollsback_early",
+    id_="ns",
+)
+@testing.combinations((True,), (False,), argnames="future", id_="s")
+class SubtransactionRecipeTest(FixtureTest):
+    run_inserts = None
+    __backend__ = True
+
+    @testing.fixture
+    def subtransaction_recipe(self):
+        return self.target_recipe()
 
     @testing.requires.savepoints
     def test_recipe_heavy_nesting(self, subtransaction_recipe):
@@ -1253,10 +1299,15 @@ class SubtransactionRecipeTest(FixtureTest):
         except:
             pass
 
-        # that was a real rollback, so no transaction
-        is_(sess.get_transaction(), None)
+        if self.recipe_rollsback_early:
+            # that was a real rollback, so no transaction
+            assert not sess.in_transaction()
+            is_(sess.get_transaction(), None)
+        else:
+            assert sess.in_transaction()
 
         sess.close()
+        assert not sess.in_transaction()
 
     def test_recipe_multi_nesting(self, subtransaction_recipe):
         sess = Session(testing.db, future=self.future)
@@ -1271,7 +1322,12 @@ class SubtransactionRecipeTest(FixtureTest):
             except:
                 pass
 
-            assert not sess.in_transaction()
+            if self.recipe_rollsback_early:
+                assert not sess.in_transaction()
+            else:
+                assert sess.in_transaction()
+
+        assert not sess.in_transaction()
 
     def test_recipe_deactive_status_check(self, subtransaction_recipe):
         sess = Session(testing.db, future=self.future)
@@ -1284,10 +1340,6 @@ class SubtransactionRecipeTest(FixtureTest):
         sess.commit()  # no error
 
 
-class FutureSubtransactionRecipeTest(SubtransactionRecipeTest):
-    future = True
-
-
 class FixtureDataTest(_LocalFixture):
     run_inserts = "each"
     __backend__ = True