Get test suite to full pass with SQLAlchemy 2.0

Remaining issues encountered when running with SQLAlchemy 2.0 for real:

* Never call str() on a URL and expect it to be meaningful anymore.
  The password is aggressively obfuscated now (users absolultely
  wouldn't let us leave it as is)
* More utilities and fixtures that were calling begin() within a
  block that would have already begun
* isnot is now called is_not; mocking "isnot" leads into too many
  weird compat layers
* ORM InstrumentedAttribute and internals use __slots__ now, mock
  seems to not be able to patch methods.  Ideally these tests would use
  a comparator subclass or something
* Connection.connection.connection is now called driver_connection,
  SQLAlchemy keeps the old name available however oslo.db test suite
  does not appear to tolerate the deprecation warning emitted,
  so add a compat layer
* mapper() is fully removed from 2.0, not sure if there is another
  not-yet-committed gerrit that removes mapper()

[1] https://docs.sqlalchemy.org/en/20/core/engines.html#sqlalchemy.create_engine.params.pool_pre_ping
[2] https://docs.sqlalchemy.org/en/20/changelog/changelog_20.html#change-2fe37eaf2295cebd3bb4ee8e5b8c575c
[3] https://github.com/sqlalchemy/sqlalchemy/issues/5648

Change-Id: Ifaca67c07f008d8bc0febeecd3e200cc7ee7a4b0

5 files changed, 123 insertions, 78 deletions

diff --git a/oslo_db/tests/sqlalchemy/test_enginefacade.py b/oslo_db/tests/sqlalchemy/test_enginefacade.py
index 4175880..c2b980f 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=False, engine=engines.writer,
+            engine=engines.writer,
             expire_on_commit=False)
         if self.slave_uri:
             maker_factories(
-                autocommit=False, engine=engines.async_reader,
+                engine=engines.async_reader,
                 expire_on_commit=False)
 
         yield makers
diff --git a/oslo_db/tests/sqlalchemy/test_exc_filters.py b/oslo_db/tests/sqlalchemy/test_exc_filters.py
index 7f64594..af3cd91 100644
--- a/oslo_db/tests/sqlalchemy/test_exc_filters.py
+++ b/oslo_db/tests/sqlalchemy/test_exc_filters.py
@@ -26,6 +26,7 @@ from sqlalchemy.orm import registry
 from sqlalchemy import sql
 
 from oslo_db import exception
+from oslo_db.sqlalchemy import compat
 from oslo_db.sqlalchemy import engines
 from oslo_db.sqlalchemy import exc_filters
 from oslo_db.sqlalchemy import utils
@@ -139,18 +140,29 @@ class TestsExceptionFilter(_SQLAExceptionMatcher, test_base.BaseTestCase):
         # statement
         self.engine.connect().close()
 
-        with test_utils.nested(
+        patches = [
             mock.patch.object(engine.dialect, "do_execute", do_execute),
             # replace the whole DBAPI rather than patching "Error"
             # as some DBAPIs might not be patchable (?)
             mock.patch.object(engine.dialect,
                               "dbapi",
                               mock.Mock(Error=self.Error)),
+
             mock.patch.object(engine.dialect, "name", dialect_name),
             mock.patch.object(engine.dialect,
                               "is_disconnect",
                               lambda *args: is_disconnect)
-        ):
+        ]
+        if compat.sqla_2:
+            patches.append(
+                mock.patch.object(
+                    engine.dialect,
+                    "loaded_dbapi",
+                    mock.Mock(Error=self.Error),
+                )
+            )
+
+        with test_utils.nested(*patches):
             yield
 
     def _run_test(self, dialect_name, statement, raises, expected,
@@ -754,7 +766,7 @@ class TestExceptionCauseMySQLSavepoint(
             session.execute(sql.text("select 1"))
 
             # close underying DB connection
-            session.connection().connection.connection.close()
+            compat.driver_connection(session.connection()).close()
 
             # alternate approach, but same idea:
             # conn_id = session.scalar("select connection_id()")
@@ -779,7 +791,7 @@ class TestExceptionCauseMySQLSavepoint(
             session.execute(sql.text("select 1"))
 
             # close underying DB connection
-            session.connection().connection.connection.close()
+            compat.driver_connection(session.connection()).close()
 
             # alternate approach, but same idea:
             # conn_id = session.scalar("select connection_id()")
@@ -947,8 +959,8 @@ class TestDuplicate(TestsExceptionFilter):
 class TestDeadlock(TestsExceptionFilter):
     statement = ('SELECT quota_usages.created_at AS '
                  'quota_usages_created_at FROM quota_usages '
-                 'WHERE quota_usages.project_id = %(project_id_1)s '
-                 'AND quota_usages.deleted = %(deleted_1)s FOR UPDATE')
+                 'WHERE quota_usages.project_id = :project_id_1 '
+                 'AND quota_usages.deleted = :deleted_1 FOR UPDATE')
     params = {
         'project_id_1': '8891d4478bbf48ad992f050cdf55e9b5',
         'deleted_1': 0
diff --git a/oslo_db/tests/sqlalchemy/test_provision.py b/oslo_db/tests/sqlalchemy/test_provision.py
index a6cedce..e0ab39f 100644
--- a/oslo_db/tests/sqlalchemy/test_provision.py
+++ b/oslo_db/tests/sqlalchemy/test_provision.py
@@ -13,6 +13,7 @@
 import os
 from unittest import mock
 
+from sqlalchemy.engine import url as sqla_url
 from sqlalchemy import exc as sa_exc
 from sqlalchemy import inspect
 from sqlalchemy import schema
@@ -156,8 +157,8 @@ class AdHocURLTest(test_base.BaseTestCase):
         fixture.setUp()
 
         self.assertEqual(
-            str(enginefacade._context_manager._factory._writer_engine.url),
-            "sqlite:///foo.db"
+            enginefacade._context_manager._factory._writer_engine.url,
+            sqla_url.make_url("sqlite:///foo.db")
             )
 
         self.assertTrue(os.path.exists("foo.db"))
@@ -176,14 +177,14 @@ class AdHocURLTest(test_base.BaseTestCase):
         self.addCleanup(
             mysql_backend.drop_named_database, "adhoc_test"
         )
-        url = str(mysql_backend.provisioned_database_url("adhoc_test"))
+        url = mysql_backend.provisioned_database_url("adhoc_test")
 
         fixture = test_fixtures.AdHocDbFixture(url)
 
         fixture.setUp()
 
         self.assertEqual(
-            str(enginefacade._context_manager._factory._writer_engine.url),
+            enginefacade._context_manager._factory._writer_engine.url,
             url
         )
 
diff --git a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
index a51d2cb..5f7b27f 100644
--- a/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
+++ b/oslo_db/tests/sqlalchemy/test_sqlalchemy.py
@@ -23,10 +23,10 @@ from unittest import mock
 
 import fixtures
 from oslo_config import cfg
-from oslo_utils import versionutils
 import sqlalchemy
 from sqlalchemy.engine import base as base_engine
 from sqlalchemy import exc
+from sqlalchemy.pool import NullPool
 from sqlalchemy import sql
 from sqlalchemy import Column, MetaData, Table
 from sqlalchemy import Integer, String
@@ -34,6 +34,7 @@ from sqlalchemy.orm import declarative_base
 
 from oslo_db import exception
 from oslo_db import options as db_options
+from oslo_db.sqlalchemy import compat
 from oslo_db.sqlalchemy import enginefacade
 from oslo_db.sqlalchemy import engines
 from oslo_db.sqlalchemy import models
@@ -388,9 +389,8 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
         self.assertIsNot(ses1, ses2)
 
     def test_get_session_arguments_override_default_settings(self):
-        ses = self.facade.get_session(autocommit=False, expire_on_commit=True)
+        ses = self.facade.get_session(expire_on_commit=True)
 
-        self.assertFalse(ses.autocommit)
         self.assertTrue(ses.expire_on_commit)
 
     @mock.patch('oslo_db.sqlalchemy.orm.get_maker')
@@ -410,7 +410,6 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
             conf.set_override(optname, optvalue, group='database')
 
         session.EngineFacade.from_config(conf,
-                                         autocommit=False,
                                          expire_on_commit=True)
 
         create_engine.assert_called_once_with(
@@ -435,7 +434,6 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
             logging_name=mock.ANY,
         )
         get_maker.assert_called_once_with(engine=create_engine(),
-                                          autocommit=False,
                                           expire_on_commit=True)
 
     def test_slave_connection(self):
@@ -696,22 +694,24 @@ class CreateEngineTest(test_base.BaseTestCase):
 
     def test_queuepool_args(self):
         engines._init_connection_args(
-            utils.make_url("mysql+pymysql://u:p@host/test"), self.args,
-            max_pool_size=10, max_overflow=10)
+            utils.make_url("mysql+pymysql://u:p@host/test"),
+            self.args,
+            {'max_pool_size': 10, 'max_overflow': 10},
+        )
         self.assertEqual(10, self.args['pool_size'])
         self.assertEqual(10, self.args['max_overflow'])
 
     def test_sqlite_memory_pool_args(self):
         for _url in ("sqlite://", "sqlite:///:memory:"):
             engines._init_connection_args(
-                utils.make_url(_url), self.args,
-                max_pool_size=10, max_overflow=10)
+                utils.make_url(_url),
+                self.args,
+                {'max_pool_size': 10, 'max_overflow': 10},
+            )
 
-            # queuepool arguments are not peresnet
-            self.assertNotIn(
-                'pool_size', self.args)
-            self.assertNotIn(
-                'max_overflow', self.args)
+            # queuepool arguments are not present
+            self.assertNotIn('pool_size', self.args)
+            self.assertNotIn('max_overflow', self.args)
 
             self.assertEqual(False,
                              self.args['connect_args']['check_same_thread'])
@@ -721,8 +721,10 @@ class CreateEngineTest(test_base.BaseTestCase):
 
     def test_sqlite_file_pool_args(self):
         engines._init_connection_args(
-            utils.make_url("sqlite:///somefile.db"), self.args,
-            max_pool_size=10, max_overflow=10)
+            utils.make_url("sqlite:///somefile.db"),
+            self.args,
+            {'max_pool_size': 10, 'max_overflow': 10},
+        )
 
         # queuepool arguments are not peresnet
         self.assertNotIn('pool_size', self.args)
@@ -731,9 +733,12 @@ class CreateEngineTest(test_base.BaseTestCase):
 
         self.assertFalse(self.args['connect_args'])
 
-        # NullPool is the default for file based connections,
-        # no need to specify this
-        self.assertNotIn('poolclass', self.args)
+        if not compat.sqla_2:
+            # NullPool is the default for file based connections,
+            # no need to specify this
+            self.assertNotIn('poolclass', self.args)
+        else:
+            self.assertIs(self.args["poolclass"], NullPool)
 
     def _test_mysql_connect_args_default(self, connect_args):
         self.assertEqual({'charset': 'utf8', 'use_unicode': 1},
@@ -741,34 +746,29 @@ class CreateEngineTest(test_base.BaseTestCase):
 
     def test_mysql_connect_args_default(self):
         engines._init_connection_args(
-            utils.make_url("mysql://u:p@host/test"), self.args)
-        self._test_mysql_connect_args_default(self.args['connect_args'])
-
-    def test_mysql_oursql_connect_args_default(self):
-        engines._init_connection_args(
-            utils.make_url("mysql+oursql://u:p@host/test"), self.args)
+            utils.make_url("mysql://u:p@host/test"), self.args, {})
         self._test_mysql_connect_args_default(self.args['connect_args'])
 
     def test_mysql_pymysql_connect_args_default(self):
         engines._init_connection_args(
-            utils.make_url("mysql+pymysql://u:p@host/test"), self.args)
+            utils.make_url("mysql+pymysql://u:p@host/test"), self.args, {})
         self.assertEqual({'charset': 'utf8'}, self.args['connect_args'])
 
     def test_mysql_mysqldb_connect_args_default(self):
         engines._init_connection_args(
-            utils.make_url("mysql+mysqldb://u:p@host/test"), self.args)
+            utils.make_url("mysql+mysqldb://u:p@host/test"), self.args, {})
         self._test_mysql_connect_args_default(self.args['connect_args'])
 
     def test_postgresql_connect_args_default(self):
         engines._init_connection_args(
-            utils.make_url("postgresql://u:p@host/test"), self.args)
+            utils.make_url("postgresql://u:p@host/test"), self.args, {})
         self.assertEqual('utf8', self.args['client_encoding'])
         self.assertFalse(self.args['connect_args'])
 
     def test_mysqlconnector_raise_on_warnings_default(self):
         engines._init_connection_args(
             utils.make_url("mysql+mysqlconnector://u:p@host/test"),
-            self.args)
+            self.args, {})
         self.assertEqual(False, self.args['connect_args']['raise_on_warnings'])
 
     def test_mysqlconnector_raise_on_warnings_override(self):
@@ -776,7 +776,7 @@ class CreateEngineTest(test_base.BaseTestCase):
             utils.make_url(
                 "mysql+mysqlconnector://u:p@host/test"
                 "?raise_on_warnings=true"),
-            self.args
+            self.args, {}
         )
 
         self.assertNotIn('raise_on_warnings', self.args['connect_args'])
@@ -851,18 +851,18 @@ class ProcessGuardTest(db_test_base._DbTestCase):
 
         with mock.patch("os.getpid", get_parent_pid):
             with self.engine.connect() as conn:
-                dbapi_id = id(conn.connection.connection)
+                dbapi_id = id(compat.driver_connection(conn))
 
         with mock.patch("os.getpid", get_child_pid):
             with self.engine.connect() as conn:
-                new_dbapi_id = id(conn.connection.connection)
+                new_dbapi_id = id(compat.driver_connection(conn))
 
         self.assertNotEqual(dbapi_id, new_dbapi_id)
 
         # ensure it doesn't trip again
         with mock.patch("os.getpid", get_child_pid):
             with self.engine.connect() as conn:
-                newer_dbapi_id = id(conn.connection.connection)
+                newer_dbapi_id = id(compat.driver_connection(conn))
 
         self.assertEqual(new_dbapi_id, newer_dbapi_id)
 
@@ -906,13 +906,12 @@ class MySQLConnectPingListenerTest(db_test_base._MySQLOpportunisticTestCase):
             with self.engine.begin() as conn:
                 self.assertTrue(isinstance(conn._transaction,
                                            base_engine.RootTransaction))
-                engines._connect_ping_listener(conn, False)
                 # TODO(ralonsoh): drop this check once SQLAlchemy minimum
                 #  version is 2.0.
-                sqla_version = versionutils.convert_version_to_tuple(
-                    sqlalchemy.__version__)
-                if sqla_version[0] >= 2:
+                if compat.sqla_2:
+                    engines._connect_ping_listener(conn)
                     self.assertIsNone(conn._transaction)
                 else:
+                    engines._connect_ping_listener(conn, False)
                     self.assertTrue(isinstance(conn._transaction,
                                                base_engine.RootTransaction))
diff --git a/oslo_db/tests/sqlalchemy/test_utils.py b/oslo_db/tests/sqlalchemy/test_utils.py
index c78ec06..059d015 100644
--- a/oslo_db/tests/sqlalchemy/test_utils.py
+++ b/oslo_db/tests/sqlalchemy/test_utils.py
@@ -26,7 +26,7 @@ from sqlalchemy.dialects.postgresql import psycopg2
 from sqlalchemy.exc import OperationalError
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.orm import declarative_base
-from sqlalchemy.orm import mapper
+from sqlalchemy.orm import registry
 from sqlalchemy.orm import Session
 from sqlalchemy import PrimaryKeyConstraint
 from sqlalchemy import sql
@@ -131,7 +131,8 @@ fake_table = Table(
     Column('key', String(50))
 )
 
-mapper(FakeTableClassicalyMapped, fake_table)
+reg = registry()
+reg.map_imperatively(FakeTableClassicalyMapped, fake_table)
 
 
 class FakeModel(object):
@@ -311,10 +312,16 @@ class TestPaginateQuery(test_base.BaseTestCase):
             'another_crit',
         ]
 
-        with mock.patch.object(self.model.user_id, 'isnot') as mock_isnot, \
-                mock.patch.object(self.model.user_id, 'is_') as mock_is_a, \
-                mock.patch.object(self.model.project_id, 'is_') as mock_is_b:
-            mock_isnot.return_value = 'asc_null_1'
+        with mock.patch.object(
+            self.model.user_id.comparator.expression, 'is_not'
+        ) as mock_is_not, \
+                mock.patch.object(
+                    self.model.user_id.comparator.expression, 'is_'
+                ) as mock_is_a, \
+                mock.patch.object(
+                    self.model.project_id.comparator.expression, 'is_'
+                ) as mock_is_b:
+            mock_is_not.return_value = 'asc_null_1'
             mock_is_a.side_effect = [
                 'desc_null_filter_1',
                 'desc_null_filter_2',
@@ -330,7 +337,7 @@ class TestPaginateQuery(test_base.BaseTestCase):
                                  sort_dirs=[
                                      'asc-nullslast', 'desc-nullsfirst'])
 
-            mock_isnot.assert_called_once_with(None)
+            mock_is_not.assert_called_once_with(None)
             mock_is_a.assert_has_calls([
                 mock.call(None),
                 mock.call(None),
@@ -384,11 +391,17 @@ class TestPaginateQuery(test_base.BaseTestCase):
         mock_and.return_value = 'some_crit'
         mock_or.side_effect = ['or_1', 'some_f']
 
-        with mock.patch.object(self.model.user_id, 'isnot') as mock_isnot, \
-                mock.patch.object(self.model.updated_at, 'is_') as mock_is_a, \
-                mock.patch.object(self.model.user_id, 'is_') as mock_is_b:
-
-            mock_isnot.return_value = 'asc_null_1'
+        with mock.patch.object(
+            self.model.user_id.comparator.expression, 'is_not'
+        ) as mock_is_not, \
+                mock.patch.object(
+                    self.model.updated_at.comparator.expression, 'is_'
+                ) as mock_is_a, \
+                mock.patch.object(
+                    self.model.user_id.comparator.expression, 'is_'
+                ) as mock_is_b:
+
+            mock_is_not.return_value = 'asc_null_1'
             mock_is_a.return_value = 'desc_null_1'
             mock_is_b.side_effect = ['asc_null_filter_1', 'asc_null_filter_2']
 
@@ -397,7 +410,7 @@ class TestPaginateQuery(test_base.BaseTestCase):
                                  marker=self.marker,
                                  sort_dirs=[
                                      'asc-nullslast', 'desc-nullsfirst'])
-            mock_isnot.assert_called_once_with(None)
+            mock_is_not.assert_called_once_with(None)
             mock_is_a.assert_called_once_with(None)
             mock_is_b.assert_has_calls([mock.call(None), mock.call(None)])
 
@@ -445,12 +458,20 @@ class TestPaginateQuery(test_base.BaseTestCase):
         ]
         self.query.filter.return_value = self.query
 
-        with mock.patch.object(self.model.user_id, 'isnot') as mock_isnot, \
-                mock.patch.object(self.model.updated_at, 'is_') as mock_is_a, \
-                mock.patch.object(self.model.user_id, 'is_') as mock_is_b, \
-                mock.patch.object(self.model.project_id, 'is_') as mock_is_c:
-
-            mock_isnot.return_value = 'asc_null_1'
+        with mock.patch.object(
+            self.model.user_id.comparator.expression, 'is_not'
+        ) as mock_is_not, \
+                mock.patch.object(
+                    self.model.updated_at.comparator.expression, 'is_'
+                ) as mock_is_a, \
+                mock.patch.object(
+                    self.model.user_id.comparator.expression, 'is_'
+                ) as mock_is_b, \
+                mock.patch.object(
+                    self.model.project_id.comparator.expression, 'is_'
+                ) as mock_is_c:
+
+            mock_is_not.return_value = 'asc_null_1'
             mock_is_a.return_value = 'desc_null_1'
             mock_is_b.side_effect = ['asc_null_filter_1', 'asc_null_filter_2']
             mock_is_c.side_effect = ['desc_null_3', 'desc_null_filter_3']
@@ -461,7 +482,7 @@ class TestPaginateQuery(test_base.BaseTestCase):
                                  sort_dirs=['asc-nullslast', 'desc-nullsfirst',
                                             'desc-nullsfirst'])
 
-            mock_isnot.assert_called_once_with(None)
+            mock_is_not.assert_called_once_with(None)
             mock_is_a.assert_called_once_with(None)
             mock_is_b.assert_has_calls([mock.call(None), mock.call(None)])
             mock_is_c.assert_has_calls([mock.call(None), mock.call(None)])
@@ -932,12 +953,12 @@ class TestConnectionUtils(test_base.BaseTestCase):
     def setUp(self):
         super(TestConnectionUtils, self).setUp()
 
-        self.full_credentials = {'backend': 'postgresql',
+        self.full_credentials = {'backend': 'postgresql+psycopg2',
                                  'database': 'test',
                                  'user': 'dude',
                                  'passwd': 'pass'}
 
-        self.connect_string = 'postgresql://dude:pass@localhost/test'
+        self.connect_string = 'postgresql+psycopg2://dude:pass@localhost/test'
 
         # NOTE(rpodolyaka): mock the dialect parts, so that we don't depend
         # on psycopg2 (or any other DBAPI implementation) in these tests
@@ -945,8 +966,20 @@ class TestConnectionUtils(test_base.BaseTestCase):
         @classmethod
         def fake_dbapi(cls):
             return mock.MagicMock()
-        patch_dbapi = mock.patch.object(psycopg2.PGDialect_psycopg2, 'dbapi',
-                                        new=fake_dbapi)
+
+        class OurDialect(psycopg2.PGDialect_psycopg2):
+            def dbapi(self):
+                return fake_dbapi
+
+            def import_dbapi(self):
+                return fake_dbapi
+
+        patch_dbapi = mock.patch.object(
+            psycopg2,
+            "PGDialect_psycopg2",
+            new=OurDialect,
+        )
+
         patch_dbapi.start()
         self.addCleanup(patch_dbapi.stop)
 
@@ -965,7 +998,7 @@ class TestConnectionUtils(test_base.BaseTestCase):
                 self.connect_string)
 
             self.assertIsInstance(eng, sqlalchemy.engine.base.Engine)
-            self.assertEqual(self.connect_string, str(eng.url))
+            self.assertEqual(utils.make_url(self.connect_string), eng.url)
 
             mock_connect.assert_called_once()
             fake_connection.close.assert_called_once()
@@ -982,10 +1015,10 @@ class TestConnectionUtils(test_base.BaseTestCase):
                 provision.Backend._ensure_backend_available,
                 self.connect_string)
             self.assertEqual(
-                "Backend 'postgresql' is unavailable: "
+                "Backend 'postgresql+psycopg2' is unavailable: "
                 "Could not connect", str(exc))
             self.assertEqual(
-                "The postgresql backend is unavailable: %s" % err,
+                "The postgresql+psycopg2 backend is unavailable: %s" % err,
                 log.output.strip())
 
     def test_ensure_backend_available_no_dbapi_raises(self):
@@ -1003,10 +1036,10 @@ class TestConnectionUtils(test_base.BaseTestCase):
                 utils.make_url(self.connect_string))
 
             self.assertEqual(
-                "Backend 'postgresql' is unavailable: "
+                "Backend 'postgresql+psycopg2' is unavailable: "
                 "No DBAPI installed", str(exc))
             self.assertEqual(
-                "The postgresql backend is unavailable: Can't import "
+                "The postgresql+psycopg2 backend is unavailable: Can't import "
                 "DBAPI module foobar", log.output.strip())
 
     def test_get_db_connection_info(self):