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 (community) 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
=== modified file 'src/maasserver/api/license_keys.py'
--- src/maasserver/api/license_keys.py 2016-03-28 13:54:47 +0000
+++ src/maasserver/api/license_keys.py 2017-06-21 15:21:39 +0000
@@ -38,9 +38,24 @@
38 # If the user provides no parametes to the create command, then38 # If the user provides no parametes to the create command, then
39 # django will make the request.data=None. This will cause the form39 # django will make the request.data=None. This will cause the form
40 # to be valid, not returning all the missing fields.40 # to be valid, not returning all the missing fields.
41 data = request.data41 if request.data is None:
42 if data is None:
43 data = {}42 data = {}
43 else:
44 data = request.data.copy()
45
46 if "distro_series" in data:
47 # Preprocess distro_series if present.
48 if "/" in data["distro_series"]:
49 if "osystem" not in data:
50 # Construct osystem value from distroseries.
51 data["osystem"] = data["distro_series"].split("/", 1)[0]
52 else:
53 # If distro_series is not of the form "os/series", we combine
54 # osystem with distro_series since that is what LicenseKeyForm
55 # expects.
56 if "osystem" in data:
57 data["distro_series"] = "%s/%s" % (
58 data["osystem"], data["distro_series"])
44 form = LicenseKeyForm(data=data)59 form = LicenseKeyForm(data=data)
45 if not form.is_valid():60 if not form.is_valid():
46 raise MAASAPIValidationError(form.errors)61 raise MAASAPIValidationError(form.errors)
4762
=== modified file 'src/maasserver/api/tests/test_licensekey.py'
--- src/maasserver/api/tests/test_licensekey.py 2017-01-28 00:51:47 +0000
+++ src/maasserver/api/tests/test_licensekey.py 2017-06-21 15:21:39 +0000
@@ -206,6 +206,44 @@
206 osystem=params['osystem'], distro_series=params['distro_series'])206 osystem=params['osystem'], distro_series=params['distro_series'])
207 self.assertAttributes(license_key, params)207 self.assertAttributes(license_key, params)
208208
209 def test_POST_supports_combined_distro_series(self):
210 # API allows specifying only distro_series containing both
211 # os and series in the "os/series" form.
212 self.become_admin()
213 release = make_rpc_release(requires_license_key=True)
214 osystem = make_rpc_osystem(releases=[release])
215 patch_usable_osystems(self, osystems=[osystem])
216 self.patch_autospec(forms, 'validate_license_key').return_value = True
217 params = {
218 'distro_series': "%s/%s" % (osystem['name'], release['name']),
219 'license_key': factory.make_name('key'),
220 }
221 response = self.client.post(reverse('license_keys_handler'), params)
222 self.assertEqual(http.client.OK, response.status_code)
223 license_key = LicenseKey.objects.get(
224 osystem=osystem['name'], distro_series=release['name'])
225 expected_params = {
226 'osystem': osystem['name'],
227 'distro_series': release['name'],
228 'license_key': params['license_key'],
229 }
230 self.assertAttributes(license_key, expected_params)
231
232 def test_POST_requires_osystem(self):
233 # If osystem is not specified and distro_series is not in the
234 # osystem/release form, API call fails.
235 self.become_admin()
236 release = make_rpc_release(requires_license_key=True)
237 osystem = make_rpc_osystem(releases=[release])
238 patch_usable_osystems(self, osystems=[osystem])
239 self.patch_autospec(forms, 'validate_license_key').return_value = True
240 params = {
241 'distro_series': release['name'],
242 'license_key': factory.make_name('key'),
243 }
244 response = self.client.post(reverse('license_keys_handler'), params)
245 self.assertEqual(http.client.BAD_REQUEST, response.status_code)
246
209 def test_POST_requires_admin(self):247 def test_POST_requires_admin(self):
210 osystem = factory.make_name('no-os')248 osystem = factory.make_name('no-os')
211 series = factory.make_name('no-series')249 series = factory.make_name('no-series')
212250
=== modified file 'src/maasserver/forms/__init__.py'
--- src/maasserver/forms/__init__.py 2017-06-21 15:21:39 +0000
+++ src/maasserver/forms/__init__.py 2017-06-21 15:21:39 +0000
@@ -2115,29 +2115,6 @@
2115 if instance is not None:2115 if instance is not None:
2116 self.initial['distro_series'] = initial_value2116 self.initial['distro_series'] = initial_value
21172117
2118 def full_clean(self):
2119 # When this form is used from the API, the distro_series field will
2120 # not be formatted correctly. This is to make it easy on the user, and
2121 # not have to call the api with distro_series=os/series. This occurs
2122 # in full_clean, so the value is correct before validation occurs on
2123 # the distro_series field.
2124 # XXX Danilo 2017-06-20: with Django 1.11, POST data is a read-only
2125 # QueryDict by default. Unfortunately, per-field validation happens
2126 # before clean() is called, and thus it is too late to do this in
2127 # clean() where it really belongs.
2128 if 'distro_series' in self.data and 'osystem' in self.data:
2129 if '/' not in self.data['distro_series']:
2130 if hasattr(self.data, "_mutable"):
2131 was_mutable = self.data._mutable
2132 self.data._mutable = True
2133 self.data['distro_series'] = '%s/%s' % (
2134 self.data['osystem'],
2135 self.data['distro_series'],
2136 )
2137 if hasattr(self.data, "_mutable"):
2138 self.data._mutable = was_mutable
2139 super(LicenseKeyForm, self).full_clean()
2140
2141 def clean(self):2118 def clean(self):
2142 """Validate distro_series and osystem match, and license_key is valid2119 """Validate distro_series and osystem match, and license_key is valid
2143 for selected operating system and series."""2120 for selected operating system and series."""
21442121
=== modified file 'src/maasserver/forms/tests/test_licensekey.py'
--- src/maasserver/forms/tests/test_licensekey.py 2017-02-17 14:23:04 +0000
+++ src/maasserver/forms/tests/test_licensekey.py 2017-06-21 15:21:39 +0000
@@ -75,21 +75,6 @@
75 {'__all__': ['Invalid license key.']},75 {'__all__': ['Invalid license key.']},
76 form.errors)76 form.errors)
7777
78 def test_handles_missing_osystem_in_distro_series(self):
79 osystem, release = self.make_os_with_license_key()
80 self.patch_autospec(forms, 'validate_license_key').return_value = True
81 key = factory.make_name('key')
82 definition = {
83 'osystem': osystem['name'],
84 'distro_series': release['name'],
85 'license_key': key,
86 }
87 form = LicenseKeyForm(data=definition.copy())
88 form.save()
89 license_key_obj = LicenseKey.objects.get(
90 osystem=osystem['name'], distro_series=release['name'])
91 self.assertAttributes(license_key_obj, definition)
92
93 def test_requires_all_fields(self):78 def test_requires_all_fields(self):
94 form = LicenseKeyForm(data={})79 form = LicenseKeyForm(data={})
95 self.assertFalse(form.is_valid(), form.errors)80 self.assertFalse(form.is_valid(), form.errors)
@@ -106,7 +91,7 @@
106 license_key=key)91 license_key=key)
107 definition = {92 definition = {
108 'osystem': osystem['name'],93 'osystem': osystem['name'],
109 'distro_series': release['name'],94 'distro_series': "%s/%s" % (osystem['name'], release['name']),
110 'license_key': key,95 'license_key': key,
111 }96 }
112 form = LicenseKeyForm(data=definition)97 form = LicenseKeyForm(data=definition)
11398
=== modified file 'src/maasserver/views/tests/test_settings_license_keys.py'
--- src/maasserver/views/tests/test_settings_license_keys.py 2016-03-28 13:54:47 +0000
+++ src/maasserver/views/tests/test_settings_license_keys.py 2017-06-21 15:21:39 +0000
@@ -125,7 +125,7 @@
125 add_link = reverse('license-key-add')125 add_link = reverse('license-key-add')
126 definition = {126 definition = {
127 'osystem': osystem['name'],127 'osystem': osystem['name'],
128 'distro_series': series,128 'distro_series': "%s/%s" % (osystem['name'], series),
129 'license_key': key,129 'license_key': key,
130 }130 }
131 response = self.client.post(add_link, definition)131 response = self.client.post(add_link, definition)
@@ -134,7 +134,12 @@
134 (response.status_code, extract_redirect(response)))134 (response.status_code, extract_redirect(response)))
135 new_license_key = LicenseKey.objects.get(135 new_license_key = LicenseKey.objects.get(
136 osystem=osystem['name'], distro_series=series)136 osystem=osystem['name'], distro_series=series)
137 self.assertAttributes(new_license_key, definition)137 expected_result = {
138 'osystem': osystem['name'],
139 'distro_series': series,
140 'license_key': key,
141 }
142 self.assertAttributes(new_license_key, expected_result)
138143
139144
140class LicenseKeyEditTest(MAASServerTestCase):145class LicenseKeyEditTest(MAASServerTestCase):
@@ -151,14 +156,19 @@
151 'license-key-edit', args=[key.osystem, key.distro_series])156 'license-key-edit', args=[key.osystem, key.distro_series])
152 definition = {157 definition = {
153 'osystem': key.osystem,158 'osystem': key.osystem,
159 'distro_series': "%s/%s" % (key.osystem, key.distro_series),
160 'license_key': new_key,
161 }
162 response = self.client.post(edit_link, definition)
163 self.assertEqual(
164 (http.client.FOUND, reverse('settings')),
165 (response.status_code, extract_redirect(response)))
166 expected_result = {
167 'osystem': key.osystem,
154 'distro_series': key.distro_series,168 'distro_series': key.distro_series,
155 'license_key': new_key,169 'license_key': new_key,
156 }170 }
157 response = self.client.post(edit_link, definition)171 self.assertAttributes(reload_object(key), expected_result)
158 self.assertEqual(
159 (http.client.FOUND, reverse('settings')),
160 (response.status_code, extract_redirect(response)))
161 self.assertAttributes(reload_object(key), definition)
162172
163173
164class LicenseKeyDeleteTest(MAASServerTestCase):174class LicenseKeyDeleteTest(MAASServerTestCase):