Merge lp:~julian-edwards/maas/nicer-mac-address-error into lp:~maas-committers/maas/trunk

[Julian Edwards]

Present a nice error rather than a stack trace when bad MAC addresses are detected in the API call to Nodes/list().

[John A Meinel (jameinel)]

I thought Validation error took a dict of {'variable': 'message'} so the exception should be:
ValueError({'macs': "Invalid MAC address(es): %s" % ", ".join(invalid_macs)})

Otherwise, good.

review: Approve

[Jeroen T. Vermeulen (jtv)]

I like how the test case reproduces the ":ZZ" error from the bug report.

review: Approve

[Julian Edwards (julian-edwards)]

<bigjools> jam: thanks for the review - not sure what difference your
suggestion makes as everything seems to work anyway. I think it's more a UI
validation thing isn't it?
<jam> bigjools: I think if you get a form with multiple errors, the 'dict'
method allows it to present both to users.
<jam> I could be 100% wrong, though.
<jam> I was just following along what other things did when I wrote a form
recently.
<bigjools> heh, I honestly have no idea. Given this was an API call I figured
a human readable string would be for the best but....
<jam> bigjools: so the big win is to just not traceback, the rest is gravy
<jam> If it works for you, that's enough for me for now.
<bigjools> righto, cheers

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-quantal/151/console reported an error when processing this lp:~julian-edwards/maas/nicer-mac-address-error branch.
Not merging it.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-05 08:04:58 +0000
+++ src/maasserver/api.py	2012-10-08 07:06:22 +0000
@@ -134,7 +134,10 @@
     NodeStateViolation,
     Unauthorized,
     )
-from maasserver.fields import validate_mac
+from maasserver.fields import (
+    mac_re,
+    validate_mac,
+    )
 from maasserver.forms import (
     get_node_create_form,
     get_node_edit_form,
@@ -762,6 +765,12 @@
         # Get filters from request.
         match_ids = get_optional_list(request.GET, 'id')
         match_macs = get_optional_list(request.GET, 'mac_address')
+        if match_macs is not None:
+            invalid_macs = [
+                mac for mac in match_macs if mac_re.match(mac) is None]
+            if len(invalid_macs) != 0:
+                raise ValidationError(
+                    "Invalid MAC address(es): %s" % ", ".join(invalid_macs))
         # Fetch nodes and apply filters.
         nodes = Node.objects.get_nodes(
             request.user, NODE_PERMISSION.VIEW, ids=match_ids)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-05 08:04:58 +0000
+++ src/maasserver/tests/test_api.py	2012-10-08 07:06:22 +0000
@@ -1616,6 +1616,26 @@
         self.assertItemsEqual(
             [matching_system_id], extract_system_ids(parsed_result))
 
+    def test_GET_list_with_invalid_macs_returns_sensible_error(self):
+        # If specifying an invalid MAC, make sure the error that's
+        # returned is not a crazy stack trace, but something nice to
+        # humans.
+        bad_mac1 = '00:E0:81:DD:D1:ZZ'  # ZZ is bad.
+        bad_mac2 = '00:E0:81:DD:D1:XX'  # XX is bad.
+        ok_mac = factory.make_mac_address()
+        response = self.client.get(self.get_uri('nodes/'), {
+            'op': 'list',
+            'mac_address': [bad_mac1, bad_mac2, ok_mac],
+            })
+        observed = response.status_code, response.content
+        expected = (
+            Equals(httplib.BAD_REQUEST),
+            Contains(
+                "Invalid MAC address(es): 00:E0:81:DD:D1:ZZ, "
+                "00:E0:81:DD:D1:XX"),
+            )
+        self.assertThat(observed, MatchesListwise(expected))
+
     def test_GET_list_allocated_returns_only_allocated_with_user_token(self):
         # If the user's allocated nodes have different session tokens,
         # list_allocated should only return the nodes that have the