launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06202
[Merge] lp:~rvb/maas/maas-add-node into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-add-node into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-add-node/+merge/90445
This is the first branch of the "add a node" (in javascript) story. Its main objective is to create an api endpoint to create a Node and any number of related MAC Addresses. For that it creates the proper form (NodeWithMACAddressesForm in src/maasserver/forms.py) to take advantage of Django's form validation machinery.
= Details =
The main trick is the creation of the OperationHandlerMetaClass metaclass (src/maasserver/api.py) and the api_exported decorator. This metaclass dynamically creates a method (called 'create') that is hooked up by Django to POST api requests. The method re-routes the request to methods decorated with api_exported based on the value of the parameter 'op' (for operation).
This branch also:
- removes the setting PISTON_IGNORE_DUPE_MODELS. Now, each model class is attached to a single piston handler.
- fixes the resource_uri methods (in NodeMacHandler and NodeHandler). The documentation about this is bloody confusing (https://bitbucket.org/jespern/django-piston/wiki/Documentation) but here is the catch: piston uses them for two different things: a) to generate the uris in the documentation (http://people.canonical.com/~gavin/docs/lp:maas/api.html) and b) to add to each object returned by the api a resource_uri (cf. test src/maasserver/tests/test_api.py test_node_resource_uri), very much like what lp does.
- removes the duplication that existed: the api documentation is available on a maas server instance (api_doc view in src/maasserver/api.py) and is generated by src/maasserver/management/commands/gen_rst_api_doc.py. Now they both use the exact same resource: generate_api_doc (src/maasserver/api.py). The only trick is that I had to import from distutils what's needed to convert the rst generated by piston into html.
--
https://code.launchpad.net/~rvb/maas/maas-add-node/+merge/90445
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-add-node into lp:maas.
=== modified file 'src/maas/settings.py'
--- src/maas/settings.py 2012-01-25 14:24:52 +0000
+++ src/maas/settings.py 2012-01-27 15:00:30 +0000
@@ -197,5 +197,3 @@
},
}
}
-
-PISTON_IGNORE_DUPE_MODELS = True
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-01-24 18:30:38 +0000
+++ src/maasserver/api.py 2012-01-27 15:00:30 +0000
@@ -10,11 +10,14 @@
__metaclass__ = type
__all__ = [
+ "api_doc",
+ "generate_api_doc",
"NodeHandler",
"NodeMacsHandler",
]
from functools import wraps
+import types
from django.core.exceptions import (
PermissionDenied,
@@ -25,13 +28,18 @@
render_to_response,
)
from django.template import RequestContext
+from docutils import core
+from maasserver.forms import NodeWithMACAddressesForm
from maasserver.macaddress import validate_mac
from maasserver.models import (
MACAddress,
Node,
)
from piston.doc import generate_doc
-from piston.handler import BaseHandler
+from piston.handler import (
+ BaseHandler,
+ HandlerMetaClass,
+ )
from piston.utils import rc
@@ -41,9 +49,9 @@
return resp
-def format_error_message(error):
+def format_error_message(error_dict):
messages = []
- for k, v in error.message_dict.iteritems():
+ for k, v in error_dict.iteritems():
if isinstance(v, list):
messages.append("%s: %s" % (k, "".join(v)))
else:
@@ -57,7 +65,7 @@
obj.save()
return obj
except ValidationError, e:
- return bad_request(format_error_message(e))
+ return bad_request(format_error_message(e.message_dict))
def validate_mac_address(mac_address):
@@ -78,6 +86,58 @@
return wraps(view_func)(_decorator)
+def api_exported(operation_name=True):
+ def _decorator(func):
+ if operation_name == 'create':
+ raise Exception("Cannot define a 'create' operation.")
+ func._api_exported = operation_name
+ return func
+ return _decorator
+
+
+# The parameter used to specify the requested operation for POST API calls.
+OP_PARAM = 'op'
+
+
+class OperationHandlerMetaClass(HandlerMetaClass):
+ def __init__(cls, name, bases, dct):
+ super(OperationHandlerMetaClass, cls).__init__(name, bases, dct)
+ # Register all the exported methods.
+ cls._available_api_methods = {}
+ api_docs = ['\n']
+ for name, slot in dct.iteritems():
+ if isinstance(slot, types.FunctionType):
+ if name == 'create':
+ raise Exception("Cannot define a 'create' method.")
+ operation_name = getattr(slot, '_api_exported', None)
+ if operation_name is not None:
+ api_name = (
+ name if operation_name is True else operation_name)
+ cls._available_api_methods[api_name] = slot
+ doc = "Method '%s' (op=%s):\n\t%s" % (
+ api_name, api_name, slot.__doc__)
+ api_docs.append(doc)
+
+ # Define a 'create' method that will route requests to the methods
+ # registered in _available_api_methods.
+ def create(cls, request, *args, **kwargs):
+ op = request.data.get(OP_PARAM, None)
+ if not isinstance(op, unicode):
+ return bad_request('Unknown operation.')
+ elif op not in cls._available_api_methods:
+ return bad_request('Unknown operation: %s.' % op)
+ else:
+ method = cls._available_api_methods[op]
+ return method(cls, request, *args, **kwargs)
+
+ # Update the __doc__ for the 'create' method.
+ doc = "Operate on nodes collection. The actual method to execute\
+ depends on the value of the '%s' parameter." % OP_PARAM
+ create.__doc__ = doc + '\n- '.join(api_docs)
+ # Add the 'create' method.
+ setattr(cls, 'create', create)
+
+
class NodeHandler(BaseHandler):
"""Manage individual Nodes."""
allowed_methods = ('GET', 'DELETE', 'PUT')
@@ -108,27 +168,31 @@
return rc.DELETED
@classmethod
- def resource_uri(cls, *args, **kwargs):
- return ('node_handler', ['system_id'])
+ def resource_uri(cls, node=None):
+ node_system_id = "system_id"
+ if node:
+ node_system_id = node.system_id
+ return ('node_handler', (node_system_id, ))
class NodesHandler(BaseHandler):
"""Manage collection of Nodes / Create Nodes."""
+ __metaclass__ = OperationHandlerMetaClass
allowed_methods = ('GET', 'POST',)
- model = Node
- fields = ('system_id', 'hostname', ('macaddress_set', ('mac_address',)))
def read(self, request):
"""Read all Nodes."""
return Node.objects.get_visible_nodes(request.user).order_by('id')
- def create(self, request):
+ @api_exported('new')
+ def new(self, request):
"""Create a new Node."""
- if 'status' in request.data:
- return bad_request('Cannot set the status for a node.')
-
- node = Node(**dict(request.data.items()))
- return validate_and_save(node)
+ form = NodeWithMACAddressesForm(request.data)
+ if form.is_valid():
+ node = form.save()
+ return node
+ else:
+ return bad_request(format_error_message(form.errors))
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -142,8 +206,6 @@
"""
allowed_methods = ('GET', 'POST',)
- fields = ('mac_address',)
- model = MACAddress
@perm_denied_handler
def read(self, request, system_id):
@@ -161,7 +223,7 @@
mac = node.add_mac_address(request.data.get('mac_address', None))
return mac
except ValidationError, e:
- return bad_request(format_error_message(e))
+ return bad_request(format_error_message(e.message_dict))
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -201,19 +263,49 @@
return rc.DELETED
@classmethod
- def resource_uri(cls, *args, **kwargs):
- return ('node_mac_handler', ['system_id', 'mac_address'])
-
-
-docs = (
- generate_doc(NodesHandler),
- generate_doc(NodeHandler),
- generate_doc(NodeMacsHandler),
- generate_doc(NodeMacHandler),
- )
+ def resource_uri(cls, mac=None):
+ node_system_id = "system_id"
+ mac_address = "mac_address"
+ if mac is not None:
+ node_system_id = mac.node.system_id
+ mac_address = mac.mac_address
+ return ('node_mac_handler', [node_system_id, mac_address])
+
+
+def generate_api_doc():
+ docs = (
+ generate_doc(NodesHandler),
+ generate_doc(NodeHandler),
+ generate_doc(NodeMacsHandler),
+ generate_doc(NodeMacHandler),
+ )
+
+ messages = ['MaaS API\n========\n\n']
+ for doc in docs:
+ for method in doc.get_methods():
+ messages.append(
+ "%s %s\n %s\n\n" % (
+ method.http_name, doc.resource_uri_template,
+ method.doc))
+ return ''.join(messages)
+
+
+def reST_to_html_fragment(a_str):
+ parts = core.publish_parts(source=a_str, writer_name='html')
+ return parts['body_pre_docinfo'] + parts['fragment']
+
+
+_API_DOC = None
def api_doc(request):
+ # 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', {'docs': docs},
+ 'maasserver/api_doc.html',
+ {'doc': reST_to_html_fragment(generate_api_doc())},
context_instance=RequestContext(request))
=== added file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/forms.py 2012-01-27 15:00:30 +0000
@@ -0,0 +1,69 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Forms."""
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ "NodeForm",
+ "MACAddressForm",
+ ]
+
+from django import forms
+from django.forms import ModelForm
+from maasserver.macaddress import MACAddressFormField
+from maasserver.models import (
+ MACAddress,
+ Node,
+ )
+
+
+class NodeForm(ModelForm):
+ system_id = forms.CharField(
+ widget=forms.TextInput(attrs={'readonly': 'readonly'}),
+ required=False)
+
+ class Meta:
+ model = Node
+ fields = ('hostname', 'system_id')
+
+
+class MACAddressForm(ModelForm):
+ class Meta:
+ model = MACAddress
+
+
+class MACAddressForm(ModelForm):
+ class Meta:
+ model = MACAddress
+
+
+class MultiplecMACAddressField(forms.MultiValueField):
+ def __init__(self, nb_macs=1, *args, **kwargs):
+ fields = [MACAddressFormField() for i in xrange(nb_macs)]
+ super(MultiplecMACAddressField, self).__init__(fields, *args, **kwargs)
+
+ def compress(self, data_list):
+ if data_list:
+ return data_list
+ return []
+
+
+class NodeWithMACAddressesForm(NodeForm):
+
+ def __init__(self, *args, **kwargs):
+ super(NodeWithMACAddressesForm, self).__init__(*args, **kwargs)
+ macs = self.data.getlist('macaddresses')
+ self.fields['macaddresses'] = MultiplecMACAddressField(len(macs))
+ self.data['macaddresses'] = macs
+
+ def save(self):
+ node = super(NodeWithMACAddressesForm, self).save()
+ for mac in self.cleaned_data['macaddresses']:
+ node.add_mac_address(mac)
+ return node
=== modified file 'src/maasserver/macaddress.py'
--- src/maasserver/macaddress.py 2012-01-24 12:44:48 +0000
+++ src/maasserver/macaddress.py 2012-01-27 15:00:30 +0000
@@ -9,20 +9,33 @@
)
__metaclass__ = type
-__all__ = []
+__all__ = [
+ "MACAddressField",
+ "MACAddressFormField",
+ ]
import re
from django.core.validators import RegexValidator
from django.db.models import Field
+from django.forms import RegexField
import psycopg2.extensions
mac_re = re.compile(r'^([0-9a-fA-F]{2}[:-]){5}[0-9a-fA-F]{2}$')
-validate_mac = RegexValidator(
- regex=mac_re,
- message=u"Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF).")
+
+mac_error_msg = u"Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)."
+
+
+validate_mac = RegexValidator(regex=mac_re, message=mac_error_msg)
+
+
+class MACAddressFormField(RegexField):
+
+ def __init__(self, *args, **kwargs):
+ super(MACAddressFormField, self).__init__(
+ regex=mac_re, error_message=mac_error_msg, *args, **kwargs)
class MACAddressField(Field):
=== modified file 'src/maasserver/management/commands/gen_rst_api_doc.py'
--- src/maasserver/management/commands/gen_rst_api_doc.py 2012-01-19 15:42:18 +0000
+++ src/maasserver/management/commands/gen_rst_api_doc.py 2012-01-27 15:00:30 +0000
@@ -1,15 +1,7 @@
from django.core.management.base import BaseCommand
-from maasserver.api import docs
+from maasserver.api import generate_api_doc
class Command(BaseCommand):
-
def handle(self, *args, **options):
- messages = ['MaaS API\n========\n\n']
- for doc in docs:
- for method in doc.get_methods():
- messages.append(
- "%s %s\n %s\n\n" % (
- method.http_name, doc.resource_uri_template,
- method.doc))
- self.stdout.write(''.join(messages))
+ self.stdout.write(generate_api_doc())
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-01-24 18:30:38 +0000
+++ src/maasserver/models.py 2012-01-27 15:00:30 +0000
@@ -10,6 +10,7 @@
__metaclass__ = type
__all__ = [
+ "generate_node_system_id",
"NODE_STATUS",
"Node",
"MACAddress",
@@ -105,8 +106,8 @@
class Node(CommonInfo):
"""A `Node` represents a physical machine used by the MaaS Server."""
system_id = models.CharField(
- max_length=41, unique=True, editable=False,
- default=generate_node_system_id)
+ max_length=41, unique=True, default=generate_node_system_id,
+ editable=False)
hostname = models.CharField(max_length=255, default='', blank=True)
status = models.IntegerField(
max_length=10, choices=NODE_STATUS_CHOICES, editable=False,
@@ -144,7 +145,7 @@
<http://en.wikipedia.org/wiki/MAC_address>`_ attached to a `Node`.
"""
mac_address = MACAddressField()
- node = models.ForeignKey(Node)
+ node = models.ForeignKey(Node, editable=False)
class Meta:
verbose_name_plural = "MAC addresses"
=== modified file 'src/maasserver/templates/maasserver/api_doc.html'
--- src/maasserver/templates/maasserver/api_doc.html 2012-01-19 16:42:25 +0000
+++ src/maasserver/templates/maasserver/api_doc.html 2012-01-27 15:00:30 +0000
@@ -1,21 +1,8 @@
{% extends "maasserver/base.html" %}
{% block content %}
- <h2>MaaS Server API Documentation</h2>
- <table>
- <thead>
- <tr><td>Resource</td><td>Description</td></tr>
- </thead>
- <tbody>
- {% for doc in docs %}
- {% for method in doc.get_methods %}
- <tr>
- <td>{{ method.http_name }} {{ doc.resource_uri_template }}</td>
- <td>{{ method.doc }}</td>
- </tr>
- {% endfor %}
- {% endfor %}
- </tbody>
- </table>
+ {% autoescape on %}
+ {{ doc|safe }}
+ {% endautoescape %}
{% endblock %}
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-01-25 14:16:04 +0000
+++ src/maasserver/tests/test_api.py 2012-01-27 15:00:30 +0000
@@ -105,16 +105,41 @@
def test_nodes_POST(self):
"""
- The api allows to create a Node.
+ The api allows to create a Node associated with MAC Addresses.
"""
response = self.client.post(
- '/api/nodes/', {'hostname': 'diane'})
+ '/api/nodes/',
+ {
+ 'op': 'new',
+ 'hostname': 'diane',
+ 'macaddresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff']
+ })
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
self.assertEqual('diane', parsed_result['hostname'])
+ self.assertEqual(41, len(parsed_result.get('system_id')))
self.assertEqual(1, Node.objects.filter(hostname='diane').count())
+ node = Node.objects.get(hostname='diane')
+ self.assertSequenceEqual(
+ ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ [mac.mac_address for mac in node.macaddress_set.all()])
+
+ def test_nodes_POST_invalid(self):
+ """
+ If the data provided to create a node with MAC Addresse is invalid,
+ a 'Bad request' response is returned.
+
+ """
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'hostname': 'diane',
+ 'macaddresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
+ })
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
def test_node_PUT(self):
"""
@@ -131,6 +156,21 @@
self.assertEqual(0, Node.objects.filter(hostname='diane').count())
self.assertEqual(1, Node.objects.filter(hostname='francis').count())
+ def test_node_resource_uri(self):
+ """
+ When a Node is returned by the api, the field 'resource_uri' provides
+ the uri for this Node.
+
+ """
+ node = factory.make_node(hostname='diane')
+ response = self.client.put(
+ '/api/nodes/%s/' % node.system_id, {'hostname': 'francis'})
+ parsed_result = json.loads(response.content)
+
+ self.assertEqual(
+ '/api/nodes/%s/' % (parsed_result['system_id']),
+ parsed_result['resource_uri'])
+
def test_node_PUT_invalid(self):
"""
If the data provided to update a node is invalid, a 'Bad request'
@@ -165,19 +205,6 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_nodes_POST_set_status_invalid(self):
- """
- The status of a newly created Node cannot be set (if will default to
- 0 (new)).
-
- """
- response = self.client.post(
- '/api/nodes/', {'status': 'new'})
-
- self.assertEqual(400, response.status_code)
- self.assertEqual(
- 'Bad Request: Cannot set the status for a node.', response.content)
-
def test_node_DELETE_non_visible_node(self):
"""
The request to delete a single node is denied if the node isn't visible
=== added file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_forms.py 2012-01-27 15:00:30 +0000
@@ -0,0 +1,57 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test forms."""
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+from django.http import QueryDict
+from maasserver.forms import NodeWithMACAddressesForm
+from maasserver.testing import TestCase
+
+
+class NodeWithMACAddressesFormTest(TestCase):
+
+ def get_QueryDict(self, params):
+ query_dict = QueryDict('', mutable=True)
+ for k, v in params.items():
+ if isinstance(v, list):
+ query_dict.setlist(k, v)
+ else:
+ query_dict[k] = v
+ return query_dict
+
+ def test_NodeWithMACAddressesForm_valid(self):
+
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'macaddresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
+
+ self.assertTrue(form.is_valid())
+ self.assertEqual(
+ ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ form.cleaned_data['macaddresses'])
+
+ def test_NodeWithMACAddressesForm_invalid(self):
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'macaddresses': ['aa:bb:cc:dd:ee:ff', 'z_invalid']}))
+
+ self.assertFalse(form.is_valid())
+
+ def test_NodeWithMACAddressesForm_save(self):
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'macaddresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
+ node = form.save()
+
+ self.assertIsNotNone(node.id) # The node is persisted.
+ self.assertSequenceEqual(
+ ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ [mac.mac_address for mac in node.macaddress_set.all()])
