Merge lp:~danilo/maas/django-upgrade-immutable-querydict into lp:~maas-committers/maas/trunk

[Данило Шеган]

Replace all in-place modifying of POST data which is immutable in more recent Django versions.

request.data contains QueryDict objects which have become immutable by default.

In most cases, it was sufficient to create a copy of the data on the API side before passing it on to the form processing machinery (or instantiating the form with it).

But for some reason that did not work that well in LicenseKeyForm: I had to modify the internal "_mutable" attribute to be able to edit the data in-place. The problem is how the data is processed there: it wants to allow API to pass in a combined osystem/distro_series in the distro_series field, thus combines those into one in full_clean(), and then clean() splits those up again. Officially, Django only supports overriding clean(), so we are basically playing with internals that we shouldn't be playing with. I am not sure how to best highlight this case. Perhaps an XXX?

This gets us down to 66 test failures with django 1.11 from ppa:danilo/django-1.11 and django-piston3 from ppa:danilo/django-piston3

[Blake Rouse (blake-rouse)]

Looks good overall. Just one comment on the section that should be of no surprise. ;-)

review: Approve

[Данило Шеган (danilo)]

Addressed the comment: after looking some more, I think I can fix this in a nicer way, but I'd rather do that as a separate branch since it requires test changes (which means more risk).

[Blake Rouse (blake-rouse)]

Sounds good. Yeah that is much better.

The clean_osystem and clean_distro_series should not rely on each others filed values. When you need to validate two field together that should take place in the clean method.

review: Approve

[Данило Шеган (danilo)]

This needs to wait for prereq before itcan land.

Follow-up branch up at https://code.launchpad.net/~danilo/maas/django-upgrade-licensekeyform-immutable-querydict/+merge/326079

=== modified file 'src/maasserver/api/boot_resources.py'
--- src/maasserver/api/boot_resources.py	2017-03-30 19:33:21 +0000
+++ src/maasserver/api/boot_resources.py	2017-06-21 15:23:27 +0000
@@ -189,7 +189,7 @@
         # If the user provides no parameters to the create command, then
         # django will treat the form as valid, and so it won't actually
         # validate any of the data.
-        data = request.data
+        data = request.data.copy()
         if data is None:
             data = {}
         if 'filetype' not in data:

=== modified file 'src/maasserver/api/commissioning_scripts.py'
--- src/maasserver/api/commissioning_scripts.py	2017-03-21 19:03:50 +0000
+++ src/maasserver/api/commissioning_scripts.py	2017-06-21 15:23:27 +0000
@@ -80,9 +80,10 @@
             is ignored; MAAS will know it by the name you pass to the request.
         """
         content = Bin(get_content_parameter(request))
-        request.data['script'] = content
-        request.data['script_type'] = SCRIPT_TYPE.COMMISSIONING
-        form = ScriptForm(data=request.data)
+        data = request.data.copy()
+        data['script'] = content
+        data['script_type'] = SCRIPT_TYPE.COMMISSIONING
+        form = ScriptForm(data=data)
         if form.is_valid():
             script = form.save()
             return {
@@ -127,9 +128,10 @@
         """Update a commissioning script."""
         script = get_object_or_404(Script, name=name)
         content = Bin(get_content_parameter(request))
-        request.data['script'] = content
-        request.data['script_type'] = SCRIPT_TYPE.COMMISSIONING
-        form = ScriptForm(instance=script, data=request.data)
+        data = request.data.copy()
+        data['script'] = content
+        data['script_type'] = SCRIPT_TYPE.COMMISSIONING
+        form = ScriptForm(instance=script, data=data)
         if form.is_valid():
             form.save()
             return rc.ALL_OK

=== modified file 'src/maasserver/api/dnsresourcerecords.py'
--- src/maasserver/api/dnsresourcerecords.py	2017-03-29 22:34:35 +0000
+++ src/maasserver/api/dnsresourcerecords.py	2017-06-21 15:23:27 +0000
@@ -93,7 +93,7 @@
         :param rrdata: resource data (everything to the right of
             resource type.)
         """
-        data = request.data
+        data = request.data.copy()
         domain = None
         fqdn = data.get('fqdn', None)
         name = data.get('name', None)
@@ -129,14 +129,14 @@
         # Do we already have a DNSResource for this fqdn?
         dnsrr = DNSResource.objects.filter(name=name, domain__id=domain.id)
         if not dnsrr.exists():
-            form = DNSResourceForm(data=request.data, request=request)
+            form = DNSResourceForm(data=data, request=request)
             if form.is_valid():
                 form.save()
             else:
                 raise MAASAPIValidationError(form.errors)
             dnsrr = DNSResource.objects.filter(name=name, domain__id=domain.id)
         data['dnsresource'] = dnsrr
-        form = DNSDataForm(data=request.data)
+        form = DNSDataForm(data=data)
         if form.is_valid():
             return form.save()
         else:
@@ -183,8 +183,9 @@
         """
         dnsdata = DNSData.objects.get_dnsdata_or_404(
             id, request.user, NODE_PERMISSION.ADMIN)
-        request.data['dnsresource'] = dnsdata.dnsresource.id
-        form = DNSDataForm(instance=dnsdata, data=request.data)
+        data = request.data.copy()
+        data['dnsresource'] = dnsdata.dnsresource.id
+        form = DNSDataForm(instance=dnsdata, data=data)
         if form.is_valid():
             return form.save()
         else:

=== modified file 'src/maasserver/api/dnsresources.py'
--- src/maasserver/api/dnsresources.py	2017-03-29 22:30:31 +0000
+++ src/maasserver/api/dnsresources.py	2017-06-21 15:23:27 +0000
@@ -85,7 +85,7 @@
         :param ip_addresses: (optional) Address (ip or id) to assign to the
             dnsresource.
         """
-        data = request.data
+        data = request.data.copy()
         fqdn = data.get('fqdn', None)
         name = data.get('name', None)
         domainname = data.get('domain', None)
@@ -107,7 +107,7 @@
                     "name:%s" % domainname, user=request.user,
                     perm=NODE_PERMISSION.VIEW)
             data['domain'] = domain.id
-        form = DNSResourceForm(data=request.data, request=request)
+        form = DNSResourceForm(data=data, request=request)
         if form.is_valid():
             return form.save()
         else:

=== modified file 'src/maasserver/api/scripts.py'
--- src/maasserver/api/scripts.py	2017-04-13 06:12:12 +0000
+++ src/maasserver/api/scripts.py	2017-06-21 15:23:27 +0000
@@ -85,13 +85,14 @@
         :param comment: A comment about what this change does.
         :type comment: unicode
         """
+        data = request.data.copy()
         if 'script' in request.FILES:
-            request.data['script'] = request.FILES.get('script').read()
+            data['script'] = request.FILES.get('script').read()
         elif len(request.FILES) == 1:
             for name, script in request.FILES.items():
-                request.data['name'] = name
-                request.data['script'] = script.read()
-        form = ScriptForm(data=request.data)
+                data['name'] = name
+                data['script'] = script.read()
+        form = ScriptForm(data=data)
         if form.is_valid():
             return form.save()
         else:
@@ -266,14 +267,15 @@
         else:
             script = get_object_or_404(Script, name=name)
 
+        data = request.data.copy()
         if 'script' in request.FILES:
-            request.data['script'] = request.FILES.get('script').read()
+            data['script'] = request.FILES.get('script').read()
         elif len(request.FILES) == 1:
             for name, script_content in request.FILES.items():
-                request.data['name'] = name
-                request.data['script'] = script_content.read()
+                data['name'] = name
+                data['script'] = script_content.read()
 
-        form = ScriptForm(instance=script, data=request.data)
+        form = ScriptForm(instance=script, data=data)
         if form.is_valid():
             return form.save()
         else:

=== modified file 'src/maasserver/forms/__init__.py'
--- src/maasserver/forms/__init__.py	2017-06-16 08:37:39 +0000
+++ src/maasserver/forms/__init__.py	2017-06-21 15:23:27 +0000
@@ -2121,12 +2121,21 @@
         # not have to call the api with distro_series=os/series. This occurs
         # in full_clean, so the value is correct before validation occurs on
         # the distro_series field.
+        # XXX Danilo 2017-06-20: with Django 1.11, POST data is a read-only
+        # QueryDict by default.  Unfortunately, per-field validation happens
+        # before clean() is called, and thus it is too late to do this in
+        # clean() where it really belongs.
         if 'distro_series' in self.data and 'osystem' in self.data:
             if '/' not in self.data['distro_series']:
+                if hasattr(self.data, "_mutable"):
+                    was_mutable = self.data._mutable
+                    self.data._mutable = True
                 self.data['distro_series'] = '%s/%s' % (
                     self.data['osystem'],
                     self.data['distro_series'],
                     )
+                if hasattr(self.data, "_mutable"):
+                    self.data._mutable = was_mutable
         super(LicenseKeyForm, self).full_clean()
 
     def clean(self):

=== modified file 'src/maasserver/forms/ephemeral.py'
--- src/maasserver/forms/ephemeral.py	2017-03-29 13:34:55 +0000
+++ src/maasserver/forms/ephemeral.py	2017-06-21 15:23:27 +0000
@@ -47,7 +47,7 @@
             required=False, initial=None, choices=choices)
 
     def __init__(self, instance, user, data=None, **kwargs):
-        data = {} if data is None else data
+        data = {} if data is None else data.copy()
         super().__init__(data=data, **kwargs)
         self._set_up_script_fields()
         self.instance = instance

=== modified file 'src/maasserver/forms/iprange.py'
--- src/maasserver/forms/iprange.py	2017-02-17 14:23:04 +0000
+++ src/maasserver/forms/iprange.py	2017-06-21 15:23:27 +0000
@@ -30,6 +30,8 @@
             self, data=None, instance=None, request=None, *args, **kwargs):
         if data is None:
             data = {}
+        else:
+            data = data.copy()
         # If this is a new IPRange, fill in the 'user' and 'subnet' fields
         # automatically, if necessary.
         if instance is None:

=== modified file 'src/maasserver/forms/script.py'
--- src/maasserver/forms/script.py	2017-04-13 02:22:38 +0000
+++ src/maasserver/forms/script.py	2017-06-21 15:23:27 +0000
@@ -48,30 +48,36 @@
             'script',
         )
 
-    def __init__(self, instance=None, **kwargs):
-        super().__init__(instance=instance, **kwargs)
+    def __init__(self, instance=None, data=None, **kwargs):
         if instance is None:
-            for field in ['name', 'script']:
-                self.fields[field].required = True
             script_data_key = 'data'
         else:
-            for field in ['name', 'script']:
-                self.fields[field].required = False
-            self.fields['script'].initial = instance.script
             script_data_key = 'new_data'
-        if 'comment' in self.data and 'script' in self.data:
+
+        data = data.copy()
+        if 'comment' in data and 'script' in data:
             script_data = {
-                'comment': self.data.get('comment'),
-                script_data_key: self.data.get('script'),
+                'comment': data.get('comment'),
+                script_data_key: data.get('script'),
             }
-            self.data['script'] = script_data
-            self.data.pop('comment')
+            data['script'] = script_data
+            data.pop('comment')
         # Alias type to script_type to allow for consistent naming in the API.
-        if 'type' in self.data and 'script_type' not in self.data:
-            self.data['script_type'] = self.data['type']
+        if 'type' in data and 'script_type' not in data:
+            data['script_type'] = data['type']
             # self.data is a QueryDict. pop returns a list containing the value
             # while directly accessing it returns just the value.
-            self.data.pop('type')
+            data.pop('type')
+
+        super().__init__(instance=instance, data=data, **kwargs)
+
+        if instance is None:
+            for field in ['name', 'script']:
+                self.fields[field].required = True
+        else:
+            for field in ['name', 'script']:
+                self.fields[field].required = False
+            self.fields['script'].initial = instance.script
 
     def clean(self):
         cleaned_data = super().clean()