launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/nicer-mac-address-error into lp:maas.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1062598 in MAAS: "maas-cli raises an error when you pass wrong MAC address filter"
  https://bugs.launchpad.net/maas/+bug/1062598

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/nicer-mac-address-error/+merge/128421
-- 
https://code.launchpad.net/~julian-edwards/maas/nicer-mac-address-error/+merge/128421
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/nicer-mac-address-error into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-05 08:04:58 +0000
+++ src/maasserver/api.py	2012-10-08 05:54:17 +0000
@@ -134,7 +134,7 @@
     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 +762,10 @@
         # Get filters from request.
         match_ids = get_optional_list(request.GET, 'id')
         match_macs = get_optional_list(request.GET, 'mac_address')
+        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 05:54:17 +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