From 43142792502afe5e5ce29a4cc3c291f673ef066e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Aug 2021 15:37:34 +0100 Subject: [PATCH 1/6] Add missing 'connect' wrapper This was missed in change Ib789cd4d11a3d5dd01fcdb99822025b11bbc234e ("Don't rely on implicit autocommit") Change-Id: I9ec27650ae5e36099a6d2b2d59bb66cd820e8ffc Signed-off-by: Stephen Finucane --- oslo_db/sqlalchemy/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py index 3a6a993..e5b3531 100644 --- a/oslo_db/sqlalchemy/utils.py +++ b/oslo_db/sqlalchemy/utils.py @@ -1162,7 +1162,9 @@ def get_non_ndbcluster_tables(connectable, skip_tables=None): params['database'] = connectable.engine.url.database query = text(query_str) - nonndbcluster = connectable.execute(query, **params) + # TODO(stephenfin): What about if this is already a Connection? + with connectable.connect() as conn, conn.begin(): + nonndbcluster = connectable.execute(query, **params) return [i[0] for i in nonndbcluster] -- GitLab From a530cbfcf2fadbc1bbd7a85086278100f15abf2e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Aug 2021 13:10:59 +0100 Subject: [PATCH 2/6] Remove the 'Session.autocommit' parameter Resolve the following RemovedIn20Warning warning: The Session.autocommit parameter is deprecated and will be removed in SQLAlchemy version 2.0. The Session now features "autobegin" behavior such that the Session.begin() method may be called if a transaction has not yet been started yet. See the section session_explicit_begin for background. Change-Id: I7867cdcea115b13f2e45e0674bb9ef2ad138aae9 Signed-off-by: Stephen Finucane --- oslo_db/sqlalchemy/enginefacade.py | 15 +- oslo_db/sqlalchemy/orm.py | 2 +- oslo_db/tests/fixtures.py | 10 +- oslo_db/tests/sqlalchemy/test_enginefacade.py | 130 +++++++++--------- oslo_db/tests/sqlalchemy/test_types.py | 3 +- 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) -- GitLab From 2c7c0cab06adcfd5e510985dc1af302a47ae9899 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 16 Jul 2021 13:04:01 +0100 Subject: [PATCH 3/6] tests: Enable SAWarning warnings We shouldn't be raising warnings from SQLAlchemy. Where we are intentionally doing so, we should capture these warnings at the test level. This requires some minor fixes. Change-Id: I9d4512dc337153edc48a2cc3bf95ab2b31c39ccf Signed-off-by: Stephen Finucane --- oslo_db/sqlalchemy/utils.py | 18 +++++++++++++++--- oslo_db/tests/fixtures.py | 5 +++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/oslo_db/sqlalchemy/utils.py b/oslo_db/sqlalchemy/utils.py index e5b3531..f1eec42 100644 --- a/oslo_db/sqlalchemy/utils.py +++ b/oslo_db/sqlalchemy/utils.py @@ -39,6 +39,7 @@ from sqlalchemy import Index from sqlalchemy import inspect from sqlalchemy import Integer from sqlalchemy import MetaData +from sqlalchemy import PrimaryKeyConstraint from sqlalchemy.sql.expression import cast from sqlalchemy.sql.expression import literal_column from sqlalchemy.sql import text @@ -608,7 +609,14 @@ def _change_deleted_column_type_to_boolean_sqlite(engine, table_name, # FIXME(stephenfin): We shouldn't be using this private API; # figure out how else to copy an arbitrary column schema - constraints = [constraint._copy() for constraint in table.constraints] + # NOTE(stephenfin): We drop PrimaryKeyConstraint-type constraints since + # these duplicate the 'primary_key=True' attribute on the speicified + # column(s). This technically breaks things when the primary key covers + # multiple columns but that's okay: these are deprecated APIs + constraints = [ + constraint._copy() for constraint in table.constraints + if not isinstance(constraint, PrimaryKeyConstraint) + ] with engine.connect() as conn: meta = table.metadata @@ -738,7 +746,10 @@ def _change_deleted_column_type_to_id_type_sqlite(engine, table_name, constraints = [] for constraint in table.constraints: - if not _is_deleted_column_constraint(constraint): + if not ( + _is_deleted_column_constraint(constraint) or + isinstance(constraint, PrimaryKeyConstraint) + ): # FIXME(stephenfin): We shouldn't be using this private API; # figure out how else to copy an arbitrary constraint schema constraints.append(constraint._copy()) @@ -749,7 +760,8 @@ def _change_deleted_column_type_to_id_type_sqlite(engine, table_name, with conn.begin(): new_table = Table( table_name + "__tmp__", meta, - *(columns + constraints)) + *(columns + constraints), + ) new_table.create(conn) indexes = [] diff --git a/oslo_db/tests/fixtures.py b/oslo_db/tests/fixtures.py index 93cdcb6..4cc596f 100644 --- a/oslo_db/tests/fixtures.py +++ b/oslo_db/tests/fixtures.py @@ -24,11 +24,12 @@ class WarningsFixture(fixtures.Fixture): self._original_warning_filters = warnings.filters[:] - # Make deprecation warnings only happen once to avoid spamming warnings.simplefilter('once', DeprecationWarning) + # Enable generic warnings to ensure we're not doing anything odd + warnings.filterwarnings( - 'error', message='Evaluating non-mapped column expression', + 'error', category=sqla_exc.SAWarning) # Enable deprecation warnings to capture upcoming SQLAlchemy changes -- GitLab From 4c451b7df55f3f77af7339ed38b49bdb5747e52e Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Aug 2021 16:19:23 +0100 Subject: [PATCH 4/6] trivial: Don't emit warnings for our own deprecations We've deprecated a number of modules recently. We don't need to emit these warnings when running unit tests. Silence things. Change-Id: I7aed7789584bf0070f11c22b5eaa0e80c42dfc9c Signed-off-by: Stephen Finucane --- oslo_db/tests/fixtures.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/oslo_db/tests/fixtures.py b/oslo_db/tests/fixtures.py index 4cc596f..62a88fb 100644 --- a/oslo_db/tests/fixtures.py +++ b/oslo_db/tests/fixtures.py @@ -26,6 +26,13 @@ class WarningsFixture(fixtures.Fixture): warnings.simplefilter('once', DeprecationWarning) + # Except things we've deprecated but are still testing until removal + + warnings.filterwarnings( + 'ignore', + category=DeprecationWarning, + module='oslo_db') + # Enable generic warnings to ensure we're not doing anything odd warnings.filterwarnings( -- GitLab From e7642fd2c8b8da1ecf18b914a4b773879861c6e8 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 16 Aug 2021 16:04:24 +0100 Subject: [PATCH 5/6] tox: Silence output Enable the 'CaptureOutput' fixture provided by oslotest by default. Change-Id: Ib34d8ab411a67816db2e26b49bec75993b5bed56 Signed-off-by: Stephen Finucane --- tox.ini | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tox.ini b/tox.ini index 1cc1071..cfe61bb 100644 --- a/tox.ini +++ b/tox.ini @@ -10,6 +10,8 @@ allowlist_externals = passenv = OS_TEST_DBAPI_ADMIN_CONNECTION setenv = + OS_STDOUT_CAPTURE=true + OS_STDERR_CAPTURE=true BASECOMMAND=stestr run {postgresql,all}: PIFPAF_POSTGRESQL=pifpaf -g OS_TEST_DBAPI_ADMIN_CONNECTION run postgresql -- {mysql,all}: PIFPAF_MYSQL=pifpaf -g OS_TEST_DBAPI_ADMIN_CONNECTION run mysql -- -- GitLab From 7f3647bf9743ddfb47abc5981fc97653d5ecf09c Mon Sep 17 00:00:00 2001 From: ljhuang Date: Wed, 3 Aug 2022 20:33:08 +0800 Subject: [PATCH 6/6] Replace abc.abstractproperty with property and abc.abstractmethod Replace abc.abstractproperty with property and abc.abstractmethod, as abc.abstractproperty has been deprecated since python3.3[1] [1]https://docs.python.org/3.8/whatsnew/3.3.html?highlight=deprecated#abc Change-Id: Id90fbd2c53fd49341043bde740500a857a4339d3 --- oslo_db/sqlalchemy/test_migrations.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/oslo_db/sqlalchemy/test_migrations.py b/oslo_db/sqlalchemy/test_migrations.py index 74181db..a0b5591 100644 --- a/oslo_db/sqlalchemy/test_migrations.py +++ b/oslo_db/sqlalchemy/test_migrations.py @@ -77,7 +77,8 @@ class WalkVersionsMixin(object, metaclass=abc.ABCMeta): """ - @abc.abstractproperty + @property + @abc.abstractmethod def INIT_VERSION(self): """Initial version of a migration repository. @@ -87,7 +88,8 @@ class WalkVersionsMixin(object, metaclass=abc.ABCMeta): """ pass - @abc.abstractproperty + @property + @abc.abstractmethod def REPOSITORY(self): """Allows basic manipulation with migration repository. @@ -95,7 +97,8 @@ class WalkVersionsMixin(object, metaclass=abc.ABCMeta): """ pass - @abc.abstractproperty + @property + @abc.abstractmethod def migration_api(self): """Provides API for upgrading, downgrading and version manipulations. @@ -103,7 +106,8 @@ class WalkVersionsMixin(object, metaclass=abc.ABCMeta): """ pass - @abc.abstractproperty + @property + @abc.abstractmethod def migrate_engine(self): """Provides engine instance. -- GitLab