Merge "Ignore unset opts when deprecated"

oslo_db/sqlalchemy/enginefacade.py

@@ -69,7 +69,7 @@ should be directed at the master database.
 """
 
 
-class _Default(object):
+class _Default:
     """Mark a value as a default value.
 
     A value in the local configuration dictionary wrapped with
@@ -98,9 +98,9 @@ class _Default(object):
             return value
 
     @classmethod
-    def resolve_w_conf(cls, value, conf_namespace, key):
+    def resolve_w_conf(cls, value, conf, key):
         if isinstance(value, _Default):
-            v = getattr(conf_namespace, key, value.value)
+            v = getattr(conf.database, key, value.value)
             if v is cls._notset:
                 return None
             else:
@@ -110,14 +110,33 @@ class _Default(object):
 
     @classmethod
     def is_set(cls, value):
-        return not isinstance(value, _Default) or \
-            value.value is not cls._notset
+        if not isinstance(value, _Default):
+            return True
+
+        return value.value is not cls._notset
 
     @classmethod
-    def is_set_w_conf(cls, value, conf_namespace, key):
-        return not isinstance(value, _Default) or \
-            value.value is not cls._notset or \
-            hasattr(conf_namespace, key)
+    def is_set_w_conf(cls, value, conf, key):
+        if hasattr(conf.database, key):
+            # If the option is set via configuration then we should always
+            # respect it...unless that option is deprecated and hasn't been set
+            # by user, in which case we should ignore it.
+
+            # oslo.config doesn't provide a public API to retrieve the opt
+            # itself, as opposed to the value of the opt :(
+            opt = conf.database._group._opts[key]['opt']
+            # ditto for the group
+            group = conf.database._group
+            if (
+                opt.deprecated_for_removal and
+                conf.get_location(key, group=group.name).location ==
+                cfg.Locations.opt_default
+            ):
+                return False
+
+            return True
+
+        return cls.is_set(value)
 
 
 class AlreadyStartedError(TypeError):
@@ -128,7 +147,7 @@ class AlreadyStartedError(TypeError):
     """
 
 
-class _TransactionFactory(object):
+class _TransactionFactory:
     """A factory for :class:`._TransactionContext` objects.
 
     By default, there is just one of these, set up
@@ -146,7 +165,7 @@ class _TransactionFactory(object):
             'sqlite_fk': _Default(False),
             'mysql_sql_mode': _Default('TRADITIONAL'),
             'mysql_wsrep_sync_wait': _Default(),
-            'mysql_enable_ndb': _Default(False),
+            'mysql_enable_ndb': _Default(),
             'connection_recycle_time': _Default(3600),
             'connection_debug': _Default(0),
             'max_pool_size': _Default(),
@@ -182,7 +201,9 @@ class _TransactionFactory(object):
                 'db_max_retries', 'db_inc_retry_interval',
                 'use_db_reconnect',
                 'db_retry_interval',
-                'db_max_retry_interval', 'backend', 'use_tpool'])
+                'db_max_retry_interval', 'backend', 'use_tpool'
+            ]
+        )
 
         self._started = False
         self._legacy_facade = None
@@ -336,7 +357,8 @@ class _TransactionFactory(object):
             for dict_ in (
                     self._url_cfg, self._engine_cfg,
                     self._maker_cfg, self._ignored_cfg,
-                    self._facade_cfg, self._transaction_ctx_cfg):
+                    self._facade_cfg, self._transaction_ctx_cfg,
+            ):
                 if k in dict_:
                     dict_[k] = _Default(v) if as_defaults else v
                     break
@@ -447,17 +469,17 @@ class _TransactionFactory(object):
 
     def _args_for_conf(self, default_cfg, conf):
         if conf is None:
-            return dict(
-                (key, _Default.resolve(value))
+            return {
+                key: _Default.resolve(value)
                 for key, value in default_cfg.items()
                 if _Default.is_set(value)
-            )
+            }
         else:
-            return dict(
-                (key, _Default.resolve_w_conf(value, conf.database, key))
+            return {
+                key: _Default.resolve_w_conf(value, conf, key)
                 for key, value in default_cfg.items()
-                if _Default.is_set_w_conf(value, conf.database, key)
-            )
+                if _Default.is_set_w_conf(value, conf, key)
+            }
 
     def _url_args_for_conf(self, conf):
         return self._args_for_conf(self._url_cfg, conf)

oslo_db/tests/sqlalchemy/test_enginefacade.py

@@ -255,12 +255,18 @@ class MockFacadeTest(test_base.BaseTestCase):
         engine_factory = mock.Mock(side_effect=create_engine)
         engine_factory(
             sql_connection=self.engine_uri,
-            **dict((k, mock.ANY) for k in self.factory._engine_cfg.keys())
+            **{
+                k: mock.ANY for k in self.factory._engine_cfg.keys()
+                if k not in ('mysql_enable_ndb',)
+            },
         )
         if self.slave_uri:
             engine_factory(
                 sql_connection=self.slave_uri,
-                **dict((k, mock.ANY) for k in self.factory._engine_cfg.keys())
+                **{
+                    k: mock.ANY for k in self.factory._engine_cfg.keys()
+                    if k not in ('mysql_enable_ndb',)
+                },
             )
 
         yield AssertDataSource(

oslo_db/tests/sqlalchemy/test_sqlalchemy.py

@@ -409,8 +409,7 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
         for optname, optvalue in overrides.items():
             conf.set_override(optname, optvalue, group='database')
 
-        session.EngineFacade.from_config(conf,
-                                         expire_on_commit=True)
+        session.EngineFacade.from_config(conf, expire_on_commit=True)
 
         create_engine.assert_called_once_with(
             sql_connection='sqlite:///:memory:',
@@ -418,7 +417,9 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
             max_pool_size=10,
             mysql_sql_mode='TRADITIONAL',
             mysql_wsrep_sync_wait=None,
-            mysql_enable_ndb=False,
+            # NOTE: mysql_enable_ndb should *not* be passed through since it's
+            # deprecated and not set in the configopts
+            # mysql_enable_ndb=False,
             sqlite_fk=False,
             connection_recycle_time=mock.ANY,
             retry_interval=mock.ANY,
@@ -433,8 +434,57 @@ class EngineFacadeTestCase(test_base.BaseTestCase):
             connection_parameters='',
             logging_name=mock.ANY,
         )
-        get_maker.assert_called_once_with(engine=create_engine(),
-                                          expire_on_commit=True)
+        get_maker.assert_called_once_with(
+            engine=create_engine(), expire_on_commit=True,
+        )
+
+    @mock.patch('oslo_db.sqlalchemy.orm.get_maker')
+    @mock.patch('oslo_db.sqlalchemy.engines.create_engine')
+    def test_creation_from_config_with_deprecated_opts(
+        self, create_engine, get_maker,
+    ):
+        conf = cfg.ConfigOpts()
+        conf.register_opts(db_options.database_opts, group='database')
+
+        overrides = {
+            'connection': 'sqlite:///:memory:',
+            'slave_connection': None,
+            'connection_debug': 100,
+            'max_pool_size': 10,
+            'mysql_sql_mode': 'TRADITIONAL',
+            'mysql_enable_ndb': True,
+        }
+        for optname, optvalue in overrides.items():
+            conf.set_override(optname, optvalue, group='database')
+
+        session.EngineFacade.from_config(conf, expire_on_commit=True)
+
+        create_engine.assert_called_once_with(
+            sql_connection='sqlite:///:memory:',
+            connection_debug=100,
+            max_pool_size=10,
+            mysql_sql_mode='TRADITIONAL',
+            mysql_wsrep_sync_wait=None,
+            # NOTE: mysql_enable_ndb *should* be passed through since it's set
+            # in the configopts
+            mysql_enable_ndb=True,
+            sqlite_fk=False,
+            connection_recycle_time=mock.ANY,
+            retry_interval=mock.ANY,
+            max_retries=mock.ANY,
+            max_overflow=mock.ANY,
+            connection_trace=mock.ANY,
+            sqlite_synchronous=mock.ANY,
+            pool_timeout=mock.ANY,
+            thread_checkin=mock.ANY,
+            json_serializer=None,
+            json_deserializer=None,
+            connection_parameters='',
+            logging_name=mock.ANY,
+        )
+        get_maker.assert_called_once_with(
+            engine=create_engine(), expire_on_commit=True,
+        )
 
     def test_slave_connection(self):
         paths = self.create_tempfiles([('db.master', ''), ('db.slave', '')],