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

Proposed by Данило Шеган on 2017-06-21
Status: Merged
Approved by: Blake Rouse on 2017-06-22
Approved revision: 6122
Merged at revision: 6105
Proposed branch: lp:~danilo/maas/django-upgrade-licensekeyform-immutable-querydict
Merge into: lp:maas/trunk
Prerequisite: lp:~danilo/maas/django-upgrade-immutable-querydict
Diff against target: 200 lines (+73/-48)
5 files modified
src/maasserver/api/license_keys.py (+17/-2)
src/maasserver/api/tests/test_licensekey.py (+38/-0)
src/maasserver/forms/__init__.py (+0/-23)
src/maasserver/forms/tests/test_licensekey.py (+1/-16)
src/maasserver/views/tests/test_settings_license_keys.py (+17/-7)
To merge this branch: bzr merge lp:~danilo/maas/django-upgrade-licensekeyform-immutable-querydict
Reviewer Review Type Date Requested Status
Blake Rouse 2017-06-21 Approve on 2017-06-22
Review via email: mp+326079@code.launchpad.net

Commit message

Move osystem/distro_series combining logic to API handlers instead of LicenseKeyForm.full_clean() which now always requires such a form for distro_series field.

Description of the change

(follow-up from https://code.launchpad.net/~danilo/maas/django-upgrade-immutable-querydict/+merge/325995)

LicenseKeyForm fields are set up dynamically, and distro_series field is set up so that the "keys" are in the form os/series. With Django 1.11, POST data has become immutable in full_clean(), so to avoid the hack of setting internal _mutable to True, this fixes the logic so the combining of osystem and distro_series happens in the API layer instead.

And contrary to what the comment in full_clean() says, it seems API never really supported specifying only distro_series in the os/series format (a test I added for that actually failed with the old code): now it does (see test instructions below).

Testing instructions:

1. make syncdb && make sampledata && make bin/maas
2. make run
3. bin/maas login dev-1.8 http://...:5240/MAAS (use admin API key)
4. Upload a Windows image using a release that requires a license key:

  $ bin/maas dev-1.8 boot-resources create name=windows/win2012 architecture=amd64/generic filetype=ddtgz content@=/etc/passwd
5a. Browse to MAAS settings page, "License keys" section should now show up and you can still add keys ("valid" Windows key is AAAAA-BBBBB-CCCCC-DDDDD-EEEEE).

    Note that when license key validation fails, there's nothing shown in the form: I don't believe that's due to my changes, though.

5b. To test via the API, you can do:

$ bin/maas dev-1.8 boot-resources create name=windows/win2016 architecture=amd64/generic filetype=ddtgz content@=/etc/group

# Use only distro_series.
$ bin/maas dev-1.8 license-keys create distro_series=windows/win2016 license_key="AAAAA-BBBBB-11111-22222-33333"Success.
Machine-readable output follows:
{
    "resource_uri": "/MAAS/api/2.0/license-key/windows/win2016",
    "distro_series": "win2016",
    "osystem": "windows",
    "license_key": "AAAAA-BBBBB-11111-22222-33333"
}
$ bin/maas dev-1.8 license-key delete windows win2016

# Use split up osystem and distro_series.
$ bin/maas dev-1.8 license-keys create osystem=windows distro_series=win2016 license_key="AAAAA-BBBBB-11111-22222-33333"
Success.
Machine-readable output follows:
{
    "resource_uri": "/MAAS/api/2.0/license-key/windows/win2016",
    "distro_series": "win2016",
    "osystem": "windows",
    "license_key": "AAAAA-BBBBB-11111-22222-33333"
}

To post a comment you must log in.
6122. By Данило Шеган on 2017-06-21

Merged django-upgrade-immutable-querydict into django-upgrade-licensekeyform-immutable-querydict.

Blake Rouse (blake-rouse) wrote :

Looks good. Split out very well and much cleaner then mutating the request.data. Thanks for making this change, even though it was not really required for the work you where doing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/license_keys.py'
2--- src/maasserver/api/license_keys.py 2016-03-28 13:54:47 +0000
3+++ src/maasserver/api/license_keys.py 2017-06-21 15:21:39 +0000
4@@ -38,9 +38,24 @@
5 # If the user provides no parametes to the create command, then
6 # django will make the request.data=None. This will cause the form
7 # to be valid, not returning all the missing fields.
8- data = request.data
9- if data is None:
10+ if request.data is None:
11 data = {}
12+ else:
13+ data = request.data.copy()
14+
15+ if "distro_series" in data:
16+ # Preprocess distro_series if present.
17+ if "/" in data["distro_series"]:
18+ if "osystem" not in data:
19+ # Construct osystem value from distroseries.
20+ data["osystem"] = data["distro_series"].split("/", 1)[0]
21+ else:
22+ # If distro_series is not of the form "os/series", we combine
23+ # osystem with distro_series since that is what LicenseKeyForm
24+ # expects.
25+ if "osystem" in data:
26+ data["distro_series"] = "%s/%s" % (
27+ data["osystem"], data["distro_series"])
28 form = LicenseKeyForm(data=data)
29 if not form.is_valid():
30 raise MAASAPIValidationError(form.errors)
31
32=== modified file 'src/maasserver/api/tests/test_licensekey.py'
33--- src/maasserver/api/tests/test_licensekey.py 2017-01-28 00:51:47 +0000
34+++ src/maasserver/api/tests/test_licensekey.py 2017-06-21 15:21:39 +0000
35@@ -206,6 +206,44 @@
36 osystem=params['osystem'], distro_series=params['distro_series'])
37 self.assertAttributes(license_key, params)
38
39+ def test_POST_supports_combined_distro_series(self):
40+ # API allows specifying only distro_series containing both
41+ # os and series in the "os/series" form.
42+ self.become_admin()
43+ release = make_rpc_release(requires_license_key=True)
44+ osystem = make_rpc_osystem(releases=[release])
45+ patch_usable_osystems(self, osystems=[osystem])
46+ self.patch_autospec(forms, 'validate_license_key').return_value = True
47+ params = {
48+ 'distro_series': "%s/%s" % (osystem['name'], release['name']),
49+ 'license_key': factory.make_name('key'),
50+ }
51+ response = self.client.post(reverse('license_keys_handler'), params)
52+ self.assertEqual(http.client.OK, response.status_code)
53+ license_key = LicenseKey.objects.get(
54+ osystem=osystem['name'], distro_series=release['name'])
55+ expected_params = {
56+ 'osystem': osystem['name'],
57+ 'distro_series': release['name'],
58+ 'license_key': params['license_key'],
59+ }
60+ self.assertAttributes(license_key, expected_params)
61+
62+ def test_POST_requires_osystem(self):
63+ # If osystem is not specified and distro_series is not in the
64+ # osystem/release form, API call fails.
65+ self.become_admin()
66+ release = make_rpc_release(requires_license_key=True)
67+ osystem = make_rpc_osystem(releases=[release])
68+ patch_usable_osystems(self, osystems=[osystem])
69+ self.patch_autospec(forms, 'validate_license_key').return_value = True
70+ params = {
71+ 'distro_series': release['name'],
72+ 'license_key': factory.make_name('key'),
73+ }
74+ response = self.client.post(reverse('license_keys_handler'), params)
75+ self.assertEqual(http.client.BAD_REQUEST, response.status_code)
76+
77 def test_POST_requires_admin(self):
78 osystem = factory.make_name('no-os')
79 series = factory.make_name('no-series')
80
81=== modified file 'src/maasserver/forms/__init__.py'
82--- src/maasserver/forms/__init__.py 2017-06-21 15:21:39 +0000
83+++ src/maasserver/forms/__init__.py 2017-06-21 15:21:39 +0000
84@@ -2115,29 +2115,6 @@
85 if instance is not None:
86 self.initial['distro_series'] = initial_value
87
88- def full_clean(self):
89- # When this form is used from the API, the distro_series field will
90- # not be formatted correctly. This is to make it easy on the user, and
91- # not have to call the api with distro_series=os/series. This occurs
92- # in full_clean, so the value is correct before validation occurs on
93- # the distro_series field.
94- # XXX Danilo 2017-06-20: with Django 1.11, POST data is a read-only
95- # QueryDict by default. Unfortunately, per-field validation happens
96- # before clean() is called, and thus it is too late to do this in
97- # clean() where it really belongs.
98- if 'distro_series' in self.data and 'osystem' in self.data:
99- if '/' not in self.data['distro_series']:
100- if hasattr(self.data, "_mutable"):
101- was_mutable = self.data._mutable
102- self.data._mutable = True
103- self.data['distro_series'] = '%s/%s' % (
104- self.data['osystem'],
105- self.data['distro_series'],
106- )
107- if hasattr(self.data, "_mutable"):
108- self.data._mutable = was_mutable
109- super(LicenseKeyForm, self).full_clean()
110-
111 def clean(self):
112 """Validate distro_series and osystem match, and license_key is valid
113 for selected operating system and series."""
114
115=== modified file 'src/maasserver/forms/tests/test_licensekey.py'
116--- src/maasserver/forms/tests/test_licensekey.py 2017-02-17 14:23:04 +0000
117+++ src/maasserver/forms/tests/test_licensekey.py 2017-06-21 15:21:39 +0000
118@@ -75,21 +75,6 @@
119 {'__all__': ['Invalid license key.']},
120 form.errors)
121
122- def test_handles_missing_osystem_in_distro_series(self):
123- osystem, release = self.make_os_with_license_key()
124- self.patch_autospec(forms, 'validate_license_key').return_value = True
125- key = factory.make_name('key')
126- definition = {
127- 'osystem': osystem['name'],
128- 'distro_series': release['name'],
129- 'license_key': key,
130- }
131- form = LicenseKeyForm(data=definition.copy())
132- form.save()
133- license_key_obj = LicenseKey.objects.get(
134- osystem=osystem['name'], distro_series=release['name'])
135- self.assertAttributes(license_key_obj, definition)
136-
137 def test_requires_all_fields(self):
138 form = LicenseKeyForm(data={})
139 self.assertFalse(form.is_valid(), form.errors)
140@@ -106,7 +91,7 @@
141 license_key=key)
142 definition = {
143 'osystem': osystem['name'],
144- 'distro_series': release['name'],
145+ 'distro_series': "%s/%s" % (osystem['name'], release['name']),
146 'license_key': key,
147 }
148 form = LicenseKeyForm(data=definition)
149
150=== modified file 'src/maasserver/views/tests/test_settings_license_keys.py'
151--- src/maasserver/views/tests/test_settings_license_keys.py 2016-03-28 13:54:47 +0000
152+++ src/maasserver/views/tests/test_settings_license_keys.py 2017-06-21 15:21:39 +0000
153@@ -125,7 +125,7 @@
154 add_link = reverse('license-key-add')
155 definition = {
156 'osystem': osystem['name'],
157- 'distro_series': series,
158+ 'distro_series': "%s/%s" % (osystem['name'], series),
159 'license_key': key,
160 }
161 response = self.client.post(add_link, definition)
162@@ -134,7 +134,12 @@
163 (response.status_code, extract_redirect(response)))
164 new_license_key = LicenseKey.objects.get(
165 osystem=osystem['name'], distro_series=series)
166- self.assertAttributes(new_license_key, definition)
167+ expected_result = {
168+ 'osystem': osystem['name'],
169+ 'distro_series': series,
170+ 'license_key': key,
171+ }
172+ self.assertAttributes(new_license_key, expected_result)
173
174
175 class LicenseKeyEditTest(MAASServerTestCase):
176@@ -151,14 +156,19 @@
177 'license-key-edit', args=[key.osystem, key.distro_series])
178 definition = {
179 'osystem': key.osystem,
180+ 'distro_series': "%s/%s" % (key.osystem, key.distro_series),
181+ 'license_key': new_key,
182+ }
183+ response = self.client.post(edit_link, definition)
184+ self.assertEqual(
185+ (http.client.FOUND, reverse('settings')),
186+ (response.status_code, extract_redirect(response)))
187+ expected_result = {
188+ 'osystem': key.osystem,
189 'distro_series': key.distro_series,
190 'license_key': new_key,
191 }
192- response = self.client.post(edit_link, definition)
193- self.assertEqual(
194- (http.client.FOUND, reverse('settings')),
195- (response.status_code, extract_redirect(response)))
196- self.assertAttributes(reload_object(key), definition)
197+ self.assertAttributes(reload_object(key), expected_result)
198
199
200 class LicenseKeyDeleteTest(MAASServerTestCase):