From 327028c4d91e1394df69be3dacf95d9617489c83 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 11:17:54 +0200 Subject: [PATCH 1/6] Fix handling of optional guardian module Currently you cannot import something from rest_framework without configuring Django, since accessing `settings.INSTALLED_APPS` for the django-guardian throws an ImproperlyConfigured error: from rest_framework.exceptions import ErrorDetail ~/Vcs/django-rest-framework/rest_framework/exceptions.py in () 16 17 from rest_framework import status ---> 18 from rest_framework.compat import unicode_to_repr 19 from rest_framework.utils.serializer_helpers import ReturnDict, ReturnList 20 ~/Vcs/django-rest-framework/rest_framework/compat.py in () 144 guardian = None 145 try: --> 146 if 'guardian' in settings.INSTALLED_APPS: 147 import guardian # noqa 148 except ImportError: ~/Vcs/django-rest-framework/.venv/lib/python3.6/site-packages/django/conf/__init__.py in __getattr__(self, name) 54 """Return the value of a setting and cache it in self.__dict__.""" 55 if self._wrapped is empty: ---> 56 self._setup(name) 57 val = getattr(self._wrapped, name) 58 self.__dict__[name] = val ~/Vcs/django-rest-framework/.venv/lib/python3.6/site-packages/django/conf/__init__.py in _setup(self, name) 39 "You must either define the environment variable %s " 40 "or call settings.configure() before accessing settings." ---> 41 % (desc, ENVIRONMENT_VARIABLE)) 42 43 self._wrapped = Settings(settings_module) ImproperlyConfigured: Requested setting INSTALLED_APPS, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings. I've thought about just trying to import it always, but that is some overhead for something optional, and would also regress #1712 - except for when handling the `ImproperlyConfigured` from there. Since it only gets used by a single class this patch moves the code there. --- rest_framework/compat.py | 10 ---------- rest_framework/filters.py | 10 +++++++--- tests/test_permissions.py | 5 ++++- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/rest_framework/compat.py b/rest_framework/compat.py index 7783bced3..7db548f00 100644 --- a/rest_framework/compat.py +++ b/rest_framework/compat.py @@ -139,16 +139,6 @@ except ImportError: requests = None -# Django-guardian is optional. Import only if guardian is in INSTALLED_APPS -# Fixes (#1712). We keep the try/except for the test suite. -guardian = None -try: - if 'guardian' in settings.INSTALLED_APPS: - import guardian # noqa -except ImportError: - pass - - # PATCH method is not implemented by Django if 'patch' not in View.http_method_names: View.http_method_names = View.http_method_names + ['patch'] diff --git a/rest_framework/filters.py b/rest_framework/filters.py index cf8f6d1f7..3b52e5c19 100644 --- a/rest_framework/filters.py +++ b/rest_framework/filters.py @@ -16,7 +16,7 @@ from django.utils import six from django.utils.encoding import force_text from django.utils.translation import ugettext_lazy as _ -from rest_framework.compat import coreapi, coreschema, distinct, guardian +from rest_framework.compat import coreapi, coreschema, distinct from rest_framework.settings import api_settings @@ -282,7 +282,11 @@ class DjangoObjectPermissionsFilter(BaseFilterBackend): has read object level permissions. """ def __init__(self): - assert guardian, 'Using DjangoObjectPermissionsFilter, but django-guardian is not installed' + try: + import guardian + except ImportError: + raise ImportError('Using DjangoObjectPermissionsFilter, but django-guardian is not installed') + self.guardian_version = guardian.VERSION perm_format = '%(app_label)s.view_%(model_name)s' @@ -300,7 +304,7 @@ class DjangoObjectPermissionsFilter(BaseFilterBackend): 'model_name': model_cls._meta.model_name } permission = self.perm_format % kwargs - if tuple(guardian.VERSION) >= (1, 3): + if tuple(self.guardian_version) >= (1, 3): # Maintain behavior compatibility with versions prior to 1.3 extra = {'accept_global_perms': False} else: diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 80b666180..c39b4d12a 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -12,7 +12,10 @@ from rest_framework import ( HTTP_HEADER_ENCODING, authentication, generics, permissions, serializers, status, views ) -from rest_framework.compat import guardian +try: + import guardian +except ImportError: + guardian = None from rest_framework.filters import DjangoObjectPermissionsFilter from rest_framework.routers import DefaultRouter from rest_framework.test import APIRequestFactory From d77784f8d27b9047e20992a5e23556ec4bb79b11 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 12:09:54 +0200 Subject: [PATCH 2/6] tox/travis: test optionals optionally --- .travis.yml | 8 ++++++++ tox.ini | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index f0d2e05f2..477fd4e3f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,6 +21,7 @@ matrix: - { python: "3.6", env: DJANGO=master } - { python: "3.6", env: DJANGO=1.11 } - { python: "3.6", env: DJANGO=2.0 } + - { python: "3.6", env: DJANGO=2.0 OPTIONALS=1 } - { python: "3.6", env: DJANGO=2.1 } - { python: "2.7", env: TOXENV=lint } - { python: "2.7", env: TOXENV=docs } @@ -32,6 +33,13 @@ matrix: - tox --installpkg ./dist/djangorestframework-*.whl - tox # test sdist + - python: "3.6" + env: TOXENV=dist-optionals + script: + - python setup.py bdist_wheel + - tox --installpkg ./dist/djangorestframework-*.whl + - tox # test sdist + exclude: - { python: "2.7", env: DJANGO=master } - { python: "2.7", env: DJANGO=2.0 } diff --git a/tox.ini b/tox.ini index 852de5e6e..a8fb25ec6 100644 --- a/tox.ini +++ b/tox.ini @@ -6,6 +6,7 @@ envlist = {py35,py36}-django21 {py35,py36}-djangomaster, dist,lint,docs, + py36-django20-optionals, [travis:env] DJANGO = @@ -14,6 +15,8 @@ DJANGO = 2.0: django20 2.1: django21 master: djangomaster +OPTIONALS = + 1: optionals [testenv] commands = ./runtests.py --fast {posargs} --coverage -rw @@ -27,15 +30,15 @@ deps = django20: Django>=2.0,<2.1 django21: Django>=2.1b1,<2.2 djangomaster: https://github.com/django/django/archive/master.tar.gz + optionals: -rrequirements/requirements-optionals.txt -rrequirements/requirements-testing.txt - -rrequirements/requirements-optionals.txt [testenv:dist] commands = ./runtests.py --fast {posargs} --no-pkgroot --staticfiles -rw deps = django + optionals: -rrequirements/requirements-optionals.txt -rrequirements/requirements-testing.txt - -rrequirements/requirements-optionals.txt [testenv:lint] basepython = python2.7 From 0537c1454f6a9c90ec6721453c9bb90d754bd62c Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 12:16:09 +0200 Subject: [PATCH 3/6] fixup! Fix handling of optional guardian module --- tests/test_permissions.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_permissions.py b/tests/test_permissions.py index c39b4d12a..f34c91682 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -12,15 +12,16 @@ from rest_framework import ( HTTP_HEADER_ENCODING, authentication, generics, permissions, serializers, status, views ) -try: - import guardian -except ImportError: - guardian = None from rest_framework.filters import DjangoObjectPermissionsFilter from rest_framework.routers import DefaultRouter from rest_framework.test import APIRequestFactory from tests.models import BasicModel +try: + import guardian +except ImportError: + guardian = None + factory = APIRequestFactory() From c0c6ca028333e939a4e5dfb20b203a08a51a47b5 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 13:15:02 +0200 Subject: [PATCH 4/6] tests.urls: handle optional coreapi --- tests/urls.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/urls.py b/tests/urls.py index 930c1f217..76ada5e3d 100644 --- a/tests/urls.py +++ b/tests/urls.py @@ -5,8 +5,12 @@ We need only the docs urls for DocumentationRenderer tests. """ from django.conf.urls import url +from rest_framework.compat import coreapi from rest_framework.documentation import include_docs_urls -urlpatterns = [ - url(r'^docs/', include_docs_urls(title='Test Suite API')), -] +if coreapi: + urlpatterns = [ + url(r'^docs/', include_docs_urls(title='Test Suite API')), + ] +else: + urlpatterns = [] From f913130055b55051244fe0cd78b8bbac1d50b924 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 16:16:05 +0200 Subject: [PATCH 5/6] tests: fix skipping of tests without coreapi/coreschema --- tests/test_encoders.py | 1 + tests/test_filters.py | 2 ++ tests/test_renderers.py | 2 ++ tests/test_schemas.py | 6 ++++++ 4 files changed, 11 insertions(+) diff --git a/tests/test_encoders.py b/tests/test_encoders.py index 27136df87..12eca8105 100644 --- a/tests/test_encoders.py +++ b/tests/test_encoders.py @@ -76,6 +76,7 @@ class JSONEncoderTests(TestCase): unique_id = uuid4() assert self.encoder.default(unique_id) == str(unique_id) + @pytest.mark.skipif(not coreapi, reason='coreapi is not installed') def test_encode_coreapi_raises_error(self): """ Tests encoding a coreapi objects raises proper error diff --git a/tests/test_filters.py b/tests/test_filters.py index f9e068fec..a7d9a07c1 100644 --- a/tests/test_filters.py +++ b/tests/test_filters.py @@ -10,6 +10,7 @@ from django.test.utils import override_settings from django.utils.six.moves import reload_module from rest_framework import filters, generics, serializers +from rest_framework.compat import coreschema from rest_framework.test import APIRequestFactory factory = APIRequestFactory() @@ -28,6 +29,7 @@ class BaseFilterTests(TestCase): with pytest.raises(NotImplementedError): self.filter_backend.filter_queryset(None, None, None) + @pytest.mark.skipif(not coreschema, reason='coreschema is not installed') def test_get_schema_fields_checks_for_coreapi(self): filters.coreapi = None with pytest.raises(AssertionError): diff --git a/tests/test_renderers.py b/tests/test_renderers.py index 667631f29..d468398d3 100644 --- a/tests/test_renderers.py +++ b/tests/test_renderers.py @@ -709,6 +709,7 @@ class AdminRendererTests(TestCase): self.assertContains(response, 'Iteritemsa string', html=True) +@pytest.mark.skipif(not coreapi, reason='coreapi is not installed') class TestDocumentationRenderer(TestCase): def test_document_with_link_named_data(self): @@ -738,6 +739,7 @@ class TestDocumentationRenderer(TestCase): assert '

Data Endpoint API

' in html +@pytest.mark.skipif(not coreapi, reason='coreapi is not installed') class TestSchemaJSRenderer(TestCase): def test_schemajs_output(self): diff --git a/tests/test_schemas.py b/tests/test_schemas.py index 47afe867d..f929fece5 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -663,6 +663,7 @@ class TestAutoSchema(TestCase): with pytest.raises(AssertionError): descriptor.get_link(None, None, None) # ???: Do the dummy arguments require a tighter assert? + @pytest.mark.skipif(not coreapi, reason='coreapi is not installed') def test_update_fields(self): """ That updating fields by-name helper is correct @@ -698,6 +699,7 @@ class TestAutoSchema(TestCase): assert len(fields) == 1 assert fields[0].required is False + @pytest.mark.skipif(not coreapi, reason='coreapi is not installed') def test_get_manual_fields(self): """That get_manual_fields is applied during get_link""" @@ -718,6 +720,7 @@ class TestAutoSchema(TestCase): assert len(fields) == 2 assert "my_extra_field" in [f.name for f in fields] + @pytest.mark.skipif(not coreapi, reason='coreapi is not installed') def test_view_with_manual_schema(self): path = '/example' @@ -764,6 +767,7 @@ class TestAutoSchema(TestCase): link = view.schema.get_link(path, method, base_url) assert link == expected + @unittest.skipUnless(coreschema, 'coreschema is not installed') def test_field_to_schema(self): label = 'Test label' help_text = 'This is a helpful test text' @@ -983,6 +987,7 @@ naming_collisions_router = SimpleRouter() naming_collisions_router.register(r'collision', NamingCollisionViewSet, base_name="collision") +@pytest.mark.skipif(not coreapi, reason='coreapi is not installed') class TestURLNamingCollisions(TestCase): """ Ref: https://github.com/encode/django-rest-framework/issues/4704 @@ -1167,6 +1172,7 @@ def test_head_and_options_methods_are_excluded(): assert inspector.get_allowed_methods(callback) == ["GET"] +@pytest.mark.skipif(not coreapi, reason='coreapi is not installed') class TestAutoSchemaAllowsFilters(object): class MockAPIView(APIView): filter_backends = [filters.OrderingFilter] From 0fc0ee17785641ad837fa4f3dbfc88b1475c5f12 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 21 Jun 2018 16:31:34 +0200 Subject: [PATCH 6/6] fixup! tox/travis: test optionals optionally --- .travis.yml | 3 ++- tox.ini | 9 +++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 477fd4e3f..16bdf2265 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,7 +21,8 @@ matrix: - { python: "3.6", env: DJANGO=master } - { python: "3.6", env: DJANGO=1.11 } - { python: "3.6", env: DJANGO=2.0 } - - { python: "3.6", env: DJANGO=2.0 OPTIONALS=1 } + - { python: "3.6", env: DJANGO=2.0 } + - { python: "3.6", env: TOXENV=py36-django20-optionals } - { python: "3.6", env: DJANGO=2.1 } - { python: "2.7", env: TOXENV=lint } - { python: "2.7", env: TOXENV=docs } diff --git a/tox.ini b/tox.ini index a8fb25ec6..5749fd6c7 100644 --- a/tox.ini +++ b/tox.ini @@ -6,7 +6,6 @@ envlist = {py35,py36}-django21 {py35,py36}-djangomaster, dist,lint,docs, - py36-django20-optionals, [travis:env] DJANGO = @@ -15,8 +14,6 @@ DJANGO = 2.0: django20 2.1: django21 master: djangomaster -OPTIONALS = - 1: optionals [testenv] commands = ./runtests.py --fast {posargs} --coverage -rw @@ -37,9 +34,13 @@ deps = commands = ./runtests.py --fast {posargs} --no-pkgroot --staticfiles -rw deps = django - optionals: -rrequirements/requirements-optionals.txt -rrequirements/requirements-testing.txt +[testenv:dist-optionals] +commands = {[testenv:dist]commands} +deps = {[testenv:dist]deps} + -rrequirements/requirements-optionals.txt + [testenv:lint] basepython = python2.7 commands = ./runtests.py --lintonly