Merge lp:~jtv/maas/validate-network-nodes-filter into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Handle the API GET request to list networks through a form. This fixes the lack of validation: passing multiple nodes means "networks connected to *all* of these nodes," so quietly ignoring unknown nodes could lead to unwanted results.

Raphaël suggested that more API calls ought to do this, so see it as a prototype for possible repeated use.

Jeroen

[Raphaël Badin (rvb)]

Very nice!

[0]

24 + if nodes is not None:
25 + for node in nodes:
26 networks = networks.filter(node=node)
27 return networks.order_by('name')

Ideally, I think this logic (the filtering and the order_by) belongs to the form (same as the usual call to form.save() when the form is a modification/creation form).

review: Approve

[Jeroen T. Vermeulen (jtv)]

OK, I've moved it. I'm tempted to write unit tests for the form at this point, but without any reuse and the API such a thin wrapper around it, it's really not worth the extra weight.

Thanks for the review!

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-02-12 04:33:17 +0000
+++ src/maasserver/api.py	2014-02-12 15:40:09 +0000
@@ -173,13 +173,14 @@
 from maasserver.forms import (
     BulkNodeActionForm,
     DownloadProgressForm,
-    NodeGroupInterfaceForeignDHCPForm,
     get_action_form,
     get_node_create_form,
     get_node_edit_form,
     NetworkForm,
+    NetworksListingForm,
     NodeActionForm,
     NodeGroupEdit,
+    NodeGroupInterfaceForeignDHCPForm,
     NodeGroupInterfaceForm,
     NodeGroupWithInterfacesForm,
     SSHKeyForm,
@@ -2767,12 +2768,10 @@
             networks.  If more than one node is given, the result will be
             restricted to networks that these nodes have in common.
         """
-        networks = Network.objects.all()
-        node_ids = get_optional_list(request.GET, 'node')
-        if node_ids is not None:
-            for node in Node.objects.filter(system_id__in=node_ids):
-                networks = networks.filter(node=node)
-        return networks.order_by('name')
+        form = NetworksListingForm(data=request.GET)
+        if not form.is_valid():
+            raise ValidationError(form.errors)
+        return form.filter_networks(Network.objects.all())
 
     @admin_method
     def create(self, request):

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-02-12 13:13:51 +0000
+++ src/maasserver/forms.py	2014-02-12 15:40:09 +0000
@@ -26,6 +26,7 @@
     "MAASAndNetworkForm",
     "MACAddressForm",
     "NetworkForm",
+    "NetworksListingForm",
     "NodeGroupEdit",
     "NodeGroupInterfaceForm",
     "NodeGroupWithInterfacesForm",
@@ -856,7 +857,6 @@
                 % (networks[index - 1]['name'], networks[index]['name']))
 
 
-# XXX JeroenVermeulen 2014-01-29 bug=1052339: This restriction is going away.
 class NodeGroupWithInterfacesForm(ModelForm):
     """Create a NodeGroup with unmanaged interfaces."""
 
@@ -1223,3 +1223,43 @@
             'netmask',
             'vlan_tag',
             )
+
+
+class NetworksListingForm(forms.Form):
+    """Form for the networks listing API."""
+
+    # Multi-value parameter, but with a name in the singular.  This is going
+    # to be passed as a GET-style parameter in the URL, so repeated as "node="
+    # for every node.
+    node = UnconstrainedMultipleChoiceField(
+        label="Show only networks that are attached to all of these nodes.",
+        required=False, error_messages={
+            'invalid_list':
+            "Invalid parameter: list of node system IDs required.",
+            })
+
+    def clean_node(self):
+        system_ids = self.cleaned_data['node']
+        if system_ids is None:
+            return None
+        system_ids = set(system_ids)
+        nodes = Node.objects.filter(system_id__in=system_ids)
+        if len(nodes) != len(system_ids):
+            unknown = system_ids.difference({node.system_id for node in nodes})
+            raise forms.ValidationError(
+                "Unknown node(s): %s."
+                % ', '.join(sorted(unknown)))
+        return nodes
+
+    def filter_networks(self, networks):
+        """Filter (and order) the given networks by the form's criteria.
+
+        :param networks: A query set of :class:`Network`.
+        :return: A version of `networks` restricted and ordered according to
+            the criteria passed to the form.
+        """
+        nodes = self.cleaned_data.get('node')
+        if nodes is not None:
+            for node in nodes:
+                networks = networks.filter(node=node)
+        return networks.order_by('name')

=== modified file 'src/maasserver/tests/test_api_networks.py'
--- src/maasserver/tests/test_api_networks.py	2014-02-12 04:26:59 +0000
+++ src/maasserver/tests/test_api_networks.py	2014-02-12 15:40:09 +0000
@@ -117,3 +117,24 @@
         self.assertEqual(
             {networks[2].name},
             {network['name'] for network in json.loads(response.content)})
+
+    def test_GET_fails_if_filtering_by_nonexistent_node(self):
+        bad_system_id = factory.make_name('no_node')
+        response = self.client.get(
+            reverse('networks_handler'),
+            {'node': [bad_system_id]})
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        self.assertEqual(
+            {'node': ["Unknown node(s): %s." % bad_system_id]},
+            json.loads(response.content))
+
+    def test_GET_ignores_duplicates(self):
+        factory.make_network()
+        node = factory.make_node(networks=[factory.make_network()])
+        response = self.client.get(
+            reverse('networks_handler'),
+            {'node': [node.system_id, node.system_id]})
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertEqual(
+            {network.name for network in node.networks.all()},
+            {network['name'] for network in json.loads(response.content)})