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

don't import authtoken model until needed #3785

Merged
merged 4 commits into from Jan 5, 2016

Conversation

sheppard
Copy link
Contributor

@sheppard sheppard commented Dec 30, 2015

This PR updates the usage of the authtoken model for better compatibility with Django 1.9, specifically to make it possible to define module-level app APIs that rely on rest_framework with the default authentication settings (which import rest_framework.authentication).

Some background: it is forbidden in Django 1.9 to import models before the app infrastructure has finished loading. This makes it a bit tricky when implementing module-level shortcut APIs like Django's admin.site, wq.db's rest.router interface, and the rest_pandas module (though the latter isn't strictly a Django "app"). Any code imported in the __init__.py for those modules needs to defer importing models until they are needed (c.f. django/django#2228).

Currently, rest_framework.authentication imports the Token model at the top level and therefore cannot be imported in Django 1.9 until after apps are done initializing (see the traceback below). This patch simply defers loading the Token model until it is needed, thus avoiding the import error. As a side effect, this also means that rest_framework.authtoken.models is never imported unless it is being used, which means the workaround for #705 is no longer needed. This issue is similar but not the same - in #705, the error occured when authtoken was not in INSTALLED_APPS, whereas this issue will occur whether or not authtoken is in INSTALLED_APPS.

  (test initialization stuff...)
  File "/local/path/wq.db/tests/__init__.py", line 10, in <module>
    django.setup()
  File "/usr/local/lib/python3.4/dist-packages/django/__init__.py", line 18, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/usr/local/lib/python3.4/dist-packages/django/apps/registry.py", line 85, in populate
    app_config = AppConfig.create(entry)
  File "/usr/local/lib/python3.4/dist-packages/django/apps/config.py", line 90, in create
    module = import_module(entry)
  File "/usr/lib/python3.4/importlib/__init__.py", line 109, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/local/path/wq.db/wq/db/rest/__init__.py", line 2, in <module>
    from .routers import ModelRouter, router  # NOQA
  File "/local/path/wq.db/wq/db/rest/routers.py", line 7, in <module>
    from rest_framework.routers import DefaultRouter, Route
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/routers.py", line 25, in <module>
    from rest_framework import views
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/views.py", line 98, in <module>
    class APIView(View):
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/views.py", line 103, in APIView
    authentication_classes = api_settings.DEFAULT_AUTHENTICATION_CLASSES
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/settings.py", line 203, in __getattr__
    val = perform_import(val, attr)
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/settings.py", line 148, in perform_import
    return [import_from_string(item, setting_name) for item in val]
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/settings.py", line 148, in <listcomp>
    return [import_from_string(item, setting_name) for item in val]
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/settings.py", line 160, in import_from_string
    module = importlib.import_module(module_path)
  File "/usr/lib/python3.4/importlib/__init__.py", line 109, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/authentication.py", line 13, in <module>
    from rest_framework.authtoken.models import Token
  File "/usr/local/lib/python3.4/dist-packages/rest_framework/authtoken/models.py", line 16, in <module>
    class Token(models.Model):
  File "/usr/local/lib/python3.4/dist-packages/django/db/models/base.py", line 94, in __new__
    app_config = apps.get_containing_app_config(module)
  File "/usr/local/lib/python3.4/dist-packages/django/apps/registry.py", line 239, in get_containing_app_config
    self.check_apps_ready()
  File "/usr/local/lib/python3.4/dist-packages/django/apps/registry.py", line 124, in check_apps_ready
    raise AppRegistryNotReady("Apps aren't loaded yet.")
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.

@tomchristie
Copy link
Member

tomchristie commented Jan 5, 2016

Possibly valid, yes - although I expect that there are already folks using the model = attribute, so we might want to be more careful about preserving any current behavior.

@sheppard
Copy link
Contributor Author

sheppard commented Jan 5, 2016

Current behavior should still work via this bit:

def get_model(self):
    if self.model is not None:
        return self.model

@xordoquy
Copy link
Collaborator

xordoquy commented Jan 5, 2016

Seconding @sheppard, I don't see how the proposed change would affect the original behavior.

@@ -180,7 +186,7 @@ def authenticate(self, request):

def authenticate_credentials(self, key):
try:
token = self.model.objects.select_related('user').get(key=key)
token = self.get_model().objects.select_related('user').get(key=key)
except self.model.DoesNotExist:
Copy link
Member

@tomchristie tomchristie Jan 5, 2016

Choose a reason for hiding this comment

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

Still referencing self.model here.

Copy link
Contributor Author

@sheppard sheppard Jan 5, 2016

Choose a reason for hiding this comment

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

Oops - I suppose some tests would help.

@tomchristie
Copy link
Member

tomchristie commented Jan 5, 2016

Current behavior should still work via this bit

Noted. My mistake. :)

@sheppard
Copy link
Contributor Author

sheppard commented Jan 5, 2016

Should it be get_model() or _get_model()? I don't this needs to be a public API.

@tomchristie
Copy link
Member

tomchristie commented Jan 5, 2016

get_model() is fine - we're not documenting it, which is what we consider our baseline for "is this public, and subject to the deprecation policy?'

@sheppard
Copy link
Contributor Author

sheppard commented Jan 5, 2016

Ok, I fixed the issue and added some more test cases.

tomchristie added a commit that referenced this issue Jan 5, 2016
don't import authtoken model until needed
@tomchristie tomchristie merged commit 37f7b76 into encode:master Jan 5, 2016
2 checks passed
@tomchristie
Copy link
Member

tomchristie commented Jan 5, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants