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

Proposed by Данило Шеган on 2017-06-19
Status: Merged
Approved by: Данило Шеган on 2017-06-21
Approved revision: 6109
Merged at revision: 6098
Proposed branch: lp:~danilo/maas/django-upgrade-legacy-migrations
Merge into: lp:maas/trunk
Prerequisite: lp:~danilo/maas/django-upgrade-method-fixes
Diff against target: 373 lines (+95/-97)
8 files modified
src/maasserver/djangosettings/development.py (+10/-1)
src/maasserver/fields.py (+0/-17)
src/maasserver/middleware.py (+54/-12)
src/maasserver/tests/__init__.py (+9/-1)
src/maasserver/tests/test_fields.py (+0/-7)
src/maasserver/tests/test_middleware.py (+6/-6)
src/maastesting/djangotestcase.py (+2/-53)
src/metadataserver/tests/__init__.py (+14/-0)
To merge this branch: bzr merge lp:~danilo/maas/django-upgrade-legacy-migrations
Reviewer Review Type Date Requested Status
Mike Pontillo 2017-06-19 Approve on 2017-06-21
Review via email: mp+325898@code.launchpad.net

Commit message

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.

Description of the change

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.

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

Merge trunk.

6107. By Данило Шеган on 2017-06-19

Fix imports.

Mike Pontillo (mpontillo) wrote :

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

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.

6108. By Данило Шеган on 2017-06-20

Drop __all__ definitions in src/{metadata,maas}server/tests/__init__.py.

Mike Pontillo (mpontillo) wrote :

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

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!

6109. By Данило Шеган on 2017-06-21

Make a comment about global INSTALLED_APPS being reset by a function.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/djangosettings/development.py'
2--- src/maasserver/djangosettings/development.py 2017-05-22 13:29:04 +0000
3+++ src/maasserver/djangosettings/development.py 2017-06-21 15:20:56 +0000
4@@ -14,12 +14,19 @@
5 )
6
7 # We expect the following settings to be overridden. They are mentioned here
8-# to silence lint warnings.
9+# to silence lint warnings: import_settings() below will actually re-set it
10+# to a tuple as set in settings.INSTALLED_APPS.
11 INSTALLED_APPS = None
12
13 # Extend base settings.
14 import_settings(settings)
15
16+prevent_migrations = StringBool().to_python(
17+ os.environ.get("MAAS_PREVENT_MIGRATIONS", 0))
18+
19+if prevent_migrations:
20+ INSTALLED_APPS += ("maasserver.tests", "metadataserver.tests")
21+
22 # Use our custom test runner, which makes sure that a local database
23 # cluster is running in the branch.
24 TEST_RUNNER = 'maastesting.djangoloader.MAASDjangoTestRunner'
25@@ -97,7 +104,9 @@
26 'sites': 'maastesting.empty',
27 'piston3': 'maastesting.empty',
28 'maasserver': 'maastesting.empty',
29+ 'maasserver.tests': 'maastesting.empty',
30 'metadataserver': 'maastesting.empty',
31+ 'metadataserver.tests': 'maastesting.empty',
32 }
33
34 PASSWORD_HASHERS = (
35
36=== modified file 'src/maasserver/fields.py'
37--- src/maasserver/fields.py 2017-06-02 11:01:43 +0000
38+++ src/maasserver/fields.py 2017-06-21 15:20:56 +0000
39@@ -320,12 +320,6 @@
40 def get_internal_type(self):
41 return 'TextField'
42
43- def get_prep_lookup(self, lookup_type, value):
44- if lookup_type not in ['exact', 'isnull']:
45- raise TypeError("Lookup type %s is not supported." % lookup_type)
46- return super(JSONObjectField, self).get_prep_lookup(
47- lookup_type, value)
48-
49 def formfield(self, form_class=None, **kwargs):
50 """Return a plain `forms.Field` here to avoid "helpful" conversions.
51
52@@ -355,17 +349,6 @@
53 def db_type(self, connection):
54 return "xml"
55
56- def get_db_prep_lookup(self, lookup_type, value, **kwargs):
57- """Limit lookup types to those that work on xml.
58-
59- Unlike character fields the xml type is non-comparible, see:
60- <http://www.postgresql.org/docs/devel/static/datatype-xml.html>
61- """
62- if lookup_type != 'isnull':
63- raise TypeError("Lookup type %s is not supported." % lookup_type)
64- return super(XMLField, self).get_db_prep_lookup(
65- lookup_type, value, **kwargs)
66-
67
68 class EditableBinaryField(BinaryField):
69 """An editable binary field.
70
71=== modified file 'src/maasserver/middleware.py'
72--- src/maasserver/middleware.py 2017-03-14 20:50:31 +0000
73+++ src/maasserver/middleware.py 2017-06-21 15:20:56 +0000
74@@ -16,6 +16,7 @@
75 import http.client
76 import json
77 import logging
78+from pprint import pformat
79 import re
80 import sys
81 import traceback
82@@ -34,6 +35,8 @@
83 HttpResponseForbidden,
84 HttpResponseRedirect,
85 )
86+from django.utils import six
87+from django.utils.encoding import force_str
88 from django.utils.http import urlquote_plus
89 from maasserver import logger
90 from maasserver.bootresources import SIMPLESTREAMS_URL_REGEXP
91@@ -55,16 +58,6 @@
92 )
93 from provisioningserver.utils.shell import ExternalProcessError
94
95-
96-try:
97- from django.http.request import build_request_repr
98-except ImportError:
99- # build_request_repr is only used for debugging: use
100- # a degraded version if build_request_repr is not
101- # available (i.e. if Django version < 1.5).
102- build_request_repr = repr
103-
104-
105 # 'Retry-After' header sent for httplib.SERVICE_UNAVAILABLE
106 # responses.
107 RETRY_AFTER_SERVICE_UNAVAILABLE = 10
108@@ -272,12 +265,61 @@
109
110 log_level = logging.DEBUG
111
112+ # Taken straight out of Django 1.8 django.http.request module to improve
113+ # our debug output on requests (dropped in Django 1.9).
114+ @classmethod
115+ def _build_request_repr(
116+ self, request, path_override=None, GET_override=None,
117+ POST_override=None, COOKIES_override=None, META_override=None):
118+ """
119+ Builds and returns the request's representation string. The request's
120+ attributes may be overridden by pre-processed values.
121+ """
122+ # Since this is called as part of error handling, we need to be very
123+ # robust against potentially malformed input.
124+ try:
125+ get = (pformat(GET_override)
126+ if GET_override is not None
127+ else pformat(request.GET))
128+ except Exception:
129+ get = '<could not parse>'
130+ if request._post_parse_error:
131+ post = '<could not parse>'
132+ else:
133+ try:
134+ post = (pformat(POST_override)
135+ if POST_override is not None
136+ else pformat(request.POST))
137+ except Exception:
138+ post = '<could not parse>'
139+ try:
140+ cookies = (pformat(COOKIES_override)
141+ if COOKIES_override is not None
142+ else pformat(request.COOKIES))
143+ except Exception:
144+ cookies = '<could not parse>'
145+ try:
146+ meta = (pformat(META_override)
147+ if META_override is not None
148+ else pformat(request.META))
149+ except Exception:
150+ meta = '<could not parse>'
151+ path = path_override if path_override is not None else request.path
152+ return force_str(
153+ '<%s\npath:%s,\nGET:%s,\nPOST:%s,\nCOOKIES:%s,\nMETA:%s>' %
154+ (request.__class__.__name__,
155+ path,
156+ six.text_type(get),
157+ six.text_type(post),
158+ six.text_type(cookies),
159+ six.text_type(meta)))
160+
161 def process_request(self, request):
162 if logger.isEnabledFor(self.log_level):
163 header = " Request dump ".center(79, "#")
164 logger.log(
165- self.log_level,
166- "%s\n%s", header, build_request_repr(request))
167+ self.log_level, "%s\n%s", header,
168+ self._build_request_repr(request))
169 return None # Allow request processing to continue unabated.
170
171 def process_response(self, request, response):
172
173=== modified file 'src/maasserver/tests/__init__.py'
174--- src/maasserver/tests/__init__.py 2015-12-01 18:12:59 +0000
175+++ src/maasserver/tests/__init__.py 2017-06-21 15:20:56 +0000
176@@ -3,4 +3,12 @@
177
178 """Tests for `maasserver`."""
179
180-__all__ = []
181+from django.apps import AppConfig
182+
183+
184+class MAASServerTestsConfig(AppConfig):
185+ name = 'maasserver.tests'
186+ label = 'maasserver_tests'
187+
188+
189+default_app_config = 'maasserver.tests.MAASServerTestsConfig'
190
191=== modified file 'src/maasserver/tests/test_fields.py'
192--- src/maasserver/tests/test_fields.py 2017-06-02 14:37:33 +0000
193+++ src/maasserver/tests/test_fields.py 2017-06-21 15:20:56 +0000
194@@ -306,10 +306,6 @@
195 test_instance = JSONFieldModel.objects.get(value__isnull=True)
196 self.assertIsNone(test_instance.value)
197
198- def test_field_another_lookup_fails(self):
199- # Others lookups are not allowed.
200- self.assertRaises(TypeError, JSONFieldModel.objects.get, value__gte=3)
201-
202 def test_form_field_is_a_plain_field(self):
203 self.assertThat(
204 JSONObjectField().formfield(),
205@@ -350,9 +346,6 @@
206 test_instance = XMLFieldModel.objects.get(value__isnull=True)
207 self.assertIsNone(test_instance.value)
208
209- def test_lookup_exact_unsupported(self):
210- self.assertRaises(TypeError, XMLFieldModel.objects.get, value="")
211-
212
213 class TestEditableBinaryField(MAASServerTestCase):
214
215
216=== modified file 'src/maasserver/tests/test_middleware.py'
217--- src/maasserver/tests/test_middleware.py 2016-05-12 19:07:37 +0000
218+++ src/maasserver/tests/test_middleware.py 2017-06-21 15:20:56 +0000
219@@ -20,7 +20,6 @@
220 )
221 from django.core.urlresolvers import reverse
222 from django.http import HttpResponse
223-from django.http.request import build_request_repr
224 from fixtures import FakeLogger
225 from maasserver import middleware as middleware_module
226 from maasserver.components import (
227@@ -239,9 +238,8 @@
228 logger = self.useFixture(FakeLogger('maasserver', logging.INFO))
229 request = factory.make_fake_request("/api/2.0/nodes/")
230 DebuggingLoggerMiddleware().process_request(request)
231- self.assertThat(
232- logger.output,
233- Not(Contains(build_request_repr(request))))
234+ debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
235+ self.assertThat(logger.output, Not(Contains(debug_output)))
236
237 def test_debugging_logger_does_not_log_response_if_info_level(self):
238 logger = self.useFixture(FakeLogger('maasserver', logging.INFO))
239@@ -250,15 +248,17 @@
240 content="test content",
241 content_type=b"text/plain; charset=utf-8")
242 DebuggingLoggerMiddleware().process_response(request, response)
243+ debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
244 self.assertThat(
245- logger.output, Not(Contains(build_request_repr(request))))
246+ logger.output, Not(Contains(debug_output)))
247
248 def test_debugging_logger_logs_request(self):
249 logger = self.useFixture(FakeLogger('maasserver', logging.DEBUG))
250 request = factory.make_fake_request("/api/2.0/nodes/")
251 request.content = "test content"
252 DebuggingLoggerMiddleware().process_request(request)
253- self.assertThat(logger.output, Contains(build_request_repr(request)))
254+ debug_output = DebuggingLoggerMiddleware._build_request_repr(request)
255+ self.assertThat(logger.output, Contains(debug_output))
256
257 def test_debugging_logger_logs_response(self):
258 logger = self.useFixture(FakeLogger('maasserver', logging.DEBUG))
259
260=== modified file 'src/maastesting/djangotestcase.py'
261--- src/maastesting/djangotestcase.py 2017-01-28 00:51:47 +0000
262+++ src/maastesting/djangotestcase.py 2017-06-21 15:20:56 +0000
263@@ -14,8 +14,6 @@
264 time,
265 )
266
267-from django.apps import apps as django_apps
268-from django.core.management import call_command
269 from django.core.signals import request_started
270 from django.db import (
271 connection,
272@@ -170,45 +168,7 @@
273 )
274
275
276-class InstallDjangoAppsMixin:
277- """Install extra applications into the Django ``INSTALLED_APPS`` setting.
278-
279- :deprecated: Do NOT use in new tests.
280- """
281-
282- def _setup_apps(self, apps):
283- self._did_set_apps = False
284- if len(apps) > 0:
285- # Inject the apps into django now that the fixture is setup.
286- self._did_set_apps = True
287- current_apps = [
288- current_app.name
289- for current_app in django_apps.get_app_configs()
290- ]
291- need_to_add = [
292- new_app
293- for new_app in apps
294- if new_app not in current_apps
295- ]
296- for app in need_to_add:
297- current_apps.append(app)
298- # Set the installed applications in django. This requires another
299- # call to unset_installed_apps to reset to the previous value.
300- django_apps.set_installed_apps(current_apps)
301- # Call migrate that will actual perform the migrations for this
302- # applications and if no migrations exists then it will fallback
303- # to performing 'syncdb'.
304- call_command("migrate")
305-
306- def _teardown_apps(self):
307- # Check that the internal __set_apps is set so that the required
308- # unset_installed_apps can be called.
309- if self._did_set_apps:
310- django_apps.unset_installed_apps()
311-
312-
313-class DjangoTestCase(
314- django.test.TestCase, MAASTestCase, InstallDjangoAppsMixin):
315+class DjangoTestCase(django.test.TestCase, MAASTestCase):
316 """A Django `TestCase` for MAAS.
317
318 Generally you should NOT directly subclass this for tests; use an
319@@ -232,12 +192,7 @@
320 # This attribute is used as a tag with Nose.
321 legacy = True
322
323- def _fixture_setup(self):
324- super(DjangoTestCase, self)._fixture_setup()
325- self._setup_apps(self.apps)
326-
327 def _fixture_teardown(self):
328- self._teardown_apps()
329 super(DjangoTestCase, self)._fixture_teardown()
330 # TODO blake_r: Fix so this is not disabled. Currently not
331 # working with Django 1.8.
332@@ -245,8 +200,7 @@
333 # check_for_rogue_database_activity(self)
334
335
336-class DjangoTransactionTestCase(
337- django.test.TransactionTestCase, MAASTestCase, InstallDjangoAppsMixin):
338+class DjangoTransactionTestCase(django.test.TransactionTestCase, MAASTestCase):
339 """A Django `TransactionTestCase` for MAAS.
340
341 A version of `MAASTestCase` that supports transactions.
342@@ -273,12 +227,7 @@
343 # This attribute is used as a tag with Nose.
344 legacy = True
345
346- def _fixture_setup(self):
347- super(DjangoTransactionTestCase, self)._fixture_setup()
348- self._setup_apps(self.apps)
349-
350 def _fixture_teardown(self):
351- self._teardown_apps()
352 super(DjangoTransactionTestCase, self)._fixture_teardown()
353 # TODO blake_r: Fix so this is not disabled. Currently not
354 # working with Django 1.8.
355
356=== modified file 'src/metadataserver/tests/__init__.py'
357--- src/metadataserver/tests/__init__.py 2013-08-23 12:54:28 +0000
358+++ src/metadataserver/tests/__init__.py 2017-06-21 15:20:56 +0000
359@@ -0,0 +1,14 @@
360+# Copyright 2012-2017 Canonical Ltd. This software is licensed under the
361+# GNU Affero General Public License version 3 (see the file LICENSE).
362+
363+"""Tests for `metadataserver`."""
364+
365+from django.apps import AppConfig
366+
367+
368+class MetadataServerTestsConfig(AppConfig):
369+ name = 'metadataserver.tests'
370+ label = 'metadataserver_tests'
371+
372+
373+default_app_config = 'metadataserver.tests.MetadataServerTestsConfig'