Merge "Support microversions on inherited Controllers"

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":
+            # Get the versioned methods that this base has
-                # NOTE(cyeoh): This resets the VER_METHOD_ATTR attribute
+            if VER_METHOD_ATTR in vars(base):
-                # between API controller class creations. This allows us
+                versioned_methods.append(getattr(base, VER_METHOD_ATTR))
-                # 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)
 
         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):

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,