launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/default-crud into lp:maas.

Commit message:
Fix the API so that CRUD operations can be defined alongside "real" operations (i.e. calls with op=something).

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/default-crud/+merge/126413

This branch adds the ability to define normal CRUD operations alongside "real" operations (i.e. calls with op=something).

= Pre-imp =

No real pre-imp for this but:
- all the details (and probably more) is documented on bug 1049933.
- this was discussed with jam when he added the XXXed code that this branch cleans up.

= Notes =

It boils down to this: since we hijack the default CRUB operations in order to allow the API to route request to the right method depending on the value of the 'op' parameter, we removed the possibility to have both CRUB operations alongside real operations for the same method ('GET', 'POST', etc.) on a handler.

I choose to extend our @api_exported utility because I think that fits best with our model.  Another solution would have been to dynamically save the default method ('read', 'create', etc.) to avoid it to be overridden by our custom method (remember, we replace the default method to route requests) but I thought this would have been more clumsy.
-- 
https://code.launchpad.net/~rvb/maas/default-crud/+merge/126413
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/default-crud into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-25 11:21:25 +0000
+++ src/maasserver/api.py	2012-09-26 09:09:26 +0000
@@ -194,19 +194,28 @@
             return actor, anonymous
 
 
-def api_exported(method='POST', exported_as=None):
+def api_exported(method='POST', exported_as=None, default_operation=False):
     """Decorator to make a method available on the API.
 
     :param method: The HTTP method over which to export the operation.
+    :type method: basestring
     :param exported_as: Optional operation name; defaults to the name of the
         exported method.
+    :type exported_as: basestring
+    :param default_operation: Should this method be exported as the default
+        operation when no 'op' parameter is provided?
+    :type default_operation: bool
 
     See also _`api_operations`.
     """
     def _decorator(func):
         if method not in dispatch_methods:
             raise ValueError("Invalid method: '%s'" % method)
-        if exported_as is None:
+        if default_operation:
+            # This will be published as the default CRUD operation (i.e.
+            # when no 'op' parameter is provided), set its name to None.
+            func._api_exported = {method: None}
+        elif exported_as is None:
             func._api_exported = {method: func.__name__}
         else:
             func._api_exported = {method: exported_as}
@@ -227,8 +236,7 @@
     op_types = types.FunctionType, types.MethodType
     return (
         isinstance(thing, op_types) and
-        getattr(thing, "_api_exported", None) is not None and
-        getattr(thing, "_api_exported", None).get(method, None) is not None)
+        getattr(thing, "_api_exported", None) is not None)
 
 
 # Define a method that will route requests to the methods registered in
@@ -239,15 +247,20 @@
     else:
         data = request.GET
     op = data.get(OP_PARAM, None)
-    if not isinstance(op, unicode):
-        return HttpResponseBadRequest("Unknown operation.")
-    elif method not in handler._available_api_methods:
-        return HttpResponseBadRequest("Unknown operation: '%s'." % op)
+    if op is not None:
+        # Make sure that the specified operation is valid.
+        if not isinstance(op, unicode):
+            return HttpResponseBadRequest("Unknown operation.")
+        unknown_operation_message = "Unknown operation: '%s'." % op
+    else:
+        # No operation specified.
+        unknown_operation_message = "Unknown default operation."
+    if method not in handler._available_api_methods:
+        return HttpResponseBadRequest(unknown_operation_message)
     elif op not in handler._available_api_methods[method]:
-        return HttpResponseBadRequest("Unknown operation: '%s'." % op)
-    else:
-        method = handler._available_api_methods[method][op]
-        return method(handler, request, *args, **kwargs)
+        return HttpResponseBadRequest(unknown_operation_message)
+    method = handler._available_api_methods[method][op]
+    return method(handler, request, *args, **kwargs)
 
 
 def api_operations(cls):
@@ -311,9 +324,13 @@
         dispatcher.__doc__ = (
             "The actual operation to execute depends on the value of the '%s' "
             "parameter:\n\n" % OP_PARAM)
-        dispatcher.__doc__ += "\n".join(
-            "- Operation '%s' (op=%s):\n\t%s" % (name, name, op.__doc__)
-            for name, op in cls._available_api_methods[method].items())
+        for name, op in cls._available_api_methods[method].items():
+            if name is not None:
+                doc = "- Operation '%s' (op=%s):\n\t%s" % (
+                    name, name, op.__doc__)
+            else:
+                doc = "- Default operation:\n\t%s" % op.__doc__
+            dispatcher.__doc__ += "\n%s" % doc
 
         # Add {'create','read','update','delete'} method.
         setattr(cls, method_name, dispatcher)
@@ -1262,6 +1279,7 @@
         'comment',
         )
 
+    @api_exported('GET', default_operation=True)
     def read(self, request, name):
         """Read a specific Node."""
         return Tag.objects.get_tag_or_404(name=name, user=request.user)
@@ -1285,11 +1303,7 @@
         tag.delete()
         return rc.DELETED
 
-    # XXX: JAM 2012-09-25 This is currently a POST because of bug:
-    #      http://pad.lv/1049933
-    #      Essentially, if you have one 'GET' op, then you can no longer get
-    #      the Tag object itself from a plain 'GET' without op.
-    @api_exported('POST')
+    @api_exported('GET')
     def nodes(self, request, name):
         """Get the list of nodes that have this tag."""
         tag = Tag.objects.get_tag_or_404(name=name, user=request.user)

=== modified file 'src/maasserver/apidoc.py'
--- src/maasserver/apidoc.py	2012-09-13 12:05:54 +0000
+++ src/maasserver/apidoc.py	2012-09-26 09:09:26 +0000
@@ -95,7 +95,8 @@
       restful: Indicates if this is a CRUD/ReSTful action.
 
     """
-    from maasserver.api import dispatch_methods  # Avoid circular imports.
+    # Avoid circular imports.
+    from maasserver.api import dispatch_methods
     operation_methods = getattr(handler, "_available_api_methods", {})
     for http_method in handler.allowed_methods:
         desc_base = dict(method=http_method)
@@ -104,9 +105,18 @@
             # custom operations instead.
             operations = handler._available_api_methods[http_method]
             for op, func in operations.items():
-                yield dict(
-                    desc_base, name=op, doc=getdoc(func),
-                    op=op, restful=False)
+                if op is None:
+                    # This is a 'default' operation (i.e. a CRUD
+                    # operation).
+                    name = dispatch_methods[http_method]
+                    func = getattr(handler, name)
+                    yield dict(
+                        desc_base, name=name, doc=getdoc(func),
+                        op=None, restful=True)
+                else:
+                    yield dict(
+                        desc_base, name=op, doc=getdoc(func),
+                        op=op, restful=False)
         else:
             # Default Piston CRUD method still stands.
             name = dispatch_methods[http_method]

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-25 11:21:25 +0000
+++ src/maasserver/tests/test_api.py	2012-09-26 09:09:26 +0000
@@ -346,8 +346,10 @@
         self.assertEqual('node-aabbccddeeff.local', node.hostname)
 
     def test_POST_fails_without_operation(self):
-        # If there is no operation ('op=operation_name') specified in the
-        # request data, a 'Bad request' response is returned.
+        # If there is no operation ('op=operation_name') specified in
+        # the request data and not default operation defined
+        # (api_exported(..., default_operation=True)) defined on the
+        # handler, a 'Bad request' response is returned.
         response = self.client.post(
             self.get_uri('nodes/'),
             {
@@ -357,7 +359,7 @@
 
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
         self.assertIn('text/html', response['Content-Type'])
-        self.assertEqual("Unknown operation.", response.content)
+        self.assertEqual("Unknown default operation.", response.content)
 
     def test_POST_fails_if_mac_duplicated(self):
         # Mac Addresses should be unique.
@@ -2210,7 +2212,7 @@
         self.assertEqual(tag.definition, parsed_result['definition'])
         self.assertEqual(tag.comment, parsed_result['comment'])
 
-    def test_GET_refuses_to_access_nonexistent_node(self):
+    def test_GET_refuses_to_access_nonexistent_tag(self):
         # When fetching a Tag, the api returns a 'Not Found' (404) error
         # if no tag is found.
         response = self.client.get(self.get_uri('tags/no-such-tag/'))
@@ -2218,8 +2220,8 @@
 
     def test_PUT_refuses_non_superuser(self):
         tag = factory.make_tag()
-        response = self.client.put(self.get_tag_uri(tag),
-                                   {'comment': 'A special comment'})
+        response = self.client.put(
+            self.get_tag_uri(tag), {'comment': 'A special comment'})
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_PUT_invalid_field(self):
@@ -2244,21 +2246,21 @@
         self.assertFalse(Tag.objects.filter(name=tag.name).exists())
         self.assertTrue(Tag.objects.filter(name='new-tag-name').exists())
 
-    def test_POST_nodes_with_no_nodes(self):
+    def test_GET_nodes_with_no_nodes(self):
         tag = factory.make_tag()
-        response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
+        response = self.client.get(self.get_tag_uri(tag), {'op': 'nodes'})
 
         self.assertEqual(httplib.OK, response.status_code)
         parsed_result = json.loads(response.content)
         self.assertEqual([], parsed_result)
 
-    def test_POST_nodes_returns_nodes(self):
+    def test_GET_nodes_returns_nodes(self):
         tag = factory.make_tag()
         node1 = factory.make_node()
         # Create a second node that isn't tagged.
         factory.make_node()
         node1.tags.add(tag)
-        response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
+        response = self.client.get(self.get_tag_uri(tag), {'op': 'nodes'})
 
         self.assertEqual(httplib.OK, response.status_code)
         parsed_result = json.loads(response.content)