launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/operation-name-bug-987635 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/operation-name-bug-987635/+merge/104059

Massive D-O-R-I-S branch. (Doris Observes Real Itch Scratching)

Pre-imp: Discussed a bit with jtv.

I wanted to sort out the fact that all the api_exported() decorators largely specify the same name as the method they are decorating, apart from 2 cases.  So I started by writing tests for api_exported, because there were none.  Yes, none.

Then, I changed "operation_name" to "exported_as" (to be consistent with the Launchpad stuff) and made it optional.  I then noticed that it's actually already optional with some trickery in api_operations() so I removed that processing from there so it's all now inside the decorator.  And tested!  And I added docstrings!!!one!

Finally, I changed the ordering of the params to api_exported so you really don't need to specify any operation names any more, you can just state the http method.
-- 
https://code.launchpad.net/~julian-edwards/maas/operation-name-bug-987635/+merge/104059
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/operation-name-bug-987635 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-04-30 02:30:50 +0000
+++ src/maasserver/api.py	2012-04-30 06:43:19 +0000
@@ -155,15 +155,26 @@
             return actor, anonymous
 
 
-def api_exported(operation_name=True, method='POST'):
+def api_exported(method='POST', exported_as=None):
+    """Decorator to make a method available on the API.
+
+    :param method: The HTTP method over which to export the operation.
+    :param exported_as: Optional operation name; defaults to the name of the
+        exported method.
+
+    See also _`api_operations`.
+    """
     def _decorator(func):
         if method not in dispatch_methods:
             raise ValueError("Invalid method: '%s'" % method)
-        if operation_name == dispatch_methods.get(method):
+        if exported_as == dispatch_methods.get(method):
             raise ValueError(
                 "Cannot define a '%s' operation." % dispatch_methods.get(
                     method))
-        func._api_exported = {method: operation_name}
+        if exported_as is None:
+            func._api_exported = {method: func.__name__}
+        else:
+            func._api_exported = {method: exported_as}
         return func
     return _decorator
 
@@ -214,11 +225,11 @@
     >>> @api_operations
     >>> class MyHandler(BaseHandler):
     >>>
-    >>>    @api_exported('exported_post_name', method='POST')
+    >>>    @api_exported(method='POST', exported_as='exported_post_name')
     >>>    def do_x(self, request):
     >>>        # process request...
     >>>
-    >>>    @api_exported('exported_get_name', method='GET')
+    >>>    @api_exported(method='GET')
     >>>    def do_y(self, request):
     >>>        # process request...
 
@@ -229,9 +240,9 @@
     op=exported_post_name&param1=1
 
     MyHandler's method 'do_y' will service GET requests with
-    'op=exported_get_name' in its request parameters.
+    'op=do_y' in its request parameters.
 
-    GET /api/path/to/MyHandler/?op=exported_get_name&param1=1
+    GET /api/path/to/MyHandler/?op=do_y&param1=1
 
     """
     # Compute the list of methods ('GET', 'POST', etc.) that need to be
@@ -248,10 +259,9 @@
         cls._available_api_methods = getattr(
             cls, "_available_api_methods", {}).copy()
         cls._available_api_methods[method] = {
-            (name if op._api_exported[method] is True
-                else op._api_exported[method]): op
-            for name, op in operations.items()
-            if method in op._api_exported}
+            op._api_exported[method]: op
+                for name, op in operations.items()
+                if method in op._api_exported}
 
         def dispatcher(self, request, *args, **kwargs):
             return perform_api_operation(
@@ -390,7 +400,7 @@
             node_system_id = node.system_id
         return ('node_handler', (node_system_id, ))
 
-    @api_exported('stop', 'POST')
+    @api_exported('POST')
     def stop(self, request, system_id):
         """Shut down a node."""
         nodes = Node.objects.stop_nodes([system_id], request.user)
@@ -399,7 +409,7 @@
                 "You are not allowed to shut down this node.")
         return nodes[0]
 
-    @api_exported('start', 'POST')
+    @api_exported('POST')
     def start(self, request, system_id):
         """Power up a node.
 
@@ -421,7 +431,7 @@
                 "You are not allowed to start up this node.")
         return nodes[0]
 
-    @api_exported('release', 'POST')
+    @api_exported('POST')
     def release(self, request, system_id):
         """Release a node.  Opposite of `NodesHandler.acquire`."""
         node = Node.objects.get_node_or_404(
@@ -462,7 +472,7 @@
     allowed_methods = ('GET', 'POST',)
     fields = NODE_FIELDS
 
-    @api_exported('new', 'POST')
+    @api_exported('POST')
     def new(self, request):
         """Create a new Node.
 
@@ -473,7 +483,7 @@
         """
         return create_node(request)
 
-    @api_exported('is_registered', 'GET')
+    @api_exported('GET')
     def is_registered(self, request):
         """Returns whether or not the given MAC address is registered within
         this MAAS (and attached to a non-retired node).
@@ -488,12 +498,12 @@
             mac_address=mac_address).exclude(
                 node__status=NODE_STATUS.RETIRED).exists()
 
-    @api_exported('accept', 'POST')
+    @api_exported('POST')
     def accept(self, request):
         """Accept a node's enlistment: not allowed to anonymous users."""
         raise Unauthorized("You must be logged in to accept nodes.")
 
-    @api_exported("check_commissioning", "POST")
+    @api_exported("POST")
     def check_commissioning(self, request):
         """Check all commissioning nodes to see if they are taking too long.
 
@@ -534,7 +544,7 @@
     allowed_methods = ('GET', 'POST',)
     anonymous = AnonNodesHandler
 
-    @api_exported('new', 'POST')
+    @api_exported('POST')
     def new(self, request):
         """Create a new Node.
 
@@ -546,7 +556,7 @@
             node.accept_enlistment(request.user)
         return node
 
-    @api_exported('accept', 'POST')
+    @api_exported('POST')
     def accept(self, request):
         """Accept declared nodes into the MAAS.
 
@@ -584,7 +594,7 @@
         return filter(
             None, [node.accept_enlistment(request.user) for node in nodes])
 
-    @api_exported('list', 'GET')
+    @api_exported('GET')
     def list(self, request):
         """List Nodes visible to the user, optionally filtered by id."""
         match_ids = request.GET.getlist('id')
@@ -594,7 +604,7 @@
             request.user, NODE_PERMISSION.VIEW, ids=match_ids)
         return nodes.order_by('id')
 
-    @api_exported('list_allocated', 'GET')
+    @api_exported('GET')
     def list_allocated(self, request):
         """Fetch Nodes that were allocated to the User/oauth token."""
         token = get_oauth_token(request)
@@ -604,7 +614,7 @@
         nodes = Node.objects.get_allocated_visible_nodes(token, match_ids)
         return nodes.order_by('id')
 
-    @api_exported('acquire', 'POST')
+    @api_exported('POST')
     def acquire(self, request):
         """Acquire an available node for deployment."""
         node = Node.objects.get_available_node_for_acquisition(
@@ -713,7 +723,7 @@
     """
     allowed_methods = ('GET',)
 
-    get = api_exported('get', 'GET')(get_file)
+    get = api_exported('GET', exported_as='get')(get_file)
 
 
 @api_operations
@@ -722,9 +732,9 @@
     allowed_methods = ('GET', 'POST',)
     anonymous = AnonFilesHandler
 
-    get = api_exported('get', 'GET')(get_file)
+    get = api_exported('GET', exported_as='get')(get_file)
 
-    @api_exported('add', 'POST')
+    @api_exported('POST')
     def add(self, request):
         """Add a new file to the file storage.
 
@@ -759,7 +769,7 @@
     """Manage the current logged-in user."""
     allowed_methods = ('POST',)
 
-    @api_exported('create_authorisation_token', method='POST')
+    @api_exported(method='POST')
     def create_authorisation_token(self, request):
         """Create an authorisation OAuth token and OAuth consumer.
 
@@ -777,7 +787,7 @@
             'consumer_key': consumer.key,
             }
 
-    @api_exported('delete_authorisation_token', method='POST')
+    @api_exported(method='POST')
     def delete_authorisation_token(self, request):
         """Delete an authorisation OAuth token and the related OAuth consumer.
 
@@ -799,7 +809,7 @@
     """Manage the MAAS' itself."""
     allowed_methods = ('POST', 'GET')
 
-    @api_exported('set_config', method='POST')
+    @api_exported(method='POST')
     def set_config(self, request):
         """Set a config value.
 
@@ -814,7 +824,7 @@
         Config.objects.set_config(name, value)
         return rc.ALL_OK
 
-    @api_exported('get_config', method='GET')
+    @api_exported(method='GET')
     def get_config(self, request):
         """Get a config value.
 

=== added file 'src/maasserver/tests/test_api_mechanism.py'
--- src/maasserver/tests/test_api_mechanism.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_api_mechanism.py	2012-04-30 06:43:19 +0000
@@ -0,0 +1,68 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Testing of the API infrastructure, as opposed to code that uses it to
+export API methods.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maastesting.testcase import TestCase
+from maasserver.testing.factory import factory
+
+from maasserver.api import (
+    api_exported,
+    dispatch_methods,
+    )
+
+
+class TestApiExported(TestCase):
+    """Testing for the api_exported decorator."""
+
+    def test_invalid_method(self):
+        # If the supplied HTTP method is not in the allowed set, it should
+        # raise a ValueError.
+        random_method = "method" + factory.getRandomString(4)
+        decorate = api_exported(random_method)
+        self.assertRaises(ValueError, decorate, lambda: None)
+
+    def test_allowed_methods(self):
+        # HTTP methods in dispatch_methods should not be allowed.
+        for method in dispatch_methods:
+            decorate = api_exported(method=dispatch_methods.get(method))
+            self.assertRaises(ValueError, decorate, lambda: None)
+
+    def test_valid_decoration(self):
+        value = "value" + factory.getRandomString()
+        decorate = api_exported()
+        decorated = decorate(lambda: value)
+        self.assertEqual(value, decorated())
+
+    def test_can_pass_export_as(self):
+        # Test that passing the optional "export_as" works as expected.
+        random_exported_name = "exportedas" + factory.getRandomString()
+        decorate = api_exported(
+            "POST", exported_as=random_exported_name)
+        decorated = decorate(lambda: None)
+
+        self.assertEqual(
+            random_exported_name, decorated._api_exported["POST"])
+
+    def test_export_as_is_optional(self):
+        # If export_as is not passed then we expect the function to be
+        # exported in the API using the actual function name itself.
+
+        def exported_function():
+            pass
+
+        decorate = api_exported("POST")
+        decorated = decorate(exported_function)
+
+        self.assertEqual("exported_function", decorated._api_exported["POST"])

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2012-04-24 15:12:18 +0000
+++ src/metadataserver/api.py	2012-04-30 06:43:19 +0000
@@ -128,7 +128,7 @@
             contents = uploaded_file.read().decode('utf-8')
             NodeCommissionResult.objects.store_data(node, name, contents)
 
-    @api_exported('signal', 'POST')
+    @api_exported('POST')
     def signal(self, request, version=None):
         """Signal commissioning status.