Merge "Remove the 'Session.autocommit' parameter"

5 files changed, 83 insertions, 77 deletions

diff --git a/oslo_db/sqlalchemy/enginefacade.py b/oslo_db/sqlalchemy/enginefacade.py
index f69811b..a2ea27a 100644
--- a/oslo_db/sqlalchemy/enginefacade.py
+++ b/oslo_db/sqlalchemy/enginefacade.py
@@ -163,7 +163,7 @@ class _TransactionFactory(object):
         }
         self._maker_cfg = {
             'expire_on_commit': _Default(False),
-            '__autocommit': True
+            '__autocommit': False,
         }
         self._transaction_ctx_cfg = {
             'rollback_reader_sessions': False,
@@ -1266,13 +1266,22 @@ class LegacyEngineFacade(object):
 
     """
     def __init__(self, sql_connection, slave_connection=None,
-                 sqlite_fk=False, autocommit=True,
+                 sqlite_fk=False, autocommit=False,
                  expire_on_commit=False, _conf=None, _factory=None, **kwargs):
         warnings.warn(
             "EngineFacade is deprecated; please use "
             "oslo_db.sqlalchemy.enginefacade",
             warning.OsloDBDeprecationWarning,
             stacklevel=2)
+
+        if autocommit is True:
+            warnings.warn(
+                'autocommit support will be removed in SQLAlchemy 2.0 and '
+                'should not be relied on; please rework your code to remove '
+                'reliance on this feature',
+                warning.OsloDBDeprecationWarning,
+                stacklevel=2)
+
         if _factory:
             self._factory = _factory
         else:
@@ -1346,7 +1355,7 @@ class LegacyEngineFacade(object):
 
     @classmethod
     def from_config(cls, conf,
-                    sqlite_fk=False, autocommit=True, expire_on_commit=False):
+                    sqlite_fk=False, autocommit=False, expire_on_commit=False):
         """Initialize EngineFacade using oslo.config config instance options.
 
         :param conf: oslo.config config instance
diff --git a/oslo_db/sqlalchemy/orm.py b/oslo_db/sqlalchemy/orm.py
index b1ca00a..a5ec4c4 100644
--- a/oslo_db/sqlalchemy/orm.py
+++ b/oslo_db/sqlalchemy/orm.py
@@ -57,7 +57,7 @@ class Session(sqlalchemy.orm.session.Session):
     """oslo.db-specific Session subclass."""
 
 
-def get_maker(engine, autocommit=True, expire_on_commit=False):
+def get_maker(engine, autocommit=False, expire_on_commit=False):
     """Return a SQLAlchemy sessionmaker using the given engine."""
     return sqlalchemy.orm.sessionmaker(bind=engine,
                                        class_=Session,
diff --git a/oslo_db/tests/fixtures.py b/oslo_db/tests/fixtures.py
index 468dcae..93cdcb6 100644
--- a/oslo_db/tests/fixtures.py
+++ b/oslo_db/tests/fixtures.py
@@ -37,15 +37,7 @@ class WarningsFixture(fixtures.Fixture):
             'error',
             category=sqla_exc.SADeprecationWarning)
 
-        # ...but filter everything out until we get around to fixing them
-        # FIXME(stephenfin): Remove all of these
-
-        warnings.filterwarnings(
-            'once',
-            message=r'The Session.autocommit parameter is deprecated .*',
-            category=sqla_exc.SADeprecationWarning)
-
-        # ...plus things that aren't our fault
+        # ...but filter things that aren't our fault
 
         # FIXME(stephenfin): These are caused by sqlalchemy-migrate, not us,
         # and should be removed when we drop support for that library
diff --git a/oslo_db/tests/sqlalchemy/test_enginefacade.py b/oslo_db/tests/sqlalchemy/test_enginefacade.py
index a188d01..bdf5104 100644
--- a/oslo_db/tests/sqlalchemy/test_enginefacade.py
+++ b/oslo_db/tests/sqlalchemy/test_enginefacade.py
@@ -357,11 +357,11 @@ class MockFacadeTest(test_base.BaseTestCase):
         maker_factories = mock.Mock(side_effect=get_maker)
 
         maker_factories(
-            autocommit=True, engine=engines.writer,
+            autocommit=False, engine=engines.writer,
             expire_on_commit=False)
         if self.slave_uri:
             maker_factories(
-                autocommit=True, engine=engines.async_reader,
+                autocommit=False, engine=engines.async_reader,
                 expire_on_commit=False)
 
         yield makers
@@ -1692,11 +1692,12 @@ class LiveFacadeTest(db_test_base._DbTestCase):
         with enginefacade.writer.using(context) as session:
             session.add(self.User(name="u1"))
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            "u1",
-            session.query(self.User.name).scalar()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                "u1",
+                session.query(self.User.name).scalar()
+            )
 
     def test_transaction_rollback(self):
         context = oslo_context.RequestContext()
@@ -1712,11 +1713,12 @@ class LiveFacadeTest(db_test_base._DbTestCase):
 
         self.assertRaises(MyException, go, context)
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            None,
-            session.query(self.User.name).scalar()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                None,
+                session.query(self.User.name).scalar()
+            )
 
     @mock.patch.object(Session, 'commit')
     @mock.patch.object(Session, 'rollback')
@@ -1783,11 +1785,12 @@ class LiveFacadeTest(db_test_base._DbTestCase):
                 s2.add(self.User(name="u1"))
                 s2.flush()
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            "u1",
-            session.query(self.User.name).scalar()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                "u1",
+                session.query(self.User.name).scalar()
+            )
 
     def test_context_deepcopy_on_connection(self):
         context = oslo_context.RequestContext()
@@ -1804,11 +1807,12 @@ class LiveFacadeTest(db_test_base._DbTestCase):
 
             self._assert_ctx_connection(ctx2, conn2)
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            "u1",
-            session.query(self.User.name).scalar()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                "u1",
+                session.query(self.User.name).scalar()
+            )
 
     @db_test_base.backend_specific("postgresql", "mysql")
     def test_external_session_transaction(self):
@@ -1840,14 +1844,14 @@ class LiveFacadeTest(db_test_base._DbTestCase):
             session.begin()
             session.add(self.User(name="u4"))
 
-        session = self.sessionmaker(autocommit=True)
-
+        session = self.sessionmaker(autocommit=False)
         # inner transaction + second part of "outer" transaction were committed
-        self.assertEqual(
-            [("u2",), ("u3",), ("u4", )],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        with session.begin():
+            self.assertEqual(
+                [("u2",), ("u3",), ("u4", )],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     def test_savepoint_transaction_decorator(self):
         context = oslo_context.RequestContext()
@@ -1880,14 +1884,14 @@ class LiveFacadeTest(db_test_base._DbTestCase):
 
         go1(context)
 
-        session = self.sessionmaker(autocommit=True)
-
+        session = self.sessionmaker(autocommit=False)
         # inner transaction + second part of "outer" transaction were committed
-        self.assertEqual(
-            [("u1",), ("u3",), ("u4", )],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        with session.begin():
+            self.assertEqual(
+                [("u1",), ("u3",), ("u4", )],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     def test_savepoint_transaction(self):
         context = oslo_context.RequestContext()
@@ -1908,14 +1912,14 @@ class LiveFacadeTest(db_test_base._DbTestCase):
 
             session.add(self.User(name="u4"))
 
-        session = self.sessionmaker(autocommit=True)
-
+        session = self.sessionmaker(autocommit=False)
         # inner transaction + second part of "outer" transaction were committed
-        self.assertEqual(
-            [("u1",), ("u3",), ("u4", )],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        with session.begin():
+            self.assertEqual(
+                [("u1",), ("u3",), ("u4", )],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     @db_test_base.backend_specific("postgresql", "mysql")
     def test_external_session_transaction_decorator(self):
@@ -1956,14 +1960,14 @@ class LiveFacadeTest(db_test_base._DbTestCase):
 
         go1(context)
 
-        session = self.sessionmaker(autocommit=True)
-
+        session = self.sessionmaker(autocommit=False)
         # inner transaction + second part of "outer" transaction were committed
-        self.assertEqual(
-            [("u2",), ("u3",), ("u4", )],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        with session.begin():
+            self.assertEqual(
+                [("u2",), ("u3",), ("u4", )],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     @db_test_base.backend_specific("postgresql", "mysql")
     def test_external_connection_transaction(self):
@@ -1995,12 +1999,13 @@ class LiveFacadeTest(db_test_base._DbTestCase):
             # add more state on the "outer" transaction
             connection.execute(self.user_table.insert().values(name="u4"))
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            [("u2",), ("u3",), ("u4", )],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                [("u2",), ("u3",), ("u4", )],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     @db_test_base.backend_specific("postgresql", "mysql")
     def test_external_writer_in_reader(self):
@@ -2030,12 +2035,13 @@ class LiveFacadeTest(db_test_base._DbTestCase):
             user = session.query(self.User).first()
             self.assertEqual("u1_commit", user.name)
 
-        session = self.sessionmaker(autocommit=True)
-        self.assertEqual(
-            [("u1_commit",)],
-            session.query(
-                self.User.name).order_by(self.User.name).all()
-        )
+        session = self.sessionmaker(autocommit=False)
+        with session.begin():
+            self.assertEqual(
+                [("u1_commit",)],
+                session.query(
+                    self.User.name).order_by(self.User.name).all()
+            )
 
     def test_replace_scope(self):
         # "timeout" is an argument accepted by
diff --git a/oslo_db/tests/sqlalchemy/test_types.py b/oslo_db/tests/sqlalchemy/test_types.py
index 4b43665..cf0c42a 100644
--- a/oslo_db/tests/sqlalchemy/test_types.py
+++ b/oslo_db/tests/sqlalchemy/test_types.py
@@ -81,8 +81,7 @@ class JsonTypesTestCase(test_base._DbTestCase):
             {'a': 'b'}
         ]
         for i, test in enumerate(tested):
-            with self.session.begin():
-                JsonTable(id=i, json=test).save(self.session)
+            JsonTable(id=i, json=test).save(self.session)
             obj = self.session.query(JsonTable).filter_by(id=i).one()
             self.assertEqual(test, obj.json)