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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 1941
Proposed branch: lp:~jtv/maas/validate-network-nodes-filter
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 129 lines (+68/-8)
3 files modified
src/maasserver/api.py (+6/-7)
src/maasserver/forms.py (+41/-1)
src/maasserver/tests/test_api_networks.py (+21/-0)
To merge this branch: bzr merge lp:~jtv/maas/validate-network-nodes-filter
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+205972@code.launchpad.net

Commit message

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.

Description of the change

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

Jeroen

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

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
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
173from maasserver.forms import (173from maasserver.forms import (
174 BulkNodeActionForm,174 BulkNodeActionForm,
175 DownloadProgressForm,175 DownloadProgressForm,
176 NodeGroupInterfaceForeignDHCPForm,
177 get_action_form,176 get_action_form,
178 get_node_create_form,177 get_node_create_form,
179 get_node_edit_form,178 get_node_edit_form,
180 NetworkForm,179 NetworkForm,
180 NetworksListingForm,
181 NodeActionForm,181 NodeActionForm,
182 NodeGroupEdit,182 NodeGroupEdit,
183 NodeGroupInterfaceForeignDHCPForm,
183 NodeGroupInterfaceForm,184 NodeGroupInterfaceForm,
184 NodeGroupWithInterfacesForm,185 NodeGroupWithInterfacesForm,
185 SSHKeyForm,186 SSHKeyForm,
@@ -2767,12 +2768,10 @@
2767 networks. If more than one node is given, the result will be2768 networks. If more than one node is given, the result will be
2768 restricted to networks that these nodes have in common.2769 restricted to networks that these nodes have in common.
2769 """2770 """
2770 networks = Network.objects.all()2771 form = NetworksListingForm(data=request.GET)
2771 node_ids = get_optional_list(request.GET, 'node')2772 if not form.is_valid():
2772 if node_ids is not None:2773 raise ValidationError(form.errors)
2773 for node in Node.objects.filter(system_id__in=node_ids):2774 return form.filter_networks(Network.objects.all())
2774 networks = networks.filter(node=node)
2775 return networks.order_by('name')
27762775
2777 @admin_method2776 @admin_method
2778 def create(self, request):2777 def create(self, request):
27792778
=== 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 @@
26 "MAASAndNetworkForm",26 "MAASAndNetworkForm",
27 "MACAddressForm",27 "MACAddressForm",
28 "NetworkForm",28 "NetworkForm",
29 "NetworksListingForm",
29 "NodeGroupEdit",30 "NodeGroupEdit",
30 "NodeGroupInterfaceForm",31 "NodeGroupInterfaceForm",
31 "NodeGroupWithInterfacesForm",32 "NodeGroupWithInterfacesForm",
@@ -856,7 +857,6 @@
856 % (networks[index - 1]['name'], networks[index]['name']))857 % (networks[index - 1]['name'], networks[index]['name']))
857858
858859
859# XXX JeroenVermeulen 2014-01-29 bug=1052339: This restriction is going away.
860class NodeGroupWithInterfacesForm(ModelForm):860class NodeGroupWithInterfacesForm(ModelForm):
861 """Create a NodeGroup with unmanaged interfaces."""861 """Create a NodeGroup with unmanaged interfaces."""
862862
@@ -1223,3 +1223,43 @@
1223 'netmask',1223 'netmask',
1224 'vlan_tag',1224 'vlan_tag',
1225 )1225 )
1226
1227
1228class NetworksListingForm(forms.Form):
1229 """Form for the networks listing API."""
1230
1231 # Multi-value parameter, but with a name in the singular. This is going
1232 # to be passed as a GET-style parameter in the URL, so repeated as "node="
1233 # for every node.
1234 node = UnconstrainedMultipleChoiceField(
1235 label="Show only networks that are attached to all of these nodes.",
1236 required=False, error_messages={
1237 'invalid_list':
1238 "Invalid parameter: list of node system IDs required.",
1239 })
1240
1241 def clean_node(self):
1242 system_ids = self.cleaned_data['node']
1243 if system_ids is None:
1244 return None
1245 system_ids = set(system_ids)
1246 nodes = Node.objects.filter(system_id__in=system_ids)
1247 if len(nodes) != len(system_ids):
1248 unknown = system_ids.difference({node.system_id for node in nodes})
1249 raise forms.ValidationError(
1250 "Unknown node(s): %s."
1251 % ', '.join(sorted(unknown)))
1252 return nodes
1253
1254 def filter_networks(self, networks):
1255 """Filter (and order) the given networks by the form's criteria.
1256
1257 :param networks: A query set of :class:`Network`.
1258 :return: A version of `networks` restricted and ordered according to
1259 the criteria passed to the form.
1260 """
1261 nodes = self.cleaned_data.get('node')
1262 if nodes is not None:
1263 for node in nodes:
1264 networks = networks.filter(node=node)
1265 return networks.order_by('name')
12261266
=== 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 @@
117 self.assertEqual(117 self.assertEqual(
118 {networks[2].name},118 {networks[2].name},
119 {network['name'] for network in json.loads(response.content)})119 {network['name'] for network in json.loads(response.content)})
120
121 def test_GET_fails_if_filtering_by_nonexistent_node(self):
122 bad_system_id = factory.make_name('no_node')
123 response = self.client.get(
124 reverse('networks_handler'),
125 {'node': [bad_system_id]})
126 self.assertEqual(httplib.BAD_REQUEST, response.status_code)
127 self.assertEqual(
128 {'node': ["Unknown node(s): %s." % bad_system_id]},
129 json.loads(response.content))
130
131 def test_GET_ignores_duplicates(self):
132 factory.make_network()
133 node = factory.make_node(networks=[factory.make_network()])
134 response = self.client.get(
135 reverse('networks_handler'),
136 {'node': [node.system_id, node.system_id]})
137 self.assertEqual(httplib.OK, response.status_code, response.content)
138 self.assertEqual(
139 {network.name for network in node.networks.all()},
140 {network['name'] for network in json.loads(response.content)})