launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11820
[Merge] lp:~allenap/maas/inspect-api into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/inspect-api into lp:maas.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~allenap/maas/inspect-api/+merge/123853
This branch has given me headaches.
The API code seems labyrinthine, but I know there's history there, and
an awful lot of this was done for 12.04, under extreme time pressure.
In other words, please don't take any of the following personally.
- It's basically CRUD with resource methods (i.e. RPC with a resource
as context). Sounds simple...
- However, if a CRUD method has a custom operation attached to it (via
the @api_operations decorator), the underlying functionality is no
longer accessible. For example, @api_operations("POST") will prevent
use C(reate), even though the operation might be unrelated to
creating things.
- There's terminology confusion. I think this might bubble up from
Piston:
HTTP methods: GET POST PUT DELETE
correspond to:
Instance methods: read create update delete
However, @api_operations needs an *HTTP* method when decorating an
*instance* method, and the handler's allowed_methods property takes
a list of HTTP methods too.
- It's odd to create methods on a handler that aren't actually useful
because allowed_methods doesn't correspond to one of them. For
example, define a "create" method but omit "POST" from
allowed_methods.
- The code to add custom operations - api_exported, is_api_exported,
perform_api_operation, api_operations - on top of Piston is
byzantine (...and I wrote a fair bit of it). There's a lot of
complex code there to do something which, on the face of it, should
be simple.
- Things like get_mandatory_param and get_optional_list are used
instead of declaring the API's interface.
- Going further: the only definition of the API is wound up in its
implementation.
- There's little distinction between the API's controllers and views
(if we think of MVC), so that the API cannot be exposed to another
medium (e.g. a message bus).
--
https://code.launchpad.net/~allenap/maas/inspect-api/+merge/123853
Your team MAAS Maintainers is requested to review the proposed merge of lp:~allenap/maas/inspect-api into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-09-10 14:42:52 +0000
+++ src/maasserver/api.py 2012-09-11 22:45:40 +0000
@@ -54,19 +54,19 @@
__metaclass__ = type
__all__ = [
+ "AccountHandler",
+ "AnonNodesHandler",
"api_doc",
"api_doc_title",
- "generate_api_doc",
+ "FilesHandler",
"get_oauth_token",
- "AccountHandler",
- "AnonNodesHandler",
- "FilesHandler",
"NodeGroupsHandler",
"NodeHandler",
- "NodesHandler",
"NodeMacHandler",
"NodeMacsHandler",
+ "NodesHandler",
"pxeconfig",
+ "render_api_docs",
]
from base64 import b64decode
@@ -99,6 +99,11 @@
from docutils import core
from formencode import validators
from formencode.validators import Invalid
+from maasserver.apidoc import (
+ describe_handler,
+ find_api_handlers,
+ generate_api_docs,
+ )
from maasserver.enum import (
ARCHITECTURE,
NODE_PERMISSION,
@@ -130,11 +135,9 @@
)
from maasserver.server_address import get_maas_facing_server_address
from maasserver.utils.orm import get_one
-from piston.doc import generate_doc
from piston.handler import (
AnonymousBaseHandler,
BaseHandler,
- HandlerMetaClass,
)
from piston.models import Token
from piston.resource import Resource
@@ -1015,7 +1018,7 @@
# Title section for the API documentation. Matches in style, format,
-# etc. whatever generate_api_doc() produces, so that you can concatenate
+# etc. whatever render_api_docs() produces, so that you can concatenate
# the two.
api_doc_title = dedent("""
========
@@ -1024,8 +1027,8 @@
""".lstrip('\n'))
-def generate_api_doc():
- """Generate ReST documentation for the REST API.
+def render_api_docs():
+ """Render ReST documentation for the REST API.
This module's docstring forms the head of the documentation; details of
the API methods follow.
@@ -1033,24 +1036,7 @@
:return: Documentation, in ReST, for the API.
:rtype: :class:`unicode`
"""
-
- # Fetch all the API Handlers (objects with the class
- # HandlerMetaClass).
module = sys.modules[__name__]
-
- all = [getattr(module, name) for name in module.__all__]
- handlers = [obj for obj in all if isinstance(obj, HandlerMetaClass)]
-
- # Make sure each handler defines a 'resource_uri' method (this is
- # easily forgotten and essential to have a proper documentation).
- for handler in handlers:
- sentinel = object()
- resource_uri = getattr(handler, "resource_uri", sentinel)
- assert resource_uri is not sentinel, "Missing resource_uri in %s" % (
- handler.__name__)
-
- docs = [generate_doc(handler) for handler in handlers]
-
messages = [
__doc__.strip(),
'',
@@ -1059,7 +1045,8 @@
'----------',
'',
]
- for doc in docs:
+ handlers = find_api_handlers(module)
+ for doc in generate_api_docs(handlers):
for method in doc.get_methods():
messages.append(
"%s %s\n %s\n" % (
@@ -1073,20 +1060,14 @@
return parts['body_pre_docinfo'] + parts['fragment']
-_API_DOC = None
-
-
def api_doc(request):
"""Get ReST documentation for the REST API."""
# Generate the documentation and keep it cached. Note that we can't do
# that at the module level because the API doc generation needs Django
# fully initialized.
- global _API_DOC
- if _API_DOC is None:
- _API_DOC = generate_api_doc()
return render_to_response(
'maasserver/api_doc.html',
- {'doc': reST_to_html_fragment(generate_api_doc())},
+ {'doc': reST_to_html_fragment(render_api_docs())},
context_instance=RequestContext(request))
@@ -1154,3 +1135,21 @@
return HttpResponse(
json.dumps(params._asdict()),
content_type="application/json")
+
+
+def describe(request):
+ """Return a description of the whole MAAS API.
+
+ Returns a JSON object describing the whole MAAS API.
+ """
+ module = sys.modules[__name__]
+ description = {
+ "doc": "MAAS API",
+ "handlers": [
+ describe_handler(handler)
+ for handler in find_api_handlers(module)
+ ],
+ }
+ return HttpResponse(
+ json.dumps(description),
+ content_type="application/json")
=== added file 'src/maasserver/apidoc.py'
--- src/maasserver/apidoc.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/apidoc.py 2012-09-11 22:45:40 +0000
@@ -0,0 +1,159 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""..."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ "describe_handler",
+ "find_api_handlers",
+ "generate_api_docs",
+ ]
+
+from inspect import (
+ getargspec,
+ getdoc,
+ )
+from itertools import (
+ chain,
+ izip,
+ repeat,
+ )
+from types import MethodType as instancemethod
+from urllib import quote
+
+from piston.doc import generate_doc
+from piston.handler import HandlerMetaClass
+
+
+def find_api_handlers(module):
+ """Find the API handlers defined in `module`.
+
+ Handlers are of type :class:`HandlerMetaClass`.
+
+ :rtype: Generator, yielding handlers.
+ """
+ try:
+ names = module.__all__
+ except AttributeError:
+ names = sorted(
+ name for name in dir(module)
+ if not name.startswith("_"))
+ for name in names:
+ candidate = getattr(module, name)
+ if isinstance(candidate, HandlerMetaClass):
+ yield candidate
+
+
+def generate_api_docs(handlers):
+ """Generate ReST documentation objects for the ReST API.
+
+ Yields Piston Documentation objects describing the current registered
+ handlers.
+
+ This also ensures that handlers define 'resource_uri' methods. This is
+ easily forgotten and essential in order to generate proper documentation.
+
+ :rtype: :class:`...`
+ """
+ sentinel = object()
+ for handler in handlers:
+ if getattr(handler, "resource_uri", sentinel) is sentinel:
+ raise AssertionError(
+ "Missing resource_uri in %s" % handler.__name__)
+ yield generate_doc(handler)
+
+
+def describe_args(args, defaults):
+ """Generate serialisable descriptions of arguments."""
+ # The end of the defaults list aligns with the end of the args list, hence
+ # we pad it with undefined, in order that we can zip it with args later.
+ undefined = object()
+ if defaults is None:
+ defaults = repeat(undefined)
+ else:
+ defaults = chain(
+ repeat(undefined, len(args) - len(defaults)),
+ defaults)
+ for arg, default in izip(args, defaults):
+ if default is undefined:
+ yield {"name": arg}
+ else:
+ yield {"name": arg, "default": default}
+
+
+def describe_function_args(func):
+ """Generate serialisable descriptions of function arguments.
+
+ When describing the API, we ignore varargs and keywords args.
+ """
+ args, _, _, defaults = getargspec(func)
+ # Trim the first arg (self or cls) if it's uninteresting.
+ if isinstance(func, (instancemethod, classmethod)):
+ args = args[1:]
+ return describe_args(args, defaults)
+
+
+def describe_function(method, func):
+ """Return a serialisable description of a handler function."""
+ return {"doc": getdoc(func), "method": method}
+
+
+def describe_operation(method, func, op):
+ """Return a serialisable description of a handler operation."""
+ description = describe_function(method, func)
+ description["args"] = list(describe_function_args(func))
+ description["op"] = op
+ return description
+
+
+def describe_handler(handler):
+ """Return a serialisable description of a handler.
+
+ :type handler: :class:`BaseHandler` instance that has been decorated by
+ `api_operations`.
+ """
+ # Avoid circular imports.
+ from maasserver.api import dispatch_methods
+
+ uri_template = generate_doc(handler).resource_uri_template
+ if uri_template is None:
+ uri_template = ""
+
+ uri_params = handler.resource_uri()
+ assert len(uri_params) <= 2 or uri_params[2] == {}, (
+ "Resource URIs with keyword parameters are not yet supported.")
+ uri_params = uri_params[1] if len(uri_params) >= 2 else []
+
+ actions = []
+ operation_methods = getattr(handler, "_available_api_methods", {})
+ for http_method in handler.allowed_methods:
+ if http_method in operation_methods:
+ # Default Piston CRUD method has been overridden; inspect
+ # custom operations instead.
+ operations = handler._available_api_methods[http_method]
+ for op, func in operations.items():
+ desc = describe_operation(http_method, func, op)
+ desc["uri"] = "%s?op=%s" % (uri_template, quote(desc["op"]))
+ desc["uri_params"] = uri_params
+ actions.append(desc)
+ else:
+ # Default Piston CRUD method still stands.
+ op = dispatch_methods[http_method]
+ func = getattr(handler, op)
+ desc = describe_function(http_method, func)
+ desc["uri"] = uri_template
+ desc["uri_params"] = uri_params
+ actions.append(desc)
+
+ return {
+ "name": handler.__name__,
+ "doc": getdoc(handler),
+ "actions": actions,
+ }
=== modified file 'src/maasserver/management/commands/generate_api_doc.py'
--- src/maasserver/management/commands/generate_api_doc.py 2012-04-17 13:48:09 +0000
+++ src/maasserver/management/commands/generate_api_doc.py 2012-09-11 22:45:40 +0000
@@ -17,10 +17,10 @@
from django.core.management.base import BaseCommand
from maasserver.api import (
api_doc_title,
- generate_api_doc,
+ render_api_docs,
)
class Command(BaseCommand):
def handle(self, *args, **options):
- self.stdout.write('\n'.join([api_doc_title, generate_api_doc()]))
+ self.stdout.write('\n'.join([api_doc_title, render_api_docs()]))
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-09-10 14:27:00 +0000
+++ src/maasserver/tests/test_api.py 2012-09-11 22:45:40 +0000
@@ -2593,3 +2593,31 @@
self.assertEqual(
httplib.FORBIDDEN, response.status_code,
explain_unexpected_response(httplib.FORBIDDEN, response))
+
+
+class TestDescribe(AnonAPITestCase):
+
+ def get_describe(self):
+ """Make a request to `describe`, and return its response dict."""
+ response = self.client.get(reverse('describe'))
+ return json.loads(response.content)
+
+ def test_describe_returns_json(self):
+ response = self.client.get(reverse('describe'))
+ self.assertThat(
+ (response.status_code,
+ response['Content-Type'],
+ response.content,
+ response.content),
+ MatchesListwise(
+ (Equals(httplib.OK),
+ Equals("application/json"),
+ StartsWith(b'{'),
+ Contains('name'))),
+ response)
+
+ def test_describe(self):
+ response = self.client.get(reverse('describe'))
+ description = json.loads(response.content)
+ self.assertSetEqual({"doc", "handlers"}, set(description))
+ self.assertIsInstance(description["handlers"], list)
=== added file 'src/maasserver/tests/test_apidoc.py'
--- src/maasserver/tests/test_apidoc.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_apidoc.py 2012-09-11 22:45:40 +0000
@@ -0,0 +1,213 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test maasserver API documentation functionality."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+import new
+
+from maasserver.api import (
+ api_exported,
+ api_operations,
+ )
+from maasserver.apidoc import (
+ describe_args,
+ describe_handler,
+ find_api_handlers,
+ generate_api_docs,
+ )
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from piston.doc import HandlerDocumentation
+from piston.handler import BaseHandler
+
+
+class TestFindingHandlers(TestCase):
+ """Tests for API inspection support: finding handlers."""
+
+ @staticmethod
+ def make_module():
+ """Return a new module with a fabricated name."""
+ name = factory.make_name("module").encode("ascii")
+ return new.module(name)
+
+ def test_empty_module(self):
+ # No handlers are found in empty modules.
+ module = self.make_module()
+ module.__all__ = []
+ self.assertSequenceEqual(
+ [], list(find_api_handlers(module)))
+
+ def test_empty_module_without_all(self):
+ # The absence of __all__ does not matter.
+ module = self.make_module()
+ self.assertSequenceEqual(
+ [], list(find_api_handlers(module)))
+
+ def test_ignore_non_handlers(self):
+ # Module properties that are not handlers are ignored.
+ module = self.make_module()
+ module.something = 123
+ self.assertSequenceEqual(
+ [], list(find_api_handlers(module)))
+
+ def test_module_with_handler(self):
+ # Handlers are discovered in a module and returned.
+ module = self.make_module()
+ module.handler = BaseHandler
+ self.assertSequenceEqual(
+ [BaseHandler], list(find_api_handlers(module)))
+
+ def test_module_with_handler_not_in_all(self):
+ # When __all__ is defined, only the names it defines are searched for
+ # handlers.
+ module = self.make_module()
+ module.handler = BaseHandler
+ module.something = "abc"
+ module.__all__ = ["something"]
+ self.assertSequenceEqual(
+ [], list(find_api_handlers(module)))
+
+
+class TestGeneratingDocs(TestCase):
+ """Tests for API inspection support: generating docs."""
+
+ @staticmethod
+ def make_handler():
+ """
+ Return a new `BaseHandler` subclass with a fabricated name and a
+ `resource_uri` class-method.
+ """
+ name = factory.make_name("handler").encode("ascii")
+ resource_uri = lambda cls: factory.make_name("resource-uri")
+ namespace = {"resource_uri": classmethod(resource_uri)}
+ return type(name, (BaseHandler,), namespace)
+
+ def test_generates_doc_for_handler(self):
+ # generate_api_docs() yields HandlerDocumentation objects for the
+ # handlers passed in.
+ handler = self.make_handler()
+ docs = list(generate_api_docs([handler]))
+ self.assertEqual(1, len(docs))
+ [doc] = docs
+ self.assertIsInstance(doc, HandlerDocumentation)
+ self.assertIs(handler, doc.handler)
+
+ def test_generates_doc_for_multiple_handlers(self):
+ # generate_api_docs() yields HandlerDocumentation objects for the
+ # handlers passed in.
+ handlers = [self.make_handler() for _ in range(5)]
+ docs = list(generate_api_docs(handlers))
+ self.assertEqual(len(handlers), len(docs))
+ self.assertEqual(handlers, [doc.handler for doc in docs])
+
+ def test_handler_without_resource_uri(self):
+ # generate_api_docs() raises an exception if a handler does not have a
+ # resource_uri attribute.
+ handler = self.make_handler()
+ del handler.resource_uri
+ docs = generate_api_docs([handler])
+ error = self.assertRaises(AssertionError, list, docs)
+ self.assertEqual(
+ "Missing resource_uri in %s" % handler.__name__,
+ unicode(error))
+
+
+class TestDescribingAPI(TestCase):
+ """Tests for functions that describe a Piston API."""
+
+ def test_describe_args(self):
+ self.assertEqual(
+ [{"name": "alice"}],
+ list(describe_args(("alice",), ())))
+ self.assertEqual(
+ [{"name": "alice"}, {"name": "bob"}],
+ list(describe_args(("alice", "bob"), ())))
+ self.assertEqual(
+ [{"name": "alice"}, {"name": "bob", "default": "carol"}],
+ list(describe_args(("alice", "bob"), ("carol",))))
+
+ def test_describe_handler(self):
+ # describe_handler() returns a description of a handler that can be
+ # readily serialised into JSON, for example.
+
+ @api_operations
+ class MegadethHandler(BaseHandler):
+ """The mighty 'deth."""
+
+ allowed_methods = "GET", "POST", "PUT"
+
+ @api_exported("POST")
+ def peace_sells(but, whos, buying):
+ """Released 1986."""
+
+ @api_exported("GET")
+ def so_far(so_good, so_what):
+ """Released 1988."""
+
+ @classmethod
+ def resource_uri(self):
+ return ("megadeth_view", ["vic", "rattlehead"])
+
+ expected_actions = [
+ {"args": [
+ {"name": "so_good"},
+ {"name": "so_what"},
+ ],
+ "doc": "Released 1988.",
+ "method": "GET",
+ "op": "so_far",
+ "uri": "?op=so_far",
+ "uri_params": ["vic", "rattlehead"]},
+ {"args": [
+ {"name": "but"},
+ {"name": "whos"},
+ {"name": "buying"},
+ ],
+ "doc": "Released 1986.",
+ "method": "POST",
+ "op": "peace_sells",
+ "uri": "?op=peace_sells",
+ "uri_params": ["vic", "rattlehead"]},
+ {"doc": None,
+ "method": "PUT",
+ "uri": "",
+ "uri_params": ["vic", "rattlehead"]},
+ ]
+
+ observed = describe_handler(MegadethHandler)
+ # The description contains `uri` and `actions` entries.
+ self.assertSetEqual({"actions", "doc", "name"}, set(observed))
+ self.assertEqual(MegadethHandler.__name__, observed["name"])
+ self.assertEqual(MegadethHandler.__doc__, observed["doc"])
+ self.assertSequenceEqual(expected_actions, observed["actions"])
+
+ def test_describe_handler_with_maas_handler(self):
+ # Ensure that describe_handler() yields something sensible with a
+ # "real" MAAS API handler.
+ from maasserver.api import NodeHandler as handler
+ description = describe_handler(handler)
+ # The RUD of CRUD actions are still available, but the C(reate) action
+ # has been overridden with custom operations. Not entirely sure how
+ # this makes sense, but there we go :)
+ expected_actions = {
+ "DELETE /api/1.0/nodes/{system_id}/",
+ "GET /api/1.0/nodes/{system_id}/",
+ "POST /api/1.0/nodes/{system_id}/?op=release",
+ "POST /api/1.0/nodes/{system_id}/?op=start",
+ "POST /api/1.0/nodes/{system_id}/?op=stop",
+ "PUT /api/1.0/nodes/{system_id}/",
+ }
+ observed_actions = {
+ "%(method)s %(uri)s" % action
+ for action in description["actions"]
+ }
+ self.assertSetEqual(expected_actions, observed_actions)
=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py 2012-09-10 14:27:00 +0000
+++ src/maasserver/urls_api.py 2012-09-11 22:45:40 +0000
@@ -20,6 +20,7 @@
AccountHandler,
AdminRestrictedResource,
api_doc,
+ describe,
FilesHandler,
MAASHandler,
NodeGroupHandler,
@@ -57,6 +58,7 @@
# API URLs accessible to anonymous users.
urlpatterns = patterns('',
url(r'doc/$', api_doc, name='api-doc'),
+ url(r'describe/$', describe, name='describe'),
url(r'nodegroups/$', nodegroups_handler, name='nodegroups_handler'),
url(r'pxeconfig/$', pxeconfig, name='pxeconfig'),
)
Follow ups
