Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django 4.0 compatibility #8178

Merged
merged 7 commits into from
Sep 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api-guide/fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ Corresponds to `django.db.models.fields.DateTimeField`.

* `format` - A string representing the output format. If not specified, this defaults to the same value as the `DATETIME_FORMAT` settings key, which will be `'iso-8601'` unless set. Setting to a format string indicates that `to_representation` return values should be coerced to string output. Format strings are described below. Setting this value to `None` indicates that Python `datetime` objects should be returned by `to_representation`. In this case the datetime encoding will be determined by the renderer.
* `input_formats` - A list of strings representing the input formats which may be used to parse the date. If not specified, the `DATETIME_INPUT_FORMATS` setting will be used, which defaults to `['iso-8601']`.
* `default_timezone` - A `pytz.timezone` representing the timezone. If not specified and the `USE_TZ` setting is enabled, this defaults to the [current timezone][django-current-timezone]. If `USE_TZ` is disabled, then datetime objects will be naive.
* `default_timezone` - A `tzinfo` subclass (`zoneinfo` or `pytz`) prepresenting the timezone. If not specified and the `USE_TZ` setting is enabled, this defaults to the [current timezone][django-current-timezone]. If `USE_TZ` is disabled, then datetime objects will be naive.

#### `DateTimeField` format strings.

Expand Down
30 changes: 22 additions & 8 deletions rest_framework/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import io
from importlib import import_module

import django
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.core.handlers.wsgi import WSGIHandler
Expand Down Expand Up @@ -357,6 +358,13 @@ class APILiveServerTestCase(testcases.LiveServerTestCase):
client_class = APIClient


def cleanup_url_patterns(cls):
if hasattr(cls, '_module_urlpatterns'):
cls._module.urlpatterns = cls._module_urlpatterns
else:
del cls._module.urlpatterns


class URLPatternsTestCase(testcases.SimpleTestCase):
"""
Isolate URL patterns on a per-TestCase basis. For example,
Expand Down Expand Up @@ -385,14 +393,20 @@ def setUpClass(cls):
cls._module.urlpatterns = cls.urlpatterns

cls._override.enable()

if django.VERSION > (4, 0):
cls.addClassCleanup(cls._override.disable)
cls.addClassCleanup(cleanup_url_patterns, cls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is addClassCleanup a new feature in Django 4.0?

I'm not seeing any differences in the docs re. the existing tearDownClass... https://docs.djangoproject.com/en/4.0/topics/testing/tools/#simpletestcase

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addClassCleanup is from unittest.TestCase, added in Python 3.8 https://docs.python.org/3.9/library/unittest.html#unittest.TestCase.addClassCleanup

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Django just happens to be using it from Django 4.0 — See django/django@faba5b7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fab. How about we switch based on the Python version, rather than the Django version?

Copy link
Collaborator Author

@carltongibson carltongibson Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should comment on the release notes for this? 🤔 (cc @felixxm )

I don't suppose there are many projects doing anything as subtle as URLPatternsTestCase

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should comment on the release notes for this? thinking (cc @felixxm )

Sure, if you think it might be helpful. Is it not enough to call super().setUpClass() at the beginning? 🤔

    @classmethod
    def setUpClass(cls):
        super().setUpClass()
        # Get the module of the TestCase subclass
        cls._module = import_module(cls.__module__)
        ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be... Let me try that after lunch 🥪

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... or use cls._overridden_settings and remove redundant cls._override.enable()/disable() calls 🤔

cls._overridden_settings = {
    'ROOT_URLCONF': cls.__module__,
    **cls._overridden_settings,
}
super().setUpClass()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving super().setUpClass() from the bottom of the setUpClass method to the top resolves this without any 4.0 branching.

Do I properly understand why? Nope.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, this works...

    @classmethod
    def setUpClass(cls):
        super().setUpClass()  # <--- This line has moved.

        # Get the module of the TestCase subclass
        cls._module = import_module(cls.__module__)
        cls._override = override_settings(ROOT_URLCONF=cls.__module__)

        if hasattr(cls._module, 'urlpatterns'):
            cls._module_urlpatterns = cls._module.urlpatterns

        cls._module.urlpatterns = cls.urlpatterns

        cls._override.enable()

    @classmethod
    def tearDownClass(cls):
        super().tearDownClass()
        cls._override.disable()

        if hasattr(cls, '_module_urlpatterns'):
            cls._module.urlpatterns = cls._module_urlpatterns
        else:
            del cls._module.urlpatterns


super().setUpClass()

@classmethod
def tearDownClass(cls):
super().tearDownClass()
cls._override.disable()
if django.VERSION < (4, 0):
@classmethod
def tearDownClass(cls):
super().tearDownClass()
cls._override.disable()

if hasattr(cls, '_module_urlpatterns'):
cls._module.urlpatterns = cls._module_urlpatterns
else:
del cls._module.urlpatterns
if hasattr(cls, '_module_urlpatterns'):
cls._module.urlpatterns = cls._module_urlpatterns
else:
del cls._module.urlpatterns
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def get_version(package):
author_email='tom@tomchristie.com', # SEE NOTE BELOW (*)
packages=find_packages(exclude=['tests*']),
include_package_data=True,
install_requires=["django>=2.2"],
install_requires=["django>=2.2", "pytz"],
python_requires=">=3.5",
zip_safe=False,
classifiers=[
Expand Down
12 changes: 11 additions & 1 deletion tests/authentication/test_authentication.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import base64

import django
import pytest
from django.conf import settings
from django.contrib.auth.models import User
Expand Down Expand Up @@ -218,7 +219,16 @@ def test_post_form_session_auth_passing_csrf(self):
Ensure POSTing form over session authentication with CSRF token succeeds.
Regression test for #6088
"""
from django.middleware.csrf import _get_new_csrf_token
# Remove this shim when dropping support for Django 2.2.
if django.VERSION < (3, 0):
from django.middleware.csrf import _get_new_csrf_token
else:
from django.middleware.csrf import (
_get_new_csrf_string, _mask_cipher_secret
)

def _get_new_csrf_token():
return _mask_cipher_secret(_get_new_csrf_string())
carltongibson marked this conversation as resolved.
Show resolved Hide resolved

self.csrf_client.login(username=self.username, password=self.password)

Expand Down
4 changes: 3 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ def pytest_addoption(parser):
def pytest_configure(config):
from django.conf import settings

# USE_L10N is deprecated, and will be removed in Django 5.0.
use_l10n = {"USE_L10N": True} if django.VERSION < (4, 0) else {}
settings.configure(
DEBUG_PROPAGATE_EXCEPTIONS=True,
DATABASES={
Expand All @@ -33,7 +35,6 @@ def pytest_configure(config):
SITE_ID=1,
SECRET_KEY='not very secret in tests',
USE_I18N=True,
USE_L10N=True,
STATIC_URL='/static/',
ROOT_URLCONF='tests.urls',
TEMPLATES=[
Expand Down Expand Up @@ -68,6 +69,7 @@ def pytest_configure(config):
PASSWORD_HASHERS=(
'django.contrib.auth.hashers.MD5PasswordHasher',
),
**use_l10n,
carltongibson marked this conversation as resolved.
Show resolved Hide resolved
)

# guardian is optional
Expand Down
19 changes: 14 additions & 5 deletions tests/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -1220,12 +1220,12 @@ class TestNoStringCoercionDecimalField(FieldValues):


class TestLocalizedDecimalField(TestCase):
@override_settings(USE_L10N=True, LANGUAGE_CODE='pl')
@override_settings(LANGUAGE_CODE='pl')
def test_to_internal_value(self):
field = serializers.DecimalField(max_digits=2, decimal_places=1, localize=True)
assert field.to_internal_value('1,1') == Decimal('1.1')

@override_settings(USE_L10N=True, LANGUAGE_CODE='pl')
@override_settings(LANGUAGE_CODE='pl')
def test_to_representation(self):
field = serializers.DecimalField(max_digits=2, decimal_places=1, localize=True)
assert field.to_representation(Decimal('1.1')) == '1,1'
Expand Down Expand Up @@ -1464,15 +1464,24 @@ def setup_class(cls):
cls.field = serializers.DateTimeField()
cls.kolkata = pytz.timezone('Asia/Kolkata')

def assertUTC(self, tzinfo):
"""
Check UTC for datetime.timezone, ZoneInfo, and pytz tzinfo instances.
"""
assert (
tzinfo is utc or
(getattr(tzinfo, "key", None) or getattr(tzinfo, "zone", None)) == "UTC"
)

def test_default_timezone(self):
assert self.field.default_timezone() == utc
self.assertUTC(self.field.default_timezone())

def test_current_timezone(self):
assert self.field.default_timezone() == utc
self.assertUTC(self.field.default_timezone())
activate(self.kolkata)
assert self.field.default_timezone() == self.kolkata
deactivate()
assert self.field.default_timezone() == utc
self.assertUTC(self.field.default_timezone())


@pytest.mark.skipif(pytz is None, reason='pytz not installed')
Expand Down
22 changes: 17 additions & 5 deletions tests/test_testing.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from io import BytesIO

import django
from django.contrib.auth.models import User
from django.shortcuts import redirect
from django.test import TestCase, override_settings
Expand Down Expand Up @@ -282,6 +283,10 @@ def test_empty_request_content_type(self):
assert request.META['CONTENT_TYPE'] == 'application/json'


def check_urlpatterns(cls):
assert urlpatterns is not cls.urlpatterns


class TestUrlPatternTestCase(URLPatternsTestCase):
urlpatterns = [
path('', view),
Expand All @@ -293,11 +298,18 @@ def setUpClass(cls):
super().setUpClass()
assert urlpatterns is cls.urlpatterns

@classmethod
def tearDownClass(cls):
assert urlpatterns is cls.urlpatterns
super().tearDownClass()
assert urlpatterns is not cls.urlpatterns
if django.VERSION > (4, 0):
cls.addClassCleanup(
check_urlpatterns,
cls
)

if django.VERSION < (4, 0):
@classmethod
def tearDownClass(cls):
assert urlpatterns is cls.urlpatterns
super().tearDownClass()
assert urlpatterns is not cls.urlpatterns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same tearDownClass vs addClassCleanup question as above.

Nothing about it in the release notes... https://docs.djangoproject.com/en/4.0/releases/4.0/ so I'm curious what prompts this to need changing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the commit I linked — we no longer call tearDown ourselves so super() isn't doing what it used to. (I presume the base implementation is empty.)

I didn't trace the exact call flow... — timings on those enable/disable or __enter__/__exit__ s is tricky, but there were failures without following the change in Django.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm that's interesting yeah. Looking through the commit in django/django@faba5b7 I can't see any reason why subclasses would need to stop using tearDownClass, even if Django 4.0 isn't using it directly for it's own implementation. But there's possibly some change in interaction I've not figured out there.

Copy link
Collaborator Author

@carltongibson carltongibson Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no 😀 — it's not obvious.

My suspicion it's the timing on the override_settings usage — which is always delicate, as we know from e.g. #5668 &friends.

😜

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once super().setUpClass() is moved in rest_framework/test.py this class doesn't need to change at all.
(Or at least, that's how it seems from my testing.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suspicion it's the timing on the override_settings usage — which is always delicate, as we know from e.g. #5668 &friends.

Yes that seems very likely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if you want to adjust please do! (Otherwise I'll have another look later on)


def test_urlpatterns(self):
assert self.client.get('/').status_code == 200
Expand Down
4 changes: 3 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ envlist =
{py35,py36,py37}-django22,
{py36,py37,py38,py39}-django31,
{py36,py37,py38,py39}-django32,
{py38,py39}-djangomain,
{py38,py39}-{django40,djangomain},
base,dist,docs,

[travis:env]
DJANGO =
2.2: django22
3.1: django31
3.2: django32
4.0: django40
main: djangomain

[testenv]
Expand All @@ -23,6 +24,7 @@ deps =
django22: Django>=2.2,<3.0
django31: Django>=3.1,<3.2
django32: Django>=3.2,<4.0
django40: Django>=4.0a1,<5.0
djangomain: https://github.com/django/django/archive/main.tar.gz
-rrequirements/requirements-testing.txt
-rrequirements/requirements-optionals.txt
Expand Down