Merge lp:~mpontillo/maas/device-api-1566109 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Ensure QueryDict is always used with WithMACAddressesMixin.

 * This is the likely fix for bug #1566109.

[Gavin Panella (allenap)]

There's a problem in WithMACAddressesMixin that illustrates one of my
pet hates.

I want to pass a MultiValueDict into the form's constructor, so that
self.data has a getlist() method. This is used by WithMACAddressesMixin.
However, Django's BaseForm has the following code:

  class BaseForm(object):
      def __init__(self, data=None, ...):
          ...
          self.data = data or {}
          ...

so when I pass an *empty* MultiValueDict in — which is an ancestor of
QueryDict, the container that Django uses to convey request arguments to
handlers, and thus a perfectly reasonable thing to pass into a form —
it's replaced with a plain old Python dict, and subsequent attempts to
use getlist() on it break with AttributeError.

All because someone was too f***ing lazy test against None:

    self.data = {} if data is None else data

My blood boils. Implicit boolean-ness is a curse.

[Gavin Panella (allenap)]

I've got the tests passing in lp:~allenap/maas/device-api-1566109 (diff:
https://paste.ubuntu.com/15789749/). This was far harder than it ought
to have been. It has upgraded my dislike for Django's form machinery
from "moderate" to "strong".

WithMACAddressesMixin expects self.data to be a QueryDict. I haven't
double checked, but it seems that not even a MultiValueDict is enough.

Django's BaseForm works on the assumption that self.data is a plain
dict, so we're already in dodgy territory, because plain dicts and
QueryDicts do not quack quite the same way.

But maybe Django's all okay with this, because its MultipleHiddenInput
and SelectMultiple widgets have the following code:

    def value_from_datadict(self, data, files, name):
        if isinstance(data, (MultiValueDict, MergeDict)):
            return data.getlist(name)
        return data.get(name, None)

I don't know. It's a mess.

[Gavin Panella (allenap)]

Looks good. A few small things to tidy up, nothing more. Also, describe the fix in the commit message; the bug number is automatically added to the commit message by the lander so there's no need to mention it again (or at all).

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/device-api-1566109 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://security.ubuntu.com/ubuntu xenial-security InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease [247 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main Sources [865 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Sources [7,757 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,182 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages [7,541 kB]
Fetched 17.6 MB in 3s (5,067 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bash bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
bind9 is already the newest version (1:9.10.3.dfsg.P4-8).
bind9utils is already the newest version (1:9.10.3.dfsg.P4-8).
build-essential is already the newest version (12.1ubuntu2).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
dnsutils is already the newest version (1:9.10.3.dfsg.P4-8).
firefox is already the newest version (45.0.2+build1-0ubuntu1).
freeipmi-...

[Mike Pontillo (mpontillo)]

Could not reproduce random failure in maasserver.rpc.tests.test_regionservice.

Trying again to land.

review: Approve

=== modified file 'src/maasserver/api/tests/test_devices.py'
--- src/maasserver/api/tests/test_devices.py	2016-04-14 15:25:36 +0000
+++ src/maasserver/api/tests/test_devices.py	2016-04-19 16:54:49 +0000
@@ -133,6 +133,24 @@
         self.assertEquals(domain, device.domain)
         self.assertEqual(device.node_type, NODE_TYPE.DEVICE)
 
+    def test_POST_without_macs_raises_appropriate_error(self):
+        hostname = factory.make_name('host')
+        domain = factory.make_Domain()
+        response = self.client.post(
+            reverse('devices_handler'),
+            {
+                'hostname': hostname,
+                'domain': domain.name,
+            })
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response.content)
+
+    def test_empty_POST_raises_appropriate_error(self):
+        response = self.client.post(
+            reverse('devices_handler'), {})
+        self.assertEqual(
+            http.client.BAD_REQUEST, response.status_code, response.content)
+
     def test_POST_returns_limited_fields(self):
         response = self.client.post(
             reverse('devices_handler'),

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2016-04-15 22:14:33 +0000
+++ src/maasserver/forms.py	2016-04-19 16:54:49 +0000
@@ -55,6 +55,7 @@
     Form,
     MultipleChoiceField,
 )
+from django.http import QueryDict
 from django.utils.safestring import mark_safe
 from lxml import etree
 from maasserver.api.utils import get_overridden_query_dict
@@ -128,6 +129,7 @@
 from maasserver.utils.converters import machine_readable_bytes
 from maasserver.utils.forms import (
     compose_invalid_choice_text,
+    get_QueryDict,
     set_form_error,
 )
 from maasserver.utils.orm import (
@@ -1055,6 +1057,12 @@
 
     def __init__(self, *args, **kwargs):
         super(WithMACAddressesMixin, self).__init__(*args, **kwargs)
+        # This is a workaround for a Django bug in which self.data (which is
+        # supposed to be a QueryDict) ends up being a normal Python dict.
+        # This class requires a QueryDict (which it seems like Django should
+        # enforce, for consistency).
+        if not isinstance(self.data, QueryDict):
+            self.data = get_QueryDict(self.data)
         self.set_up_mac_addresses_field()
 
     def set_up_mac_addresses_field(self):

=== modified file 'src/maasserver/tests/test_forms_device.py'
--- src/maasserver/tests/test_forms_device.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/tests/test_forms_device.py	2016-04-19 16:54:49 +0000
@@ -5,10 +5,27 @@
 
 __all__ = []
 
-from maasserver.forms import DeviceForm
+from django.http import HttpRequest
+from django.http.request import QueryDict
+from maasserver.forms import (
+    DeviceForm,
+    DeviceWithMACsForm,
+)
+from maasserver.models import (
+    Device,
+    Interface,
+)
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maasserver.utils.orm import reload_object
+from maasserver.utils.forms import get_QueryDict
+from maasserver.utils.orm import (
+    get_one,
+    reload_object,
+)
+from testtools.matchers import (
+    Contains,
+    Equals,
+)
 
 
 class TestDeviceForm(MAASServerTestCase):
@@ -39,3 +56,48 @@
         reload_object(parent)
 
         self.assertEqual(parent, device.parent)
+
+
+class TestDeviceWithMACsForm(MAASServerTestCase):
+
+    def make_request(self):
+        """Return a :class:`HttpRequest` with the given parameters."""
+        request = HttpRequest()
+        request.user = factory.make_User()
+        return request
+
+    def test_contains_mac_addresses_field_and_converts_non_querydict(self):
+        form = DeviceWithMACsForm(data={})
+        self.assertThat(form.fields, Contains('mac_addresses'))
+        self.assertIsInstance(form.data, QueryDict)
+
+    def test_creates_device_with_mac(self):
+        hostname = factory.make_name("device")
+        mac = factory.make_mac_address()
+        form = DeviceWithMACsForm(data=get_QueryDict({
+            "hostname": hostname,
+            "mac_addresses": mac
+        }), request=self.make_request())
+        self.assertTrue(form.is_valid(), dict(form.errors))
+        form.save()
+        device = get_one(Device.objects.filter(hostname=hostname))
+        self.assertThat(device.hostname, Equals(hostname))
+        iface = get_one(Interface.objects.filter(mac_address=mac))
+        self.assertThat(iface.node, Equals(device))
+
+    def test_creates_device_with_macs(self):
+        hostname = factory.make_name("device")
+        mac1 = factory.make_mac_address()
+        mac2 = factory.make_mac_address()
+        form = DeviceWithMACsForm(data=get_QueryDict({
+            "hostname": hostname,
+            "mac_addresses": [mac1, mac2]
+        }), request=self.make_request())
+        self.assertTrue(form.is_valid(), dict(form.errors))
+        form.save()
+        device = get_one(Device.objects.filter(hostname=hostname))
+        self.assertThat(device.hostname, Equals(hostname))
+        iface = get_one(Interface.objects.filter(mac_address=mac1))
+        self.assertThat(iface.node, Equals(device))
+        iface = get_one(Interface.objects.filter(mac_address=mac2))
+        self.assertThat(iface.node, Equals(device))

=== modified file 'src/maasserver/utils/forms.py'
--- src/maasserver/utils/forms.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/utils/forms.py	2016-04-19 16:54:49 +0000
@@ -32,9 +32,9 @@
     )
 
 
-def get_QueryDict(params):
+def get_QueryDict(params, mutable=True):
     """Convert `params` to a `QueryDict`."""
-    query_dict = QueryDict('', mutable=True)
+    query_dict = QueryDict('', mutable=mutable)
     for k, v in params.items():
         if isinstance(v, list):
             query_dict.setlist(k, v)