launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12532
[Merge] lp:~allenap/maas/default-crud-rename-api-exported into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/default-crud-rename-api-exported into lp:maas with lp:~allenap/maas/default-crud as a prerequisite.
Commit message:
Renames the api_exported decorator to operation, and demands a declaration of idempotence instead of an HTTP method.
Other bits of the new dispatch code is also cleaned up.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1049933 in MAAS: "The public-facing API code confusingly conflates CRUD, HTTP methods, and piggybacked operations"
https://bugs.launchpad.net/maas/+bug/1049933
For more details, see:
https://code.launchpad.net/~allenap/maas/default-crud-rename-api-exported/+merge/126575
--
https://code.launchpad.net/~allenap/maas/default-crud-rename-api-exported/+merge/126575
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/default-crud-rename-api-exported into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-09-26 23:50:29 +0000
+++ src/maasserver/api.py 2012-09-26 23:50:29 +0000
@@ -203,21 +203,21 @@
return actor, anonymous
-def api_exported(method='POST', exported_as=None):
+def operation(idempotent, exported_as=None):
"""Decorator to make a method available on the API.
- :param method: The HTTP method over which to export the operation.
+ :param idempotent: If this operation is idempotent. Idempotent operations
+ are made available via HTTP GET, non-idempotent operations via HTTP
+ POST.
:param exported_as: Optional operation name; defaults to the name of the
exported method.
-
"""
+ method = "GET" if idempotent else "POST"
def _decorator(func):
- if method not in OperationsResource.callmap:
- raise ValueError("Invalid method: '%s'" % method)
if exported_as is None:
- func._api_exported = {method: func.__name__}
+ func.export = method, func.__name__
else:
- func._api_exported = {method: exported_as}
+ func.export = method, exported_as
return func
return _decorator
@@ -238,30 +238,24 @@
cls = super(OperationsHandlerType, metaclass).__new__(
metaclass, name, bases, namespace)
- # Create an http-method:function mapping for CRUD operations.
+ # Create a signature:function mapping for CRUD operations.
crud = {
- http_method: getattr(cls, method)
+ (http_method, None): getattr(cls, method)
for http_method, method in OperationsResource.crudmap.items()
if getattr(cls, method, None) is not None
}
- # Create a operation-name:function mapping for non-CRUD operations.
- # These functions contain an _api_exported attribute that will be
- # used later on.
+ # Create a signature:function mapping for non-CRUD operations.
operations = {
- name: attribute for name, attribute in vars(cls).items()
- if getattr(attribute, "_api_exported", None) is not None
+ attribute.export: attribute
+ for attribute in vars(cls).values()
+ if getattr(attribute, "export", None) is not None
}
# Create the exports mapping.
exports = {}
- exports.update(
- ((http_method, None), function)
- for http_method, function in crud.items())
- exports.update(
- (signature, function)
- for name, function in operations.items()
- for signature in function._api_exported.items())
+ exports.update(crud)
+ exports.update(operations)
# Update the class.
cls.exports = exports
@@ -491,7 +485,7 @@
node_system_id = node.system_id
return ('node_handler', (node_system_id, ))
- @api_exported('POST')
+ @operation(idempotent=False)
def stop(self, request, system_id):
"""Shut down a node."""
nodes = Node.objects.stop_nodes([system_id], request.user)
@@ -500,7 +494,7 @@
"You are not allowed to shut down this node.")
return nodes[0]
- @api_exported('POST')
+ @operation(idempotent=False)
def start(self, request, system_id):
"""Power up a node.
@@ -532,7 +526,7 @@
"You are not allowed to start up this node.")
return nodes[0]
- @api_exported('POST')
+ @operation(idempotent=False)
def release(self, request, system_id):
"""Release a node. Opposite of `NodesHandler.acquire`."""
node = Node.objects.get_node_or_404(
@@ -574,7 +568,7 @@
create = read = update = delete = None
fields = DISPLAYED_NODE_FIELDS
- @api_exported('POST')
+ @operation(idempotent=False)
def new(self, request):
"""Create a new Node.
@@ -585,7 +579,7 @@
"""
return create_node(request)
- @api_exported('GET')
+ @operation(idempotent=True)
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).
@@ -600,12 +594,12 @@
mac_address=mac_address).exclude(
node__status=NODE_STATUS.RETIRED).exists()
- @api_exported('POST')
+ @operation(idempotent=False)
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("POST")
+ @operation(idempotent=False)
def check_commissioning(self, request):
"""Check all commissioning nodes to see if they are taking too long.
@@ -644,7 +638,7 @@
create = read = update = delete = None
anonymous = AnonNodesHandler
- @api_exported('POST')
+ @operation(idempotent=False)
def new(self, request):
"""Create a new Node.
@@ -656,7 +650,7 @@
node.accept_enlistment(request.user)
return node
- @api_exported('POST')
+ @operation(idempotent=False)
def accept(self, request):
"""Accept declared nodes into the MAAS.
@@ -694,7 +688,7 @@
return filter(
None, [node.accept_enlistment(request.user) for node in nodes])
- @api_exported('POST')
+ @operation(idempotent=False)
def accept_all(self, request):
"""Accept all declared nodes into the MAAS.
@@ -713,7 +707,7 @@
nodes = [node.accept_enlistment(request.user) for node in nodes]
return filter(None, nodes)
- @api_exported('GET')
+ @operation(idempotent=True)
def list(self, request):
"""List Nodes visible to the user, optionally filtered by criteria.
@@ -734,7 +728,7 @@
nodes = nodes.filter(macaddress__mac_address__in=match_macs)
return nodes.order_by('id')
- @api_exported('GET')
+ @operation(idempotent=True)
def list_allocated(self, request):
"""Fetch Nodes that were allocated to the User/oauth token."""
token = get_oauth_token(request)
@@ -742,7 +736,7 @@
nodes = Node.objects.get_allocated_visible_nodes(token, match_ids)
return nodes.order_by('id')
- @api_exported('POST')
+ @operation(idempotent=False)
def acquire(self, request):
"""Acquire an available node for deployment."""
node = Node.objects.get_available_node_for_acquisition(
@@ -850,7 +844,7 @@
"""
create = read = update = delete = None
- get = api_exported('GET', exported_as='get')(get_file)
+ get = operation(idempotent=True, exported_as='get')(get_file)
class FilesHandler(OperationsHandler):
@@ -858,9 +852,9 @@
create = read = update = delete = None
anonymous = AnonFilesHandler
- get = api_exported('GET', exported_as='get')(get_file)
+ get = operation(idempotent=True, exported_as='get')(get_file)
- @api_exported('POST')
+ @operation(idempotent=False)
def add(self, request):
"""Add a new file to the file storage.
@@ -898,7 +892,7 @@
create = read = update = delete = None
fields = DISPLAYED_NODEGROUP_FIELDS
- @api_exported('GET')
+ @operation(idempotent=True)
def list(self, request):
"""List of node groups."""
return NodeGroup.objects.all()
@@ -907,7 +901,7 @@
def resource_uri(cls):
return ('nodegroups_handler', [])
- @api_exported('POST')
+ @operation(idempotent=False)
def refresh_workers(self, request):
"""Request an update of all node groups' configurations.
@@ -921,7 +915,7 @@
NodeGroup.objects.refresh_workers()
return HttpResponse("Sending worker refresh.", status=httplib.OK)
- @api_exported('POST')
+ @operation(idempotent=False)
def register(self, request):
"""Register a new `NodeGroup`.
@@ -983,12 +977,12 @@
create = read = update = delete = None
fields = DISPLAYED_NODEGROUP_FIELDS
- @api_exported('GET')
+ @operation(idempotent=True)
def list(self, request):
"""List of node groups."""
return NodeGroup.objects.all()
- @api_exported('POST')
+ @operation(idempotent=False)
def accept(self, request):
"""Accept nodegroup enlistment(s).
@@ -1006,7 +1000,7 @@
else:
raise PermissionDenied("That method is reserved to admin users.")
- @api_exported('POST')
+ @operation(idempotent=False)
def reject(self, request):
"""Reject nodegroup enlistment(s).
@@ -1064,7 +1058,7 @@
uuid = nodegroup.uuid
return ('nodegroup_handler', [uuid])
- @api_exported('POST')
+ @operation(idempotent=False)
def update_leases(self, request, uuid):
leases = get_mandatory_param(request.data, 'leases')
nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
@@ -1087,13 +1081,13 @@
create = read = update = delete = None
fields = DISPLAYED_NODEGROUP_FIELDS
- @api_exported('GET')
+ @operation(idempotent=True)
def list(self, request, uuid):
"""List of NodeGroupInterfaces of a NodeGroup."""
nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
return NodeGroupInterface.objects.filter(nodegroup=nodegroup)
- @api_exported('POST')
+ @operation(idempotent=False)
def new(self, request, uuid):
"""Create a new NodeGroupInterface for this NodeGroup.
@@ -1191,7 +1185,7 @@
"""Manage the current logged-in user."""
create = read = update = delete = None
- @api_exported('POST')
+ @operation(idempotent=False)
def create_authorisation_token(self, request):
"""Create an authorisation OAuth token and OAuth consumer.
@@ -1209,7 +1203,7 @@
'consumer_key': consumer.key,
}
- @api_exported('POST')
+ @operation(idempotent=False)
def delete_authorisation_token(self, request):
"""Delete an authorisation OAuth token and the related OAuth consumer.
@@ -1273,7 +1267,7 @@
# 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')
+ @operation(idempotent=False)
def nodes(self, request, name):
"""Get the list of nodes that have this tag."""
return Tag.objects.get_nodes(name, user=request.user)
@@ -1291,13 +1285,13 @@
"""Manage collection of Tags."""
create = read = update = delete = None
- @api_exported('POST')
+ @operation(idempotent=False)
def new(self, request):
"""Create a new `Tag`.
"""
return create_tag(request)
- @api_exported('GET')
+ @operation(idempotent=True)
def list(self, request):
"""List Tags.
"""
@@ -1333,7 +1327,7 @@
"""Manage the MAAS' itself."""
create = read = update = delete = None
- @api_exported('POST')
+ @operation(idempotent=False)
def set_config(self, request):
"""Set a config value.
@@ -1348,7 +1342,7 @@
Config.objects.set_config(name, value)
return rc.ALL_OK
- @api_exported('GET')
+ @operation(idempotent=True)
def get_config(self, request):
"""Get a config value.
@@ -1501,7 +1495,7 @@
def resource_uri(cls):
return ('boot_images_handler', [])
- @api_exported('POST')
+ @operation(idempotent=False)
def report_boot_images(self, request):
"""Report images available to net-boot nodes from.
=== modified file 'src/maasserver/tests/test_api_mechanism.py'
--- src/maasserver/tests/test_api_mechanism.py 2012-09-26 23:50:29 +0000
+++ src/maasserver/tests/test_api_mechanism.py 2012-09-26 23:50:29 +0000
@@ -14,44 +14,51 @@
__metaclass__ = type
__all__ = []
-from maasserver.api import api_exported
+from maasserver.api import operation
from maasserver.testing.factory import factory
from maastesting.testcase import TestCase
-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 = factory.make_name('method', sep='')
- decorate = api_exported(random_method)
- self.assertRaises(ValueError, decorate, lambda: None)
+class TestOperationDecorator(TestCase):
+ """Testing for the `operation` decorator."""
def test_valid_decoration(self):
value = "value" + factory.getRandomString()
- decorate = api_exported()
+ decorate = operation(idempotent=False)
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 = factory.make_name("exportedas", sep='')
- decorate = api_exported("POST", exported_as=random_exported_name)
+ def test_can_passexported_as(self):
+ # Test that passing the optional "exported_as" works as expected.
+ randomexported_name = factory.make_name("exportedas", sep='')
+ decorate = operation(
+ idempotent=False, exported_as=randomexported_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
+ self.assertEqual(randomexported_name, decorated.export[1])
+
+ def testexported_as_is_optional(self):
+ # If exported_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")
+ decorate = operation(idempotent=True)
decorated = decorate(exported_function)
-
- self.assertEqual("exported_function", decorated._api_exported["POST"])
+ self.assertEqual("exported_function", decorated.export[1])
+
+ def test_idempotent_uses_GET(self):
+ # If a function is declared as idempotent the export signature
+ # includes the HTTP GET method.
+ func = lambda: None
+ self.assertEqual(
+ ("GET", func.__name__),
+ operation(idempotent=True)(func).export)
+
+ def test_non_idempotent_uses_POST(self):
+ # If a function is declared as not idempotent the export signature
+ # includes the HTTP POST method.
+ func = lambda: None
+ self.assertEqual(
+ ("POST", func.__name__),
+ operation(idempotent=False)(func).export)
=== modified file 'src/maasserver/tests/test_apidoc.py'
--- src/maasserver/tests/test_apidoc.py 2012-09-26 23:50:29 +0000
+++ src/maasserver/tests/test_apidoc.py 2012-09-26 23:50:29 +0000
@@ -16,7 +16,7 @@
from django.conf import settings
from maasserver.api import (
- api_exported,
+ operation,
OperationsHandler,
)
from maasserver.apidoc import (
@@ -150,11 +150,11 @@
create = read = delete = None
- @api_exported("POST")
+ @operation(idempotent=False)
def peace_sells_but_whos_buying(self, request, vic, rattlehead):
"""Released 1986."""
- @api_exported("GET")
+ @operation(idempotent=True)
def so_far_so_good_so_what(self, request, vic, rattlehead):
"""Released 1988."""
=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py 2012-09-26 23:50:29 +0000
+++ src/metadataserver/api.py 2012-09-26 23:50:29 +0000
@@ -23,9 +23,9 @@
from django.http import HttpResponse
from django.shortcuts import get_object_or_404
from maasserver.api import (
- api_exported,
extract_oauth_key,
get_mandatory_param,
+ operation,
OperationsHandler,
)
from maasserver.enum import (
@@ -184,7 +184,7 @@
contents = raw_content.decode('utf-8')
NodeCommissionResult.objects.store_data(node, name, contents)
- @api_exported('POST')
+ @operation(idempotent=False)
def signal(self, request, version=None, mac=None):
"""Signal commissioning status.
@@ -235,7 +235,7 @@
return rc.ALL_OK
- @api_exported('POST')
+ @operation(idempotent=False)
def netboot_off(self, request, version=None, mac=None):
"""Turn off netboot on the node.
@@ -246,7 +246,7 @@
node.set_netboot(False)
return rc.ALL_OK
- @api_exported('POST')
+ @operation(idempotent=False)
def netboot_on(self, request, version=None, mac=None):
"""Turn on netboot on the node."""
node = get_queried_node(request, for_mac=mac)
@@ -374,18 +374,18 @@
class AnonMetaDataHandler(VersionIndexHandler):
"""Anonymous metadata."""
- @api_exported('GET')
+ @operation(idempotent=True)
def get_enlist_preseed(self, request, version=None):
"""Render and return a preseed script for enlistment."""
return HttpResponse(get_enlist_preseed(), mimetype="text/plain")
- @api_exported('GET')
+ @operation(idempotent=True)
def get_preseed(self, request, version=None, system_id=None):
"""Render and return a preseed script for the given node."""
node = get_object_or_404(Node, system_id=system_id)
return HttpResponse(get_preseed(node), mimetype="text/plain")
- @api_exported('POST')
+ @operation(idempotent=False)
def netboot_off(self, request, version=None, system_id=None):
"""Turn off netboot on the node.
