launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06209
[Merge] lp:~rvb/maas/maas-add-node-api into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-add-node-api into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-add-node-api/+merge/90838
This branch changes the api so that it returns a json version of the validation errors when the validation fails (previously, the errors where transformed into a simple string). This will allow the caller to deal with validation errors properly. Additionally, the validation logic of the mac_addresses field in NodeWithMACAddressesForm has been changed: if the number of MAC Address fields is > 1, a validation failure will result in a single message. This is more coherent with the fact that the multipe MAC Address field is treated as a single field.
Drive by fixes:
- change the name of the MAC Addresses multi field in NodeWithMACAddressesForm from macaddresses to mac_addresses (this is more consistent with the naming of the mac_address field in MACAddress).
- ignore empty MAC Addresses that might be present in the MAC Addresses multi field in NodeWithMACAddressesForm.
- self.data = self.data.copy() is required because real-life QueryDicts provided by Django are read-only.
--
https://code.launchpad.net/~rvb/maas/maas-add-node-api/+merge/90838
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-add-node-api into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-01-30 09:03:17 +0000
+++ src/maasserver/api.py 2012-01-31 09:20:28 +0000
@@ -23,6 +23,7 @@
PermissionDenied,
ValidationError,
)
+from django.http import HttpResponseBadRequest
from django.shortcuts import (
get_object_or_404,
render_to_response,
@@ -40,29 +41,13 @@
from piston.utils import rc
-def bad_request(message):
- resp = rc.BAD_REQUEST
- resp.write(': %s' % message)
- return resp
-
-
-def format_error_message(error_dict):
- messages = []
- for k, v in error_dict.iteritems():
- if isinstance(v, list):
- messages.append("%s: %s" % (k, "".join(v)))
- else:
- messages.append("%s: %s" % (k, v))
- return "Invalid input: " + " / ".join(messages)
-
-
def validate_and_save(obj):
try:
obj.full_clean()
obj.save()
return obj
except ValidationError, e:
- return bad_request(format_error_message(e.message_dict))
+ return HttpResponseBadRequest(e.message_dict)
def validate_mac_address(mac_address):
@@ -70,7 +55,7 @@
validate_mac(mac_address)
return True, None
except ValidationError:
- return False, bad_request('Invalid MAC Address.')
+ return False, HttpResponseBadRequest('Invalid MAC Address.')
def perm_denied_handler(view_func):
@@ -109,9 +94,9 @@
def perform_api_operation(handler, request, *args, **kwargs):
op = request.data.get(OP_PARAM, None)
if not isinstance(op, unicode):
- return bad_request('Unknown operation.')
+ return HttpResponseBadRequest("Unknown operation.")
elif op not in handler._available_api_methods:
- return bad_request('Unknown operation: %s.' % op)
+ return HttpResponseBadRequest("Unknown operation: '%s'." % op)
else:
method = handler._available_api_methods[op]
return method(handler, request, *args, **kwargs)
@@ -222,7 +207,7 @@
node = form.save()
return node
else:
- return bad_request(format_error_message(form.errors))
+ return HttpResponseBadRequest(form.errors)
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -253,7 +238,7 @@
mac = node.add_mac_address(request.data.get('mac_address', None))
return mac
except ValidationError, e:
- return bad_request(format_error_message(e.message_dict))
+ return HttpResponseBadRequest(e.message_dict)
@classmethod
def resource_uri(cls, *args, **kwargs):
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-01-27 18:06:49 +0000
+++ src/maasserver/forms.py 2012-01-31 09:20:28 +0000
@@ -58,12 +58,23 @@
def __init__(self, *args, **kwargs):
super(NodeWithMACAddressesForm, self).__init__(*args, **kwargs)
- macs = self.data.getlist('macaddresses')
- self.fields['macaddresses'] = MultipleMACAddressField(len(macs))
- self.data['macaddresses'] = macs
+ macs = [mac for mac in self.data.getlist('mac_addresses') if mac]
+ self._nb_macs = len(macs)
+ self.fields['mac_addresses'] = MultipleMACAddressField(len(macs))
+ self.data = self.data.copy()
+ self.data['mac_addresses'] = macs
+
+ def is_valid(self):
+ valid = super(NodeWithMACAddressesForm, self).is_valid()
+ # If the number of MAC Address fields is > 1, provide a unified
+ # error message if the validation has failed.
+ if not valid and self._nb_macs > 1:
+ self.errors['mac_addresses'] = (
+ ['At least one of the MAC Addresses is invalid.'])
+ return valid
def save(self):
node = super(NodeWithMACAddressesForm, self).save()
- for mac in self.cleaned_data['macaddresses']:
+ for mac in self.cleaned_data['mac_addresses']:
node.add_mac_address(mac)
return node
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-01-27 18:06:49 +0000
+++ src/maasserver/tests/test_api.py 2012-01-31 09:20:28 +0000
@@ -113,7 +113,7 @@
{
'op': 'new',
'hostname': 'diane',
- 'macaddresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff']
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff']
})
parsed_result = json.loads(response.content)
@@ -126,6 +126,40 @@
['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_no_operation(self):
+ """
+ If there is no operation ('op=operation_name') specified in the
+ request data, a 'Bad request' response is returned.
+
+ """
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'hostname': 'diane',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
+ })
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertEqual('Unknown operation.', response.content)
+
+ def test_nodes_POST_bad_operation(self):
+ """
+ If the operation ('op=operation_name') specified in the
+ request data is unknown, a 'Bad request' response is returned.
+
+ """
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'op': 'invalid_operation',
+ 'hostname': 'diane',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
+ })
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertEqual(
+ "Unknown operation: 'invalid_operation'.", response.content)
+
def test_nodes_POST_invalid(self):
"""
If the data provided to create a node with MAC Addresse is invalid,
@@ -135,11 +169,17 @@
response = self.client.post(
'/api/nodes/',
{
+ 'op': 'new',
'hostname': 'diane',
- 'macaddresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
})
+ parsed_result = json.loads(response.content)
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertEqual(['mac_addresses'], parsed_result.keys())
+ self.assertEqual(
+ ["At least one of the MAC Addresses is invalid."],
+ parsed_result['mac_addresses'])
def test_node_PUT(self):
"""
@@ -343,11 +383,13 @@
response = self.client.post(
'/api/nodes/%s/macs/' % self.node.system_id,
{'mac_address': 'invalid-mac'})
+ parsed_result = json.loads(response.content)
self.assertEqual(400, response.status_code)
+ self.assertEqual(['mac_address'], parsed_result.keys())
self.assertEqual(
- 'Bad Request: Invalid input: mac_address: Enter a valid MAC '
- 'address (e.g. AA:BB:CC:DD:EE:FF).', response.content)
+ ["Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF)."],
+ parsed_result['mac_address'])
def test_macs_DELETE_mac(self):
"""
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-01-27 14:30:12 +0000
+++ src/maasserver/tests/test_forms.py 2012-01-31 09:20:28 +0000
@@ -31,24 +31,53 @@
form = NodeWithMACAddressesForm(
self.get_QueryDict(
- {'macaddresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
+ {'mac_addresses': ['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())
+ form.cleaned_data['mac_addresses'])
+
+ def test_NodeWithMACAddressesForm_simple_invalid(self):
+ # If the form only has one (invalid) MAC Address field to validate,
+ # the error message in form.errors['mac_addresses'] is the
+ # message from the field's validation error.
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'mac_addresses': ['invalid']}))
+
+ self.assertFalse(form.is_valid())
+ self.assertEqual(['mac_addresses'], form.errors.keys())
+ self.assertEqual(
+ ['Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF).'],
+ form.errors['mac_addresses'])
+
+ def test_NodeWithMACAddressesForm_multiple_invalid(self):
+ # If the form has multiple MAC Address fields to validate,
+ # if one or more fields are invalid, a single error message is
+ # present in form.errors['mac_addresses'] after validation.
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'mac_addresses': ['invalid_1', 'invalid_2']}))
+
+ self.assertFalse(form.is_valid())
+ self.assertEqual(['mac_addresses'], form.errors.keys())
+ self.assertEqual(
+ ['At least one of the MAC Addresses is invalid.'],
+ form.errors['mac_addresses'])
+
+ def test_NodeWithMACAddressesForm_empty(self):
+ # Empty values in the list of MAC Adresses are simply ignored.
+ form = NodeWithMACAddressesForm(
+ self.get_QueryDict(
+ {'mac_addresses': ['aa:bb:cc:dd:ee:ff', '']}))
+
+ self.assertTrue(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']}))
+ {'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']}))
node = form.save()
self.assertIsNotNone(node.id) # The node is persisted.
