Merge lp:~danilo/maas/django-upgrade-legacy-migrations into lp:~maas-committers/maas/trunk

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

1. Remove legacy region test setup and instead make it part of the development settings to avoid problems due to differences in import order between Django 1.8 and 1.11.
2. Replace use of build_request_repr for the DebuggingLoggerMiddleware.
3. Stop restricting lookup types for JSONField and XMLField fields because this was of limited use and entirely changed in 1.10.

We also stop restricting lookup types for JSONField and XMLField fields: get_prep_lookup and get_db_prep_lookup methods are gone in 1.10, and lookups are defined in a different way since, and it's not worth it to override all existing built-in lookups for these two fields and catching weird comparisons like >= on JSON fields.

Most of the legacy test setup is now simplified, though there's still cleanup rogue DB connections that fails in 1.8 but now again works in 1.11 that we may want to re-enable.

[Mike Pontillo (mpontillo)]

I'm confused about how appending to INSTALLED_APPS is going to work. I see the following code in the file, when I look at the full diff context:

    # We expect the following settings to be overridden. They are mentioned here
    # to silence lint warnings.
    INSTALLED_APPS = None

    # Extend base settings.
    import_settings(settings)

    prevent_migrations = StringBool().to_python(
    os.environ.get("MAAS_PREVENT_MIGRATIONS", 0))

    if prevent_migrations:
        INSTALLED_APPS += ("maasserver.tests", "metadataserver.tests")

So how is a tuple going to be appended to None?

review: Needs Information

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

import_settings(settings) will actually set it. I am not too happy with how the code works and looks, but import_settings() imports the stuff from a passed-in module into the global namespace: that's where we get INSTALLED_APPS from.

FWIW, as the comment indicates, INSTALLED_APPS is set to None just to avoid lint warnings when it's modified or accessed later on in the file.

Also, note that I have run the tests in both 1.8 and 1.11 about a gazillion times in each of my django 1.11 branches, and I never submit them before they are an improvement in 1.11 (as in a reduced count of test failures), and pass completely with 1.8.

[Mike Pontillo (mpontillo)]

Sounds good; thanks for the explanation.

I'd be happier with this branch if you leave a comment around the INSTALLED_APPS foolishness (Django's, not yours). ;-) But I won't block you on it.

review: Approve

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

I was going to complain about Django too, but it's our code that does that in maasserver.djangosettings.import_settings() — it may be idiomatic for Django, but we've partaken in the foolishness :)

Thanks for the re-review!

=== modified file 'src/maasserver/djangosettings/development.py'
--- src/maasserver/djangosettings/development.py	2017-05-22 13:29:04 +0000
+++ src/maasserver/djangosettings/development.py	2017-06-21 15:20:56 +0000
@@ -14,12 +14,19 @@
 )
 
 # We expect the following settings to be overridden. They are mentioned here
-# to silence lint warnings.
+# to silence lint warnings: import_settings() below will actually re-set it
+# to a tuple as set in settings.INSTALLED_APPS.
 INSTALLED_APPS = None
 
 # Extend base settings.
 import_settings(settings)
 
+prevent_migrations = StringBool().to_python(
+    os.environ.get("MAAS_PREVENT_MIGRATIONS", 0))
+
+if prevent_migrations:
+    INSTALLED_APPS += ("maasserver.tests", "metadataserver.tests")
+
 # Use our custom test runner, which makes sure that a local database
 # cluster is running in the branch.
 TEST_RUNNER = 'maastesting.djangoloader.MAASDjangoTestRunner'
@@ -97,7 +104,9 @@
         'sites': 'maastesting.empty',
         'piston3': 'maastesting.empty',
         'maasserver': 'maastesting.empty',
+        'maasserver.tests': 'maastesting.empty',
         'metadataserver': 'maastesting.empty',
+        'metadataserver.tests': 'maastesting.empty',
     }
 
 PASSWORD_HASHERS = (

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2017-06-02 11:01:43 +0000
+++ src/maasserver/fields.py	2017-06-21 15:20:56 +0000
@@ -320,12 +320,6 @@
     def get_internal_type(self):
         return 'TextField'
 
-    def get_prep_lookup(self, lookup_type, value):
-        if lookup_type not in ['exact', 'isnull']:
-            raise TypeError("Lookup type %s is not supported." % lookup_type)
-        return super(JSONObjectField, self).get_prep_lookup(
-            lookup_type, value)
-
     def formfield(self, form_class=None, **kwargs):
         """Return a plain `forms.Field` here to avoid "helpful" conversions.
 
@@ -355,17 +349,6 @@
     def db_type(self, connection):
         return "xml"
 
-    def get_db_prep_lookup(self, lookup_type, value, **kwargs):
-        """Limit lookup types to those that work on xml.
-
-        Unlike character fields the xml type is non-comparible, see:
-        <http://www.postgresql.org/docs/devel/static/datatype-xml.html>
-        """
-        if lookup_type != 'isnull':
-            raise TypeError("Lookup type %s is not supported." % lookup_type)
-        return super(XMLField, self).get_db_prep_lookup(
-            lookup_type, value, **kwargs)
-
 
 class EditableBinaryField(BinaryField):
     """An editable binary field.

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2017-03-14 20:50:31 +0000
+++ src/maasserver/middleware.py	2017-06-21 15:20:56 +0000
@@ -16,6 +16,7 @@
 import http.client
 import json
 import logging
+from pprint import pformat
 import re
 import sys
 import traceback
@@ -34,6 +35,8 @@
     HttpResponseForbidden,
     HttpResponseRedirect,
 )
+from django.utils import six
+from django.utils.encoding import force_str
 from django.utils.http import urlquote_plus
 from maasserver import logger
 from maasserver.bootresources import SIMPLESTREAMS_URL_REGEXP
@@ -55,16 +58,6 @@
 )
 from provisioningserver.utils.shell import ExternalProcessError
 
-
-try:
-    from django.http.request import build_request_repr
-except ImportError:
-    # build_request_repr is only used for debugging: use
-    # a degraded version if build_request_repr is not
-    # available (i.e. if Django version < 1.5).
-    build_request_repr = repr
-
-
 # 'Retry-After' header sent for httplib.SERVICE_UNAVAILABLE
 # responses.
 RETRY_AFTER_SERVICE_UNAVAILABLE = 10
@@ -272,12 +265,61 @@
 
     log_level = logging.DEBUG
 
+    # Taken straight out of Django 1.8 django.http.request module to improve
+    # our debug output on requests (dropped in Django 1.9).
+    @classmethod
+    def _build_request_repr(
+            self, request, path_override=None, GET_override=None,
+            POST_override=None, COOKIES_override=None, META_override=None):
+        """
+        Builds and returns the request's representation string. The request's
+        attributes may be overridden by pre-processed values.
+        """
+        # Since this is called as part of error handling, we need to be very
+        # robust against potentially malformed input.
+        try:
+            get = (pformat(GET_override)
+                   if GET_override is not None
+                   else pformat(request.GET))
+        except Exception:
+            get = '<could not parse>'
+        if request._post_parse_error:
+            post = '<could not parse>'
+        else:
+            try:
+                post = (pformat(POST_override)
+                        if POST_override is not None
+                        else pformat(request.POST))
+            except Exception:
+                post = '<could not parse>'
+        try:
+            cookies = (pformat(COOKIES_override)
+                       if COOKIES_override is not None
+                       else pformat(request.COOKIES))
+        except Exception:
+            cookies = '<could not parse>'
+        try:
+            meta = (pformat(META_override)
+                    if META_override is not None
+                    else pformat(request.META))
+        except Exception:
+            meta = '<could not parse>'
+        path = path_override if path_override is not None else request.path
+        return force_str(
+            '<%s\npath:%s,\nGET:%s,\nPOST:%s,\nCOOKIES:%s,\nMETA:%s>' %
+            (request.__class__.__name__,
+             path,
+             six.text_type(get),
+             six.text_type(post),
+             six.text_type(cookies),
+             six.text_type(meta)))
+
     def process_request(self, request):
         if logger.isEnabledFor(self.log_level):
             header = " Request dump ".center(79, "#")
             logger.log(
-                self.log_level,
-                "%s\n%s", header, build_request_repr(request))
+                self.log_level, "%s\n%s", header,
+                self._build_request_repr(request))
         return None  # Allow request processing to continue unabated.
 
     def process_response(self, request, response):

=== modified file 'src/maasserver/tests/__init__.py'
--- src/maasserver/tests/__init__.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/tests/__init__.py	2017-06-21 15:20:56 +0000
@@ -3,4 +3,12 @@
 
 """Tests for `maasserver`."""
 
-__all__ = []
+from django.apps import AppConfig
+
+
+class MAASServerTestsConfig(AppConfig):
+    name = 'maasserver.tests'
+    label = 'maasserver_tests'
+
+
+default_app_config = 'maasserver.tests.MAASServerTestsConfig'

=== modified file 'src/maasserver/tests/test_fields.py'
--- src/maasserver/tests/test_fields.py	2017-06-02 14:37:33 +0000
+++ src/maasserver/tests/test_fields.py	2017-06-21 15:20:56 +0000
@@ -306,10 +306,6 @@
         test_instance = JSONFieldModel.objects.get(value__isnull=True)
         self.assertIsNone(test_instance.value)
 
-    def test_field_another_lookup_fails(self):
-        # Others lookups are not allowed.
-        self.assertRaises(TypeError, JSONFieldModel.objects.get, value__gte=3)
-
     def test_form_field_is_a_plain_field(self):
         self.assertThat(
             JSONObjectField().formfield(),
@@ -350,9 +346,6 @@
         test_instance = XMLFieldModel.objects.get(value__isnull=True)
         self.assertIsNone(test_instance.value)
 
-    def test_lookup_exact_unsupported(self):
-        self.assertRaises(TypeError, XMLFieldModel.objects.get, value="")
-
 
 class TestEditableBinaryField(MAASServerTestCase):
 

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2016-05-12 19:07:37 +0000
+++ src/maasserver/tests/test_middleware.py	2017-06-21 15:20:56 +0000
@@ -20,7 +20,6 @@
 )
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
-from django.http.request import build_request_repr
 from fixtures import FakeLogger
 from maasserver import middleware as middleware_module
 from maasserver.components import (
@@ -239,9 +238,8 @@
         logger = self.useFixture(FakeLogger('maasserver', logging.INFO))
         request = factory.make_fake_request("/api/2.0/nodes/")
         DebuggingLoggerMiddleware().process_request(request)
-        self.assertThat(
-            logger.output,
-            Not(Contains(build_request_repr(request))))
+        debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
+        self.assertThat(logger.output, Not(Contains(debug_output)))
 
     def test_debugging_logger_does_not_log_response_if_info_level(self):
         logger = self.useFixture(FakeLogger('maasserver', logging.INFO))
@@ -250,15 +248,17 @@
             content="test content",
             content_type=b"text/plain; charset=utf-8")
         DebuggingLoggerMiddleware().process_response(request, response)
+        debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
         self.assertThat(
-            logger.output, Not(Contains(build_request_repr(request))))
+            logger.output, Not(Contains(debug_output)))
 
     def test_debugging_logger_logs_request(self):
         logger = self.useFixture(FakeLogger('maasserver', logging.DEBUG))
         request = factory.make_fake_request("/api/2.0/nodes/")
         request.content = "test content"
         DebuggingLoggerMiddleware().process_request(request)
-        self.assertThat(logger.output, Contains(build_request_repr(request)))
+        debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
+        self.assertThat(logger.output, Contains(debug_output))
 
     def test_debugging_logger_logs_response(self):
         logger = self.useFixture(FakeLogger('maasserver', logging.DEBUG))

=== modified file 'src/maastesting/djangotestcase.py'
--- src/maastesting/djangotestcase.py	2017-01-28 00:51:47 +0000
+++ src/maastesting/djangotestcase.py	2017-06-21 15:20:56 +0000
@@ -14,8 +14,6 @@
     time,
 )
 
-from django.apps import apps as django_apps
-from django.core.management import call_command
 from django.core.signals import request_started
 from django.db import (
     connection,
@@ -170,45 +168,7 @@
         )
 
 
-class InstallDjangoAppsMixin:
-    """Install extra applications into the Django ``INSTALLED_APPS`` setting.
-
-    :deprecated: Do NOT use in new tests.
-    """
-
-    def _setup_apps(self, apps):
-        self._did_set_apps = False
-        if len(apps) > 0:
-            # Inject the apps into django now that the fixture is setup.
-            self._did_set_apps = True
-            current_apps = [
-                current_app.name
-                for current_app in django_apps.get_app_configs()
-            ]
-            need_to_add = [
-                new_app
-                for new_app in apps
-                if new_app not in current_apps
-            ]
-            for app in need_to_add:
-                current_apps.append(app)
-            # Set the installed applications in django. This requires another
-            # call to unset_installed_apps to reset to the previous value.
-            django_apps.set_installed_apps(current_apps)
-            # Call migrate that will actual perform the migrations for this
-            # applications and if no migrations exists then it will fallback
-            # to performing 'syncdb'.
-            call_command("migrate")
-
-    def _teardown_apps(self):
-        # Check that the internal __set_apps is set so that the required
-        # unset_installed_apps can be called.
-        if self._did_set_apps:
-            django_apps.unset_installed_apps()
-
-
-class DjangoTestCase(
-        django.test.TestCase, MAASTestCase, InstallDjangoAppsMixin):
+class DjangoTestCase(django.test.TestCase, MAASTestCase):
     """A Django `TestCase` for MAAS.
 
     Generally you should NOT directly subclass this for tests; use an
@@ -232,12 +192,7 @@
     # This attribute is used as a tag with Nose.
     legacy = True
 
-    def _fixture_setup(self):
-        super(DjangoTestCase, self)._fixture_setup()
-        self._setup_apps(self.apps)
-
     def _fixture_teardown(self):
-        self._teardown_apps()
         super(DjangoTestCase, self)._fixture_teardown()
         # TODO blake_r: Fix so this is not disabled. Currently not
         # working with Django 1.8.
@@ -245,8 +200,7 @@
         # check_for_rogue_database_activity(self)
 
 
-class DjangoTransactionTestCase(
-        django.test.TransactionTestCase, MAASTestCase, InstallDjangoAppsMixin):
+class DjangoTransactionTestCase(django.test.TransactionTestCase, MAASTestCase):
     """A Django `TransactionTestCase` for MAAS.
 
     A version of `MAASTestCase` that supports transactions.
@@ -273,12 +227,7 @@
     # This attribute is used as a tag with Nose.
     legacy = True
 
-    def _fixture_setup(self):
-        super(DjangoTransactionTestCase, self)._fixture_setup()
-        self._setup_apps(self.apps)
-
     def _fixture_teardown(self):
-        self._teardown_apps()
         super(DjangoTransactionTestCase, self)._fixture_teardown()
         # TODO blake_r: Fix so this is not disabled. Currently not
         # working with Django 1.8.

=== modified file 'src/metadataserver/tests/__init__.py'
--- src/metadataserver/tests/__init__.py	2013-08-23 12:54:28 +0000
+++ src/metadataserver/tests/__init__.py	2017-06-21 15:20:56 +0000
@@ -0,0 +1,14 @@
+# Copyright 2012-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `metadataserver`."""
+
+from django.apps import AppConfig
+
+
+class MetadataServerTestsConfig(AppConfig):
+    name = 'metadataserver.tests'
+    label = 'metadataserver_tests'
+
+
+default_app_config = 'metadataserver.tests.MetadataServerTestsConfig'