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

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

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.

(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"
}

[Blake Rouse (blake-rouse)]

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

=== 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 @@
         # If the user provides no parametes to the create command, then
         # django will make the request.data=None. This will cause the form
         # to be valid, not returning all the missing fields.
-        data = request.data
-        if data is None:
+        if request.data is None:
             data = {}
+        else:
+            data = request.data.copy()
+
+        if "distro_series" in data:
+            # Preprocess distro_series if present.
+            if "/" in data["distro_series"]:
+                if "osystem" not in data:
+                    # Construct osystem value from distroseries.
+                    data["osystem"] = data["distro_series"].split("/", 1)[0]
+            else:
+                # If distro_series is not of the form "os/series", we combine
+                # osystem with distro_series since that is what LicenseKeyForm
+                # expects.
+                if "osystem" in data:
+                    data["distro_series"] = "%s/%s" % (
+                        data["osystem"], data["distro_series"])
         form = LicenseKeyForm(data=data)
         if not form.is_valid():
             raise MAASAPIValidationError(form.errors)

=== 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 @@
             osystem=params['osystem'], distro_series=params['distro_series'])
         self.assertAttributes(license_key, params)
 
+    def test_POST_supports_combined_distro_series(self):
+        # API allows specifying only distro_series containing both
+        # os and series in the "os/series" form.
+        self.become_admin()
+        release = make_rpc_release(requires_license_key=True)
+        osystem = make_rpc_osystem(releases=[release])
+        patch_usable_osystems(self, osystems=[osystem])
+        self.patch_autospec(forms, 'validate_license_key').return_value = True
+        params = {
+            'distro_series': "%s/%s" % (osystem['name'], release['name']),
+            'license_key': factory.make_name('key'),
+        }
+        response = self.client.post(reverse('license_keys_handler'), params)
+        self.assertEqual(http.client.OK, response.status_code)
+        license_key = LicenseKey.objects.get(
+            osystem=osystem['name'], distro_series=release['name'])
+        expected_params = {
+            'osystem': osystem['name'],
+            'distro_series': release['name'],
+            'license_key': params['license_key'],
+        }
+        self.assertAttributes(license_key, expected_params)
+
+    def test_POST_requires_osystem(self):
+        # If osystem is not specified and distro_series is not in the
+        # osystem/release form, API call fails.
+        self.become_admin()
+        release = make_rpc_release(requires_license_key=True)
+        osystem = make_rpc_osystem(releases=[release])
+        patch_usable_osystems(self, osystems=[osystem])
+        self.patch_autospec(forms, 'validate_license_key').return_value = True
+        params = {
+            'distro_series': release['name'],
+            'license_key': factory.make_name('key'),
+        }
+        response = self.client.post(reverse('license_keys_handler'), params)
+        self.assertEqual(http.client.BAD_REQUEST, response.status_code)
+
     def test_POST_requires_admin(self):
         osystem = factory.make_name('no-os')
         series = factory.make_name('no-series')

=== 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 @@
             if instance is not None:
                 self.initial['distro_series'] = initial_value
 
-    def full_clean(self):
-        # When this form is used from the API, the distro_series field will
-        # not be formatted correctly. This is to make it easy on the user, and
-        # 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):
         """Validate distro_series and osystem match, and license_key is valid
         for selected operating system and series."""

=== 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 @@
             {'__all__': ['Invalid license key.']},
             form.errors)
 
-    def test_handles_missing_osystem_in_distro_series(self):
-        osystem, release = self.make_os_with_license_key()
-        self.patch_autospec(forms, 'validate_license_key').return_value = True
-        key = factory.make_name('key')
-        definition = {
-            'osystem': osystem['name'],
-            'distro_series': release['name'],
-            'license_key': key,
-            }
-        form = LicenseKeyForm(data=definition.copy())
-        form.save()
-        license_key_obj = LicenseKey.objects.get(
-            osystem=osystem['name'], distro_series=release['name'])
-        self.assertAttributes(license_key_obj, definition)
-
     def test_requires_all_fields(self):
         form = LicenseKeyForm(data={})
         self.assertFalse(form.is_valid(), form.errors)
@@ -106,7 +91,7 @@
             license_key=key)
         definition = {
             'osystem': osystem['name'],
-            'distro_series': release['name'],
+            'distro_series': "%s/%s" % (osystem['name'], release['name']),
             'license_key': key,
             }
         form = LicenseKeyForm(data=definition)

=== 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 @@
         add_link = reverse('license-key-add')
         definition = {
             'osystem': osystem['name'],
-            'distro_series': series,
+            'distro_series': "%s/%s" % (osystem['name'], series),
             'license_key': key,
             }
         response = self.client.post(add_link, definition)
@@ -134,7 +134,12 @@
             (response.status_code, extract_redirect(response)))
         new_license_key = LicenseKey.objects.get(
             osystem=osystem['name'], distro_series=series)
-        self.assertAttributes(new_license_key, definition)
+        expected_result = {
+            'osystem': osystem['name'],
+            'distro_series': series,
+            'license_key': key,
+            }
+        self.assertAttributes(new_license_key, expected_result)
 
 
 class LicenseKeyEditTest(MAASServerTestCase):
@@ -151,14 +156,19 @@
             'license-key-edit', args=[key.osystem, key.distro_series])
         definition = {
             'osystem': key.osystem,
+            'distro_series': "%s/%s" % (key.osystem, key.distro_series),
+            'license_key': new_key,
+            }
+        response = self.client.post(edit_link, definition)
+        self.assertEqual(
+            (http.client.FOUND, reverse('settings')),
+            (response.status_code, extract_redirect(response)))
+        expected_result = {
+            'osystem': key.osystem,
             'distro_series': key.distro_series,
             'license_key': new_key,
             }
-        response = self.client.post(edit_link, definition)
-        self.assertEqual(
-            (http.client.FOUND, reverse('settings')),
-            (response.status_code, extract_redirect(response)))
-        self.assertAttributes(reload_object(key), definition)
+        self.assertAttributes(reload_object(key), expected_result)
 
 
 class LicenseKeyDeleteTest(MAASServerTestCase):