From 577d43b02d200b5b60eefc875f1542faa09ebfd9 Mon Sep 17 00:00:00 2001 From: Amrith Kumar Date: Fri, 3 Feb 2017 23:53:43 -0500 Subject: [PATCH] insulate TroveContext from possible changes in oslo.context Earlier code introduced in[1] aimed to handle the situation where a context object serialized and sent over a message queue as a dictionary is received by a version of code that doesn't necessarily understand that context object. That code relied on the fact that vars(TroveContext).keys() was a dictionary that matched the full set of parameters that you can send to the initializers for TroveContext() and RequestContext(). Recent changes in oslo.context [2] and [3] broke that assumption. vars() on TroveContext provided a dictionary with the internal representation(s) that are not the same as the kwargs in RequestContext() and the params that TroveContext used to pop. To get around this, new code introduced here uses oslo_utils.reflection to determine all the possible kwargs that can be sent to TroveContext, some of which are consumed by TroveContext and the rest of which are sent along to RequestContext(). The construct in the earlier _remove_incompatible_context_args() also modified kwargs.keys() by pop'ing values from it while iterating over this. Python 3 takes a dim view of this and therefore some changes have been made to accomodate this. A unittest has been added to ensure that the from_dict() method properly dumps stuff it doesn't know about. The test explicitly verifies the warning generated when a bogus argument is eliminated. [1] 24c5e8e244380e3e7e309f0b4aaffde32dbf0789 [2] 2394cff0631944a9259bfe04925e444d9f817758 [3] f25543fcc792ebf155728a91fde06e8dc4e96cea Change-Id: I477dd29e034295e770925091c4ac6268c22ae59b Related-Bug:#1661790 --- tools/trove-pylint.config | 12 ++++++ trove/common/context.py | 41 ++++++++++++-------- trove/tests/unittests/common/test_context.py | 22 +++++++++++ 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/tools/trove-pylint.config b/tools/trove-pylint.config index fad1504995..6a38a8bede 100644 --- a/tools/trove-pylint.config +++ b/tools/trove-pylint.config @@ -249,6 +249,18 @@ "Class 'Commands' has no 'has' member", "Commands.params_of" ], + [ + "trove/common/context.py", + "E1101", + "Module 'inspect' has no 'getfullargspec' member", + "TroveContext._remove_incompatible_context_args" + ], + [ + "trove/common/context.py", + "no-member", + "Module 'inspect' has no 'getfullargspec' member", + "TroveContext._remove_incompatible_context_args" + ], [ "trove/common/extensions.py", "E1003", diff --git a/trove/common/context.py b/trove/common/context.py index 5098626a22..ed00ed488a 100644 --- a/trove/common/context.py +++ b/trove/common/context.py @@ -20,9 +20,12 @@ Projects should subclass this class if they wish to enhance the request context or provide additional information in their specific WSGI pipeline. """ + from oslo_context import context from oslo_log import log as logging +from oslo_utils import reflection +from trove.common.i18n import _ from trove.common import local from trove.common.serializable_notification import SerializableNotification @@ -34,15 +37,15 @@ class TroveContext(context.RequestContext): Stores information about the security context under which the user accesses the system, as well as additional request information. """ - def __init__(self, **kwargs): - self.limit = kwargs.pop('limit', None) - self.marker = kwargs.pop('marker', None) - self.service_catalog = kwargs.pop('service_catalog', None) - self.user_identity = kwargs.pop('user_identity', None) - self.instance_id = kwargs.pop('instance_id', None) - - # TODO(esp): not sure we need this - self.timeout = kwargs.pop('timeout', None) + def __init__(self, limit=None, marker=None, service_catalog=None, + user_identity=None, instance_id=None, timeout=None, + **kwargs): + self.limit = limit + self.marker = marker + self.service_catalog = service_catalog + self.user_identity = user_identity + self.instance_id = instance_id + self.timeout = timeout super(TroveContext, self).__init__(**kwargs) if not hasattr(local.store, 'context'): @@ -65,16 +68,20 @@ class TroveContext(context.RequestContext): @classmethod def _remove_incompatible_context_args(cls, values): - LOG.debug("Running in unsafe mode and ignoring incompatible context.") - return values + realvalues = {} + + args = (reflection.get_callable_args(context.RequestContext.__init__) + + reflection.get_callable_args(TroveContext.__init__)) - context_keys = vars(cls()).keys() for dict_key in values.keys(): - if dict_key not in context_keys: - LOG.debug("Argument being removed before instantiating " - "TroveContext object - %s" % dict_key) - values.pop(dict_key, None) - return values + if dict_key in args: + realvalues[dict_key] = values.get(dict_key) + else: + LOG.warning(_("Argument being removed before instantiating " + "TroveContext object - %(key)s = %(value)s"), + {'key': dict_key, 'value': values.get(dict_key)}) + + return realvalues @classmethod def from_dict(cls, values): diff --git a/trove/tests/unittests/common/test_context.py b/trove/tests/unittests/common/test_context.py index f49a7a18c3..2c98d5b8e0 100644 --- a/trove/tests/unittests/common/test_context.py +++ b/trove/tests/unittests/common/test_context.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. # +import mock from mock import Mock from testtools.matchers import Equals, Is @@ -63,3 +64,24 @@ class TestTroveContext(trove_testtools.TestCase): self.assertThat(n_dict.get('notification_classname'), Equals('trove.common.notification.' 'DBaaSInstanceCreate')) + + def test_create_with_bogus(self): + with mock.patch('trove.common.context.LOG') as mock_log: + ctx = context.TroveContext.from_dict( + {'user': 'test_user_id', + 'request_id': 'test_req_id', + 'tenant': 'abc', + 'blah_blah': 'blah blah'}) + mock_log.warning.assert_called() + mock_log.warning.assert_called_with('Argument being removed ' + 'before instantiating ' + 'TroveContext object - ' + '%(key)s = %(value)s', + {'value': 'blah blah', + 'key': 'blah_blah'}) + self.assertThat(ctx.user, Equals('test_user_id')) + self.assertThat(ctx.request_id, Equals('test_req_id')) + self.assertThat(ctx.tenant, Equals('abc')) + self.assertThat(ctx.limit, Is(None)) + self.assertThat(ctx.marker, Is(None)) + self.assertThat(ctx.service_catalog, Is(None))