Fix handling of optional guardian module #6038

blueyed · 2018-06-21T09:48:03Z

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 <module>()
     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 <module>()
    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.

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 <module>() 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 <module>() 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 encode#1712 - except for when handling the `ImproperlyConfigured` from there. Since it only gets used by a single class this patch moves the code there.

blueyed · 2018-06-21T09:48:57Z

rest_framework/filters.py

+        try:
+            import guardian
+        except ImportError:
+            raise ImportError('Using DjangoObjectPermissionsFilter, but django-guardian is not installed')


I think changing this to an ImportError instead of AssertionError is fine?!

Probably use a RuntimeError.

We could also include a comment linking to the issue. (As we do in filter_queryset)

carltongibson · 2018-06-21T10:08:26Z

rest_framework/compat.py

-# Fixes (#1712). We keep the try/except for the test suite.
-guardian = None
-try:
-    if 'guardian' in settings.INSTALLED_APPS:


Is the INSTALLED_APPS bit necessary? i.e. Could we not just try the import guardian? (And so keep this in compat, which is my only reservation here. Otherwise I'd be 👍)

It would regress #1712 then.
And catching the ImproperlyConfigured error then (in a generic way) also could have the side effect of swallowing Django's own.

carltongibson · 2018-06-21T11:08:22Z

OK. Thanks. I’ll have a think about it. (Will need to talk to management, but it would be nice to be able to do it.)

rpkilby · 2018-06-21T12:41:09Z

.travis.yml

+        script:
+          - python setup.py bdist_wheel
+          - tox --installpkg ./dist/djangorestframework-*.whl
+          - tox  # test sdist


This env isn't listed in tox?

Does not have to be (AFAIK).
Or at least I've assumed that tox-travis will use the explicit env, but haven't checked the build job itself.
(tox itself allows for tox -e dist-optionals)

rpkilby · 2018-06-21T12:43:57Z

tox.ini

@@ -14,6 +15,8 @@ DJANGO =
    2.0: django20
    2.1: django21
    master: djangomaster
+OPTIONALS =
+    1: optionals


Would prefer to override the [testenv] block instead of introducing an environment variable. eg,

[testenv:py36-django20-optionals deps = {[testenv]deps} -rrequirments/requirments-optionals.txt

How to run it on Travis then? via an explicit TOXENV?

Done it like that now (see #6041).

rpkilby · 2018-06-21T12:52:07Z

Hm. I'm kind of the opinion that we just break django-guardian support out of core, similar to how django-filter was broken out of core.

This is partly driven by the fact that django-guardian seems to have largely ceased development. It's been almost a year since its latest release, and during that time, there's only been ~20 odd commits, most of them related to basic maintenance.

tomchristie

Thanks - some comments here. Let's try to keep the change footprint as minimal as possible.

Aside: The other tack onto this would be to use import guardian in compat.py without the if in INSTALLED_APPS check, and push this issue onto Django guardian instead. (ie. should allow import prior to django.configure())

tomchristie · 2018-06-21T12:48:59Z

.travis.yml

+          - python setup.py bdist_wheel
+          - tox --installpkg ./dist/djangorestframework-*.whl
+          - tox  # test sdist
+


These changes don't immediately appear connected to the rest of the PR - let's drop them.

They are necessary to cover the change in the tests, but I can move that into a separate PR.

tomchristie · 2018-06-21T12:49:48Z

tox.ini

@@ -14,6 +15,8 @@ DJANGO =
    2.0: django20
    2.1: django21
    master: djangomaster
+OPTIONALS =
+    1: optionals


Can we drop this and just keep the tox.ini as-is?

It is required for running tests with (and without) optionals on Travis.
tox-travis uses this then.

@blueyed see my comment above on how to override without requiring the additional factor.

tomchristie · 2018-06-21T12:51:00Z

rest_framework/filters.py

+        try:
+            import guardian
+        except ImportError:
+            raise ImportError('Using DjangoObjectPermissionsFilter, but django-guardian is not installed')


Probably use a RuntimeError.

tomchristie · 2018-06-21T12:52:05Z

rest_framework/filters.py

+        try:
+            import guardian
+        except ImportError:
+            raise ImportError('Using DjangoObjectPermissionsFilter, but django-guardian is not installed')


We could also include a comment linking to the issue. (As we do in filter_queryset)

rpkilby · 2018-06-21T12:57:23Z

push this issue onto Django guardian instead.

@tomchristie - thoughts on breaking this out into a separate package?

tomchristie · 2018-06-21T13:09:58Z

@rpkilby - In favor. (Tho I guess that's a separate issue to this. Even as a third party lib it'd still need to use deferred import to avoid this issue, right?)

rpkilby · 2018-06-21T13:51:02Z

@tomchristie - As a third party package the issue is a little less critical, since you're not likely to directly import it. That said, the change is still an improvement.

I'm happy to do the work, breaking this out into a separate package. Should we do this under the encode organization, or should I place it under my personal account?

blueyed · 2018-06-21T14:40:45Z

I've created #6041 to fix optionals first.
Then this could eventually rebased / picked up again, depending on what @rpkilby comes up with in a separate package.

tomchristie · 2018-06-21T17:26:41Z

Personal account.

rpkilby · 2018-06-22T23:13:53Z

Closing in favor of #6054, given the additional testing/CI changes.

rpkilby · 2018-06-23T02:13:40Z

@tomchristie, I've broken out the code related to django-guardian into https://github.com/rpkilby/django-rest-framework-guardian

Specifically, this is just the DjangoObjectPermissionsFilter, as DjangoObjectPermissions actually isn't specific to django-guardian.

If everything looks good, I'll go ahead and publish django-rest-framework-guardian, and we can start deprecating here.

blueyed commented Jun 21, 2018

View reviewed changes

carltongibson reviewed Jun 21, 2018

View reviewed changes

blueyed added 2 commits June 21, 2018 12:09

tox/travis: test optionals optionally

d77784f

fixup! Fix handling of optional guardian module

0537c14

tests.urls: handle optional coreapi

c0c6ca0

rpkilby reviewed Jun 21, 2018

View reviewed changes

tomchristie requested changes Jun 21, 2018

View reviewed changes

blueyed added 2 commits June 21, 2018 16:16

tests: fix skipping of tests without coreapi/coreschema

f913130

fixup! tox/travis: test optionals optionally

0fc0ee1

blueyed mentioned this pull request Jun 21, 2018

tests: cover and fix handling of optionals #6041

Closed

blueyed mentioned this pull request Jun 21, 2018

tests: fix usage of transaction.non_atomic_requests #6043

Merged

rpkilby mentioned this pull request Jun 22, 2018

Move guardian imports out of compat #6054

Merged

rpkilby closed this Jun 22, 2018

blueyed deleted the guardian2 branch June 23, 2018 00:00

rpkilby mentioned this pull request Jul 10, 2018

Deprecate DjangoObjectPermissionsFilter #6075

Merged

Uh oh!

Fix handling of optional guardian module #6038

Fix handling of optional guardian module #6038

Uh oh!

Conversation

blueyed commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carltongibson commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpkilby commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomchristie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpkilby Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpkilby commented Jun 21, 2018

Uh oh!

tomchristie commented Jun 21, 2018

Uh oh!

rpkilby commented Jun 21, 2018

Uh oh!

blueyed commented Jun 21, 2018

Uh oh!

tomchristie commented Jun 21, 2018

Uh oh!

rpkilby commented Jun 22, 2018

Uh oh!

rpkilby commented Jun 23, 2018

Uh oh!

Uh oh!

blueyed commented Jun 21, 2018 •

edited

Loading

rpkilby commented Jun 21, 2018 •

edited

Loading

rpkilby Jun 21, 2018 •

edited

Loading