From bb52092cd37855a3dfcfae9287e231865e9af3fa Mon Sep 17 00:00:00 2001 From: Aashish Sharma Date: Wed, 19 May 2021 11:27:13 +0530 Subject: [PATCH] mgr/dashboard: API Version changes doesnt appy to pre-defined methods(list, create etc.) Methods like list(), create(), get() etc doesn't get applied the version.Also for the endpoints that get the version changed, the docs and the request header has still the version v1.0+ in them. So with the version reduced it gives 415 error when trying to make the request. This PR fixes this issue. Fixes: https://tracker.ceph.com/issues/50855 Signed-off-by: Aashish Sharma --- doc/dev/developer_guide/dash-devel.rst | 22 ++++++- .../mgr/dashboard/controllers/__init__.py | 65 ++++++++++++------- src/pybind/mgr/dashboard/controllers/docs.py | 35 +++++++--- src/pybind/mgr/dashboard/tests/test_docs.py | 21 ++++-- .../mgr/dashboard/tests/test_versioning.py | 10 +++ 5 files changed, 112 insertions(+), 41 deletions(-) diff --git a/doc/dev/developer_guide/dash-devel.rst b/doc/dev/developer_guide/dash-devel.rst index 4cd81a1dcb5..5702806b7d7 100644 --- a/doc/dev/developer_guide/dash-devel.rst +++ b/doc/dev/developer_guide/dash-devel.rst @@ -1466,6 +1466,25 @@ same applies to other request types: | DELETE | Yes | delete | 204 | +--------------+------------+----------------+-------------+ +To use a custom endpoint for the above listed methods, you can +use ``@RESTController.MethodMap`` + +.. code-block:: python + + import cherrypy + from ..tools import ApiController, RESTController + + @RESTController.MethodMap(version='0.1') + def create(self): + return {"msg": "Hello"} + +This decorator supports three parameters to customize the +endpoint: + +* ``resource"``: resource id. +* ``status=200``: set the HTTP status response code +* ``version``: version + How to use a custom API endpoint in a RESTController? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -1494,7 +1513,7 @@ used. To use a custom endpoint inside a restricted ``RESTController`` use def some_post_endpoint(self, **data): return {"msg": data} -Both decorators also support four parameters to customize the +Both decorators also support five parameters to customize the endpoint: * ``method="GET"``: the HTTP method allowed to access this endpoint. @@ -1503,6 +1522,7 @@ endpoint: * ``status=200``: set the HTTP status response code * ``query_params=[]``: list of method parameter names that correspond to URL query parameters. +* ``version``: version How to restrict access to a controller? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/pybind/mgr/dashboard/controllers/__init__.py b/src/pybind/mgr/dashboard/controllers/__init__.py index ee8d3bd7bb4..babfbb4296d 100755 --- a/src/pybind/mgr/dashboard/controllers/__init__.py +++ b/src/pybind/mgr/dashboard/controllers/__init__.py @@ -812,14 +812,14 @@ class RESTController(BaseController): } _method_mapping = collections.OrderedDict([ - ('list', {'method': 'GET', 'resource': False, 'status': 200}), - ('create', {'method': 'POST', 'resource': False, 'status': 201}), - ('bulk_set', {'method': 'PUT', 'resource': False, 'status': 200}), - ('bulk_delete', {'method': 'DELETE', 'resource': False, 'status': 204}), - ('get', {'method': 'GET', 'resource': True, 'status': 200}), - ('delete', {'method': 'DELETE', 'resource': True, 'status': 204}), - ('set', {'method': 'PUT', 'resource': True, 'status': 200}), - ('singleton_set', {'method': 'PUT', 'resource': False, 'status': 200}) + ('list', {'method': 'GET', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}), + ('create', {'method': 'POST', 'resource': False, 'status': 201, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('bulk_set', {'method': 'PUT', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('bulk_delete', {'method': 'DELETE', 'resource': False, 'status': 204, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('get', {'method': 'GET', 'resource': True, 'status': 200, 'version': DEFAULT_VERSION}), + ('delete', {'method': 'DELETE', 'resource': True, 'status': 204, 'version': DEFAULT_VERSION}), # noqa E501 #pylint: disable=line-too-long + ('set', {'method': 'PUT', 'resource': True, 'status': 200, 'version': DEFAULT_VERSION}), + ('singleton_set', {'method': 'PUT', 'resource': False, 'status': 200, 'version': DEFAULT_VERSION}) # noqa E501 #pylint: disable=line-too-long ]) @classmethod @@ -858,10 +858,10 @@ class RESTController(BaseController): cls._update_endpoint_params_method_map( func, res_id_params, endpoint_params) - elif hasattr(func, "_collection_method_"): + elif hasattr(func, "collection_method"): cls._update_endpoint_params_collection_map(func, endpoint_params) - elif hasattr(func, "_resource_method_"): + elif hasattr(func, "resource_method"): cls._update_endpoint_params_resource_method( res_id_params, endpoint_params, func) @@ -900,27 +900,27 @@ class RESTController(BaseController): else: path_params = ["{{{}}}".format(p) for p in res_id_params] endpoint_params['path'] += "/{}".format("/".join(path_params)) - if func._resource_method_['path']: - endpoint_params['path'] += func._resource_method_['path'] + if func.resource_method['path']: + endpoint_params['path'] += func.resource_method['path'] else: endpoint_params['path'] += "/{}".format(func.__name__) - endpoint_params['status'] = func._resource_method_['status'] - endpoint_params['method'] = func._resource_method_['method'] - endpoint_params['version'] = func._resource_method_['version'] - endpoint_params['query_params'] = func._resource_method_['query_params'] + endpoint_params['status'] = func.resource_method['status'] + endpoint_params['method'] = func.resource_method['method'] + endpoint_params['version'] = func.resource_method['version'] + endpoint_params['query_params'] = func.resource_method['query_params'] if not endpoint_params['sec_permissions']: endpoint_params['permission'] = cls._permission_map[endpoint_params['method']] @classmethod def _update_endpoint_params_collection_map(cls, func, endpoint_params): - if func._collection_method_['path']: - endpoint_params['path'] = func._collection_method_['path'] + if func.collection_method['path']: + endpoint_params['path'] = func.collection_method['path'] else: endpoint_params['path'] = "/{}".format(func.__name__) - endpoint_params['status'] = func._collection_method_['status'] - endpoint_params['method'] = func._collection_method_['method'] - endpoint_params['query_params'] = func._collection_method_['query_params'] - endpoint_params['version'] = func._collection_method_['version'] + endpoint_params['status'] = func.collection_method['status'] + endpoint_params['method'] = func.collection_method['method'] + endpoint_params['query_params'] = func.collection_method['query_params'] + endpoint_params['version'] = func.collection_method['version'] if not endpoint_params['sec_permissions']: endpoint_params['permission'] = cls._permission_map[endpoint_params['method']] @@ -937,6 +937,8 @@ class RESTController(BaseController): endpoint_params['status'] = meth['status'] endpoint_params['method'] = meth['method'] + if hasattr(func, "method_map_method"): + endpoint_params['version'] = func.method_map_method['version'] if not endpoint_params['sec_permissions']: endpoint_params['permission'] = cls._permission_map[endpoint_params['method']] @@ -959,7 +961,7 @@ class RESTController(BaseController): status = 200 def _wrapper(func): - func._resource_method_ = { + func.resource_method = { 'method': method, 'path': path, 'status': status, @@ -969,6 +971,21 @@ class RESTController(BaseController): return func return _wrapper + @staticmethod + def MethodMap(resource=False, status=None, version=DEFAULT_VERSION): # noqa: N802 + + if status is None: + status = 200 + + def _wrapper(func): + func.method_map_method = { + 'resource': resource, + 'status': status, + 'version': version + } + return func + return _wrapper + @staticmethod def Collection(method=None, path=None, status=None, query_params=None, # noqa: N802 version=DEFAULT_VERSION): @@ -979,7 +996,7 @@ class RESTController(BaseController): status = 200 def _wrapper(func): - func._collection_method_ = { + func.collection_method = { 'method': method, 'path': path, 'status': status, diff --git a/src/pybind/mgr/dashboard/controllers/docs.py b/src/pybind/mgr/dashboard/controllers/docs.py index 295a36ad855..368dbeb4240 100644 --- a/src/pybind/mgr/dashboard/controllers/docs.py +++ b/src/pybind/mgr/dashboard/controllers/docs.py @@ -185,7 +185,7 @@ class Docs(BaseController): return schema.as_dict() @classmethod - def _gen_responses(cls, method, resp_object=None): + def _gen_responses(cls, method, resp_object=None, version=None): resp: Dict[str, Dict[str, Union[str, Any]]] = { '400': { "description": "Operation exception. Please check the " @@ -203,26 +203,30 @@ class Docs(BaseController): "response body for the stack trace." } } + + if not version: + version = DEFAULT_VERSION + if method.lower() == 'get': resp['200'] = {'description': "OK", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'post': resp['201'] = {'description': "Resource created.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'put': resp['200'] = {'description': "Resource updated.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() == 'delete': resp['204'] = {'description': "Resource deleted.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if method.lower() in ['post', 'put', 'delete']: resp['202'] = {'description': "Operation is still executing." " Please check the task queue.", - 'content': {'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): + 'content': {'application/vnd.ceph.api.v{}+json'.format(version): {'type': 'object'}}} if resp_object: @@ -230,7 +234,7 @@ class Docs(BaseController): if status_code in resp: resp[status_code].update({ 'content': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v{}+json'.format(version): { 'schema': cls._gen_schema_for_content(response_body)}}}) return resp @@ -263,7 +267,7 @@ class Docs(BaseController): return parameters @classmethod - def _gen_paths(cls, all_endpoints): + def gen_paths(cls, all_endpoints): # pylint: disable=R0912 method_order = ['get', 'post', 'put', 'delete'] paths = {} @@ -283,8 +287,19 @@ class Docs(BaseController): func = endpoint.func summary = '' + version = '' resp = {} p_info = [] + + if hasattr(func, 'method_map_method'): + version = func.method_map_method['version'] + + elif hasattr(func, 'resource_method'): + version = func.resource_method['version'] + + elif hasattr(func, 'collection_method'): + version = func.collection_method['version'] + if hasattr(func, 'doc_info'): if func.doc_info['summary']: summary = func.doc_info['summary'] @@ -304,7 +319,7 @@ class Docs(BaseController): 'tags': [cls._get_tag(endpoint)], 'description': func.__doc__, 'parameters': params, - 'responses': cls._gen_responses(method, resp) + 'responses': cls._gen_responses(method, resp, version) } if summary: methods[method.lower()]['summary'] = summary @@ -340,7 +355,7 @@ class Docs(BaseController): host = cherrypy.request.base.split('://', 1)[1] if not offline else 'example.com' logger.debug("Host: %s", host) - paths = cls._gen_paths(all_endpoints) + paths = cls.gen_paths(all_endpoints) if not base_url: base_url = "/" diff --git a/src/pybind/mgr/dashboard/tests/test_docs.py b/src/pybind/mgr/dashboard/tests/test_docs.py index 84cfddd629e..63d5f1731a1 100644 --- a/src/pybind/mgr/dashboard/tests/test_docs.py +++ b/src/pybind/mgr/dashboard/tests/test_docs.py @@ -3,7 +3,6 @@ from __future__ import absolute_import import unittest -from .. import DEFAULT_VERSION from ..api.doc import SchemaType from ..controllers import ApiController, ControllerDoc, Endpoint, EndpointDoc, RESTController from ..controllers.docs import Docs @@ -32,10 +31,14 @@ class DecoratedController(RESTController): }, ) @Endpoint(json_response=False) - @RESTController.Resource('PUT') + @RESTController.Resource('PUT', version='0.1') def decorated_func(self, parameter): pass + @RESTController.MethodMap(version='0.1') + def list(self): + pass + # To assure functionality of @EndpointDoc, @GroupDoc class DocDecoratorsTest(ControllerTestCase): @@ -77,7 +80,7 @@ class DocsTest(ControllerTestCase): self.assertEqual(Docs()._type_to_str(None), str(SchemaType.OBJECT)) def test_gen_paths(self): - outcome = Docs()._gen_paths(False)['/api/doctest//{doctest}/decorated_func']['put'] + outcome = Docs().gen_paths(False)['/api/doctest//{doctest}/decorated_func']['put'] self.assertIn('tags', outcome) self.assertIn('summary', outcome) self.assertIn('parameters', outcome) @@ -85,7 +88,7 @@ class DocsTest(ControllerTestCase): expected_response_content = { '200': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v0.1+json': { 'schema': {'type': 'array', 'items': {'type': 'object', 'properties': { 'my_prop': { @@ -93,7 +96,7 @@ class DocsTest(ControllerTestCase): 'description': '200 property desc.'}}}, 'required': ['my_prop']}}}, '202': { - 'application/vnd.ceph.api.v{}+json'.format(DEFAULT_VERSION): { + 'application/vnd.ceph.api.v0.1+json': { 'schema': {'type': 'object', 'properties': {'my_prop': { 'type': 'string', @@ -106,8 +109,14 @@ class DocsTest(ControllerTestCase): # Check that a schema of type 'object' is received in the response. self.assertEqual(expected_response_content['202'], outcome['responses']['202']['content']) + def test_gen_method_paths(self): + outcome = Docs().gen_paths(False)['/api/doctest/']['get'] + + self.assertEqual({'application/vnd.ceph.api.v0.1+json': {'type': 'object'}}, + outcome['responses']['200']['content']) + def test_gen_paths_all(self): - paths = Docs()._gen_paths(False) + paths = Docs().gen_paths(False) for key in paths: self.assertTrue(any(base in key.split('/')[1] for base in ['api', 'ui-api'])) diff --git a/src/pybind/mgr/dashboard/tests/test_versioning.py b/src/pybind/mgr/dashboard/tests/test_versioning.py index d051e0550a4..666b388bf28 100644 --- a/src/pybind/mgr/dashboard/tests/test_versioning.py +++ b/src/pybind/mgr/dashboard/tests/test_versioning.py @@ -11,6 +11,7 @@ from . import ControllerTestCase # pylint: disable=no-name-in-module class VTest(RESTController): RESOURCE_ID = "vid" + @RESTController.MethodMap(version="0.1") def list(self): return {'version': ""} @@ -31,6 +32,15 @@ class RESTVersioningTest(ControllerTestCase, unittest.TestCase): def setup_server(cls): cls.setup_controllers([VTest], "/test") + def test_list(self): + for (version, expected_status) in [ + ("0.1", 200), + ("2.0", 415) + ]: + with self.subTest(version=version): + self._get('/test/api/vtest', version=version) + self.assertStatus(expected_status) + def test_v1(self): for (version, expected_status) in [ ("1.0", 200), -- 2.47.3