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

Proposed by Данило Шеган on 2017-06-20
Status: Merged
Approved by: Данило Шеган on 2017-06-21
Approved revision: 6120
Merged at revision: 6101
Proposed branch: lp:~danilo/maas/django-upgrade-immutable-querydict
Merge into: lp:maas/trunk
Prerequisite: lp:~danilo/maas/django-upgrade-legacy-migrations
Diff against target: 259 lines (+60/-38)
9 files modified
src/maasserver/api/boot_resources.py (+1/-1)
src/maasserver/api/commissioning_scripts.py (+8/-6)
src/maasserver/api/dnsresourcerecords.py (+6/-5)
src/maasserver/api/dnsresources.py (+2/-2)
src/maasserver/api/scripts.py (+10/-8)
src/maasserver/forms/__init__.py (+9/-0)
src/maasserver/forms/ephemeral.py (+1/-1)
src/maasserver/forms/iprange.py (+2/-0)
src/maasserver/forms/script.py (+21/-15)
To merge this branch: bzr merge lp:~danilo/maas/django-upgrade-immutable-querydict
Reviewer Review Type Date Requested Status
Blake Rouse 2017-06-20 Approve on 2017-06-20
Review via email: mp+325995@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

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

review: Approve
Данило Шеган (danilo) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/boot_resources.py'
2--- src/maasserver/api/boot_resources.py 2017-03-30 19:33:21 +0000
3+++ src/maasserver/api/boot_resources.py 2017-06-21 15:23:27 +0000
4@@ -189,7 +189,7 @@
5 # If the user provides no parameters to the create command, then
6 # django will treat the form as valid, and so it won't actually
7 # validate any of the data.
8- data = request.data
9+ data = request.data.copy()
10 if data is None:
11 data = {}
12 if 'filetype' not in data:
13
14=== modified file 'src/maasserver/api/commissioning_scripts.py'
15--- src/maasserver/api/commissioning_scripts.py 2017-03-21 19:03:50 +0000
16+++ src/maasserver/api/commissioning_scripts.py 2017-06-21 15:23:27 +0000
17@@ -80,9 +80,10 @@
18 is ignored; MAAS will know it by the name you pass to the request.
19 """
20 content = Bin(get_content_parameter(request))
21- request.data['script'] = content
22- request.data['script_type'] = SCRIPT_TYPE.COMMISSIONING
23- form = ScriptForm(data=request.data)
24+ data = request.data.copy()
25+ data['script'] = content
26+ data['script_type'] = SCRIPT_TYPE.COMMISSIONING
27+ form = ScriptForm(data=data)
28 if form.is_valid():
29 script = form.save()
30 return {
31@@ -127,9 +128,10 @@
32 """Update a commissioning script."""
33 script = get_object_or_404(Script, name=name)
34 content = Bin(get_content_parameter(request))
35- request.data['script'] = content
36- request.data['script_type'] = SCRIPT_TYPE.COMMISSIONING
37- form = ScriptForm(instance=script, data=request.data)
38+ data = request.data.copy()
39+ data['script'] = content
40+ data['script_type'] = SCRIPT_TYPE.COMMISSIONING
41+ form = ScriptForm(instance=script, data=data)
42 if form.is_valid():
43 form.save()
44 return rc.ALL_OK
45
46=== modified file 'src/maasserver/api/dnsresourcerecords.py'
47--- src/maasserver/api/dnsresourcerecords.py 2017-03-29 22:34:35 +0000
48+++ src/maasserver/api/dnsresourcerecords.py 2017-06-21 15:23:27 +0000
49@@ -93,7 +93,7 @@
50 :param rrdata: resource data (everything to the right of
51 resource type.)
52 """
53- data = request.data
54+ data = request.data.copy()
55 domain = None
56 fqdn = data.get('fqdn', None)
57 name = data.get('name', None)
58@@ -129,14 +129,14 @@
59 # Do we already have a DNSResource for this fqdn?
60 dnsrr = DNSResource.objects.filter(name=name, domain__id=domain.id)
61 if not dnsrr.exists():
62- form = DNSResourceForm(data=request.data, request=request)
63+ form = DNSResourceForm(data=data, request=request)
64 if form.is_valid():
65 form.save()
66 else:
67 raise MAASAPIValidationError(form.errors)
68 dnsrr = DNSResource.objects.filter(name=name, domain__id=domain.id)
69 data['dnsresource'] = dnsrr
70- form = DNSDataForm(data=request.data)
71+ form = DNSDataForm(data=data)
72 if form.is_valid():
73 return form.save()
74 else:
75@@ -183,8 +183,9 @@
76 """
77 dnsdata = DNSData.objects.get_dnsdata_or_404(
78 id, request.user, NODE_PERMISSION.ADMIN)
79- request.data['dnsresource'] = dnsdata.dnsresource.id
80- form = DNSDataForm(instance=dnsdata, data=request.data)
81+ data = request.data.copy()
82+ data['dnsresource'] = dnsdata.dnsresource.id
83+ form = DNSDataForm(instance=dnsdata, data=data)
84 if form.is_valid():
85 return form.save()
86 else:
87
88=== modified file 'src/maasserver/api/dnsresources.py'
89--- src/maasserver/api/dnsresources.py 2017-03-29 22:30:31 +0000
90+++ src/maasserver/api/dnsresources.py 2017-06-21 15:23:27 +0000
91@@ -85,7 +85,7 @@
92 :param ip_addresses: (optional) Address (ip or id) to assign to the
93 dnsresource.
94 """
95- data = request.data
96+ data = request.data.copy()
97 fqdn = data.get('fqdn', None)
98 name = data.get('name', None)
99 domainname = data.get('domain', None)
100@@ -107,7 +107,7 @@
101 "name:%s" % domainname, user=request.user,
102 perm=NODE_PERMISSION.VIEW)
103 data['domain'] = domain.id
104- form = DNSResourceForm(data=request.data, request=request)
105+ form = DNSResourceForm(data=data, request=request)
106 if form.is_valid():
107 return form.save()
108 else:
109
110=== modified file 'src/maasserver/api/scripts.py'
111--- src/maasserver/api/scripts.py 2017-04-13 06:12:12 +0000
112+++ src/maasserver/api/scripts.py 2017-06-21 15:23:27 +0000
113@@ -85,13 +85,14 @@
114 :param comment: A comment about what this change does.
115 :type comment: unicode
116 """
117+ data = request.data.copy()
118 if 'script' in request.FILES:
119- request.data['script'] = request.FILES.get('script').read()
120+ data['script'] = request.FILES.get('script').read()
121 elif len(request.FILES) == 1:
122 for name, script in request.FILES.items():
123- request.data['name'] = name
124- request.data['script'] = script.read()
125- form = ScriptForm(data=request.data)
126+ data['name'] = name
127+ data['script'] = script.read()
128+ form = ScriptForm(data=data)
129 if form.is_valid():
130 return form.save()
131 else:
132@@ -266,14 +267,15 @@
133 else:
134 script = get_object_or_404(Script, name=name)
135
136+ data = request.data.copy()
137 if 'script' in request.FILES:
138- request.data['script'] = request.FILES.get('script').read()
139+ data['script'] = request.FILES.get('script').read()
140 elif len(request.FILES) == 1:
141 for name, script_content in request.FILES.items():
142- request.data['name'] = name
143- request.data['script'] = script_content.read()
144+ data['name'] = name
145+ data['script'] = script_content.read()
146
147- form = ScriptForm(instance=script, data=request.data)
148+ form = ScriptForm(instance=script, data=data)
149 if form.is_valid():
150 return form.save()
151 else:
152
153=== modified file 'src/maasserver/forms/__init__.py'
154--- src/maasserver/forms/__init__.py 2017-06-16 08:37:39 +0000
155+++ src/maasserver/forms/__init__.py 2017-06-21 15:23:27 +0000
156@@ -2121,12 +2121,21 @@
157 # not have to call the api with distro_series=os/series. This occurs
158 # in full_clean, so the value is correct before validation occurs on
159 # the distro_series field.
160+ # XXX Danilo 2017-06-20: with Django 1.11, POST data is a read-only
161+ # QueryDict by default. Unfortunately, per-field validation happens
162+ # before clean() is called, and thus it is too late to do this in
163+ # clean() where it really belongs.
164 if 'distro_series' in self.data and 'osystem' in self.data:
165 if '/' not in self.data['distro_series']:
166+ if hasattr(self.data, "_mutable"):
167+ was_mutable = self.data._mutable
168+ self.data._mutable = True
169 self.data['distro_series'] = '%s/%s' % (
170 self.data['osystem'],
171 self.data['distro_series'],
172 )
173+ if hasattr(self.data, "_mutable"):
174+ self.data._mutable = was_mutable
175 super(LicenseKeyForm, self).full_clean()
176
177 def clean(self):
178
179=== modified file 'src/maasserver/forms/ephemeral.py'
180--- src/maasserver/forms/ephemeral.py 2017-03-29 13:34:55 +0000
181+++ src/maasserver/forms/ephemeral.py 2017-06-21 15:23:27 +0000
182@@ -47,7 +47,7 @@
183 required=False, initial=None, choices=choices)
184
185 def __init__(self, instance, user, data=None, **kwargs):
186- data = {} if data is None else data
187+ data = {} if data is None else data.copy()
188 super().__init__(data=data, **kwargs)
189 self._set_up_script_fields()
190 self.instance = instance
191
192=== modified file 'src/maasserver/forms/iprange.py'
193--- src/maasserver/forms/iprange.py 2017-02-17 14:23:04 +0000
194+++ src/maasserver/forms/iprange.py 2017-06-21 15:23:27 +0000
195@@ -30,6 +30,8 @@
196 self, data=None, instance=None, request=None, *args, **kwargs):
197 if data is None:
198 data = {}
199+ else:
200+ data = data.copy()
201 # If this is a new IPRange, fill in the 'user' and 'subnet' fields
202 # automatically, if necessary.
203 if instance is None:
204
205=== modified file 'src/maasserver/forms/script.py'
206--- src/maasserver/forms/script.py 2017-04-13 02:22:38 +0000
207+++ src/maasserver/forms/script.py 2017-06-21 15:23:27 +0000
208@@ -48,30 +48,36 @@
209 'script',
210 )
211
212- def __init__(self, instance=None, **kwargs):
213- super().__init__(instance=instance, **kwargs)
214+ def __init__(self, instance=None, data=None, **kwargs):
215 if instance is None:
216- for field in ['name', 'script']:
217- self.fields[field].required = True
218 script_data_key = 'data'
219 else:
220- for field in ['name', 'script']:
221- self.fields[field].required = False
222- self.fields['script'].initial = instance.script
223 script_data_key = 'new_data'
224- if 'comment' in self.data and 'script' in self.data:
225+
226+ data = data.copy()
227+ if 'comment' in data and 'script' in data:
228 script_data = {
229- 'comment': self.data.get('comment'),
230- script_data_key: self.data.get('script'),
231+ 'comment': data.get('comment'),
232+ script_data_key: data.get('script'),
233 }
234- self.data['script'] = script_data
235- self.data.pop('comment')
236+ data['script'] = script_data
237+ data.pop('comment')
238 # Alias type to script_type to allow for consistent naming in the API.
239- if 'type' in self.data and 'script_type' not in self.data:
240- self.data['script_type'] = self.data['type']
241+ if 'type' in data and 'script_type' not in data:
242+ data['script_type'] = data['type']
243 # self.data is a QueryDict. pop returns a list containing the value
244 # while directly accessing it returns just the value.
245- self.data.pop('type')
246+ data.pop('type')
247+
248+ super().__init__(instance=instance, data=data, **kwargs)
249+
250+ if instance is None:
251+ for field in ['name', 'script']:
252+ self.fields[field].required = True
253+ else:
254+ for field in ['name', 'script']:
255+ self.fields[field].required = False
256+ self.fields['script'].initial = instance.script
257
258 def clean(self):
259 cleaned_data = super().clean()