From b3732ee2978c63d48cfbfcde3116e5140af5e995 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 30 Jun 2016 15:47:12 +0200 Subject: [PATCH] Support microversions on inherited Controllers Current microversions implementation only work on Controllers that inherit directly from cinder.api.openstack.wsgi.Controller, which means that our v3 Controllers that inherit it's behavior from v2 Controllers will have problems with new added methods as these will not get added to the right class and cross pollinate other classes. It's because we assume that all our Controllers inherit from wsgi.Controller and there will be no more inheritance. This patch allows out Controller classes to inherit from other Controller classes and get the right versioned methods. Closes-Bug: #1597771 Change-Id: I07f16e1efafe4e230af7016eff2e45e5c1f2f329 --- cinder/api/openstack/wsgi.py | 48 ++++++-- cinder/tests/unit/api/test_versions.py | 156 +++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 12 deletions(-) diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py index 48e0c868343..cc4718e55fd 100644 --- a/cinder/api/openstack/wsgi.py +++ b/cinder/api/openstack/wsgi.py @@ -1058,21 +1058,32 @@ class ControllerMetaclass(type): # Find all actions actions = {} extensions = [] - versioned_methods = None + # NOTE(geguileo): We'll keep a list of versioned methods that have been + # added by the new metaclass (dictionary in attribute VER_METHOD_ATTR + # on Controller class) and all the versioned methods from the different + # base classes so we can consolidate them. + versioned_methods = [] + + # NOTE(cyeoh): This resets the VER_METHOD_ATTR attribute + # between API controller class creations. This allows us + # to use a class decorator on the API methods that doesn't + # require naming explicitly what method is being versioned as + # it can be implicit based on the method decorated. It is a bit + # ugly. + if bases != (object,) and VER_METHOD_ATTR in vars(Controller): + # Get the versioned methods that this metaclass creation has added + # to the Controller class + versioned_methods.append(getattr(Controller, VER_METHOD_ATTR)) + # Remove them so next metaclass has a clean start + delattr(Controller, VER_METHOD_ATTR) + # start with wsgi actions from base classes for base in bases: actions.update(getattr(base, 'wsgi_actions', {})) - if base.__name__ == "Controller": - # NOTE(cyeoh): This resets the VER_METHOD_ATTR attribute - # between API controller class creations. This allows us - # to use a class decorator on the API methods that doesn't - # require naming explicitly what method is being versioned as - # it can be implicit based on the method decorated. It is a bit - # ugly. - if VER_METHOD_ATTR in base.__dict__: - versioned_methods = getattr(base, VER_METHOD_ATTR) - delattr(base, VER_METHOD_ATTR) + # Get the versioned methods that this base has + if VER_METHOD_ATTR in vars(base): + versioned_methods.append(getattr(base, VER_METHOD_ATTR)) for key, value in cls_dict.items(): if not callable(value): @@ -1086,11 +1097,24 @@ class ControllerMetaclass(type): cls_dict['wsgi_actions'] = actions cls_dict['wsgi_extensions'] = extensions if versioned_methods: - cls_dict[VER_METHOD_ATTR] = versioned_methods + cls_dict[VER_METHOD_ATTR] = mcs.consolidate_vers(versioned_methods) return super(ControllerMetaclass, mcs).__new__(mcs, name, bases, cls_dict) + @staticmethod + def consolidate_vers(versioned_methods): + """Consolidates a list of versioned methods dictionaries.""" + if not versioned_methods: + return {} + result = versioned_methods.pop(0) + for base_methods in versioned_methods: + for name, methods in base_methods.items(): + method_list = result.setdefault(name, []) + method_list.extend(methods) + method_list.sort(reverse=True) + return result + @six.add_metaclass(ControllerMetaclass) class Controller(object): diff --git a/cinder/tests/unit/api/test_versions.py b/cinder/tests/unit/api/test_versions.py index c459397cb8c..1d3dc9c842a 100644 --- a/cinder/tests/unit/api/test_versions.py +++ b/cinder/tests/unit/api/test_versions.py @@ -17,6 +17,7 @@ import ddt import mock from oslo_serialization import jsonutils from oslo_utils import encodeutils +import six import webob from cinder.api.openstack import api_version_request @@ -154,6 +155,161 @@ class VersionsControllerTestCase(test.TestCase): else: self.assertNotIn(VERSION_HEADER_NAME, response.headers) + def test_versions_inheritance_internals_of_non_base_controller(self): + """Test ControllerMetaclass works inheriting from non base class.""" + def _get_str_version(version): + return "%s.%s" % (version._ver_major, version._ver_minor) + + def assert_method_equal(expected, observed): + if six.PY2: + expected = expected.im_func + self.assertEqual(expected, observed) + + class ControllerParent(wsgi.Controller): + @wsgi.Controller.api_version('3.0') + def index(self, req): + pass + + # We create this class in between to confirm that we don't leave + # undesired versioned methods in the wsgi.Controller class. + class Controller(wsgi.Controller): + @wsgi.Controller.api_version('2.0') + def index(self, req): + pass + + class ControllerChild(ControllerParent): + @wsgi.Controller.api_version('3.1') + def index(self, req): + pass + + @wsgi.Controller.api_version('3.2') + def new_method(self, req): + pass + + # ControllerParent will only have its own index method + self.assertSetEqual({'index'}, set(ControllerParent.versioned_methods)) + self.assertEqual(1, len(ControllerParent.versioned_methods['index'])) + index = ControllerParent.versioned_methods['index'][0] + assert_method_equal(ControllerParent.index, index.func) + self.assertEqual('index', index.name) + self.assertEqual('3.0', _get_str_version(index.start_version)) + self.assertEqual('None.None', _get_str_version(index.end_version)) + + # Same thing will happen with the Controller class, thus confirming + # that we don't cross pollinate our classes with undesired methods. + self.assertSetEqual({'index'}, set(Controller.versioned_methods)) + self.assertEqual(1, len(Controller.versioned_methods['index'])) + index = Controller.versioned_methods['index'][0] + assert_method_equal(Controller.index, index.func) + self.assertEqual('index', index.name) + self.assertEqual('2.0', _get_str_version(index.start_version)) + self.assertEqual('None.None', _get_str_version(index.end_version)) + + # ControllerChild will inherit index method from ControllerParent and + # add its own version as well as add a new method + self.assertSetEqual({'index', 'new_method'}, + set(ControllerChild.versioned_methods)) + self.assertEqual(2, len(ControllerChild.versioned_methods['index'])) + + # The methods are ordered from newest version to oldest version + index = ControllerChild.versioned_methods['index'][0] + assert_method_equal(ControllerChild.index, index.func) + self.assertEqual('index', index.name) + self.assertEqual('3.1', _get_str_version(index.start_version)) + self.assertEqual('None.None', _get_str_version(index.end_version)) + + index = ControllerChild.versioned_methods['index'][1] + assert_method_equal(ControllerParent.index, index.func) + self.assertEqual('index', index.name) + self.assertEqual('3.0', _get_str_version(index.start_version)) + self.assertEqual('None.None', _get_str_version(index.end_version)) + + # New method also gets added even if it didn't exist in any of the base + # classes. + self.assertEqual(1, + len(ControllerChild.versioned_methods['new_method'])) + new_method = ControllerChild.versioned_methods['new_method'][0] + assert_method_equal(ControllerChild.new_method, new_method.func) + self.assertEqual('new_method', new_method.name) + self.assertEqual('3.2', _get_str_version(new_method.start_version)) + self.assertEqual('None.None', _get_str_version(new_method.end_version)) + + @ddt.data( + ('2.0', 'index', 406, 'ControllerParent'), + ('2.0', 'show', 406, 'ControllerParent'), + ('3.0', 'index', 404, 'ControllerParent'), + ('3.0', 'show', 404, 'ControllerParent'), + ('3.1', 'index', 'parent', 'ControllerParent'), + ('3.1', 'show', 404, 'ControllerParent'), + ('3.2', 'index', 'parent', 'ControllerParent'), + ('3.2', 'show', 404, 'ControllerParent'), + + ('2.0', 'index', 406, 'Controller'), + ('2.0', 'show', 406, 'Controller'), + ('3.0', 'index', 404, 'Controller'), + ('3.0', 'show', 404, 'Controller'), + ('3.1', 'index', 'single', 'Controller'), + ('3.1', 'show', 404, 'Controller'), + ('3.2', 'index', 'single', 'Controller'), + ('3.2', 'show', 404, 'Controller'), + + ('2.0', 'index', 406, 'ControllerChild'), + ('2.0', 'show', 406, 'ControllerChild'), + ('3.0', 'index', 404, 'ControllerChild'), + ('3.0', 'show', 404, 'ControllerChild'), + ('3.1', 'index', 'parent', 'ControllerChild'), + ('3.1', 'show', 404, 'ControllerChild'), + ('3.2', 'index', 'child 3.2', 'ControllerChild'), + ('3.2', 'show', 404, 'ControllerChild'), + ('3.3', 'index', 'child 3.3', 'ControllerChild'), + ('3.3', 'show', 'show', 'ControllerChild')) + @ddt.unpack + def test_versions_inheritance_of_non_base_controller(self, version, call, + expected, controller): + """Test ControllerMetaclass works inheriting from non base class.""" + class ControllerParent(wsgi.Controller): + @wsgi.Controller.api_version('3.1') + def index(self, req): + return 'parent' + + # We create this class in between to confirm that we don't leave + # undesired versioned methods in the wsgi.Controller class. + class Controller(wsgi.Controller): + @wsgi.Controller.api_version('3.1') + def index(self, req): + return 'single' + + class ControllerChild(ControllerParent): + # We don't add max version to confirm that once we set a newer + # version it doesn't really matter because the newest one will be + # called. + @wsgi.Controller.api_version('3.2') + def index(self, req): + return 'child 3.2' + + # TODO(geguileo): Figure out a way to make microversions work in a + # way that doesn't raise complaints from duplicated method. + @wsgi.Controller.api_version('3.3') # noqa + def index(self, req): + return 'child 3.3' + + @wsgi.Controller.api_version('3.3') + def show(self, req, *args, **kwargs): + return 'show' + + base_dir = '/tests' if call == 'index' else '/tests/123' + req = self.build_request(base_dir=base_dir, header_version=version) + app = fakes.TestRouter(locals()[controller]()) + + response = req.get_response(app) + resp = encodeutils.safe_decode(response.body, incoming='utf-8') + + if isinstance(expected, six.string_types): + self.assertEqual(200, response.status_int) + self.assertEqual(expected, resp) + else: + self.assertEqual(expected, response.status_int) + def test_versions_version_not_found(self): api_version_request_4_0 = api_version_request.APIVersionRequest('4.0') self.mock_object(api_version_request,