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

fix: avoid circular import of/via rest_framework's APIView #430

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jun 14, 2021

The typing hints added in 9819424 caused a regression with version
0.17.1, where importing OpenApiAuthenticationExtension from
drf_spectacular.extensions might cause a circular import error:

ImportError: Could not import
'project.app.authentication.MyAuthentication' for API setting
'DEFAULT_AUTHENTICATION_CLASSES'. ImportError: cannot import name
'OpenApiAuthenticationExtension' from partially initialized module
'drf_spectacular.extensions' (most likely due to a circular import)
(…/.venv/lib/python3.9/site-packages/drf_spectacular/extensions.py).

The typing hints added in 9819424 caused a regression with version
0.17.1, where importing `OpenApiAuthenticationExtension` from
`drf_spectacular.extensions` might cause a circular import error:

> ImportError: Could not import
> 'project.app.authentication.MyAuthentication' for API setting
> 'DEFAULT_AUTHENTICATION_CLASSES'. ImportError: cannot import name
> 'OpenApiAuthenticationExtension' from partially initialized module
> 'drf_spectacular.extensions' (most likely due to a circular import)
> (…/.venv/lib/python3.9/site-packages/drf_spectacular/extensions.py).
@blueyed
Copy link
Contributor Author

blueyed commented Jun 14, 2021

This probably should have a regression test, but wanted to leave it here for now already.

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #430 (b744034) into master (06737f9) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
- Coverage   98.58%   98.56%   -0.02%     
==========================================
  Files          55       55              
  Lines        5493     5493              
==========================================
- Hits         5415     5414       -1     
- Misses         78       79       +1     
Impacted Files Coverage Δ
drf_spectacular/extensions.py 94.87% <50.00%> (-2.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06737f9...b744034. Read the comment docs.

@jairhenrique
Copy link
Contributor

I've get same problem!

@tfranzel
Copy link
Owner

@blueyed thanks for reporting! i didn't think this import could cause any issues. i have trouble understanding exactly why that is.

sure, the APIView transitively imports the settings as it is at the top of views.py (from rest_framework.settings import api_settings). so far so good. what i don't get is that the very old import below that (from drf_spectacular.plumbing import OpenApiGeneratorExtension) loads plumbing.py which again import the settings. in theory that should do exactly the same. i can't see the difference.

@jairhenrique
Copy link
Contributor

@tfranzel I've get this error when try to load my custom auth class.

Traceback (most recent call last):
  File "/home/vsts/work/1/s/src/manage.py", line 21, in <module>
    main()
  File "/home/vsts/work/1/s/src/manage.py", line 17, in main
    return check_method()
  File "python3.9/site-packages/django/urls/resolvers.py", line 412, in check
    for pattern in self.url_patterns:
  File "python3.9/site-packages/django/utils/functional.py", line 48, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "python3.9/site-packages/django/urls/resolvers.py", line 598, in url_patterns
    patterns = getattr(self.urlconf_module, "urlpatterns", self.urlconf_module)
  File "python3.9/site-packages/django/utils/functional.py", line 48, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "python3.9/site-packages/django/urls/resolvers.py", line 591, in urlconf_module
    return import_module(self.urlconf_name)
  File "python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 855, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/home/vsts/work/1/s/src/madmax/urls.py", line 19, in <module>
    from drf_spectacular.views import SpectacularAPIView, SpectacularSwaggerView
  File "python3.9/site-packages/drf_spectacular/views.py", line 12, in <module>
    from rest_framework.views import APIView
  File "python3.9/site-packages/rest_framework/views.py", line 17, in <module>
    from rest_framework.schemas import DefaultSchema
  File "python3.9/site-packages/rest_framework/schemas/__init__.py", line 33, in <module>
    authentication_classes=api_settings.DEFAULT_AUTHENTICATION_CLASSES,
  File "python3.9/site-packages/rest_framework/settings.py", line 225, in __getattr__
    val = perform_import(val, attr)
  File "python3.9/site-packages/rest_framework/settings.py", line 168, in perform_import
    return [import_from_string(item, setting_name) for item in val]
  File "python3.9/site-packages/rest_framework/settings.py", line 168, in <listcomp>
    return [import_from_string(item, setting_name) for item in val]
  File "python3.9/site-packages/rest_framework/settings.py", line 180, in import_from_string
    raise ImportError(msg)
ImportError: Could not import 'path.to.my.custom.api.auth.CustomAuthentication' for API setting 'DEFAULT_AUTHENTICATION_CLASSES'. ImportError: cannot import name 'APIView' from partially initialized module 'rest_framework.views' (most likely due to a circular import) (python3.9/site-packages/rest_framework/views.py).

@tfranzel
Copy link
Owner

tfranzel commented Jun 14, 2021

@jairhenrique very strange. these errors are the worst. is your custom auth class in the same python file as the AuthExtension?

@jairhenrique
Copy link
Contributor

jairhenrique commented Jun 14, 2021

@tfranzel yes

from typing import Any, Dict

from django.contrib.auth.models import User
from drf_spectacular.extensions import OpenApiAuthenticationExtension
from rest_framework_simplejwt.authentication import JWTAuthentication
from rest_framework_simplejwt.exceptions import (
    AuthenticationFailed,
    InvalidToken,
)
from rest_framework_simplejwt.settings import api_settings


class MyAuthentication(JWTAuthentication):
    def get_user(self, validated_token: Dict[str, Any]) -> User:
        """
        Attempts to find and return a user using the given validated token.
        """
        try:
            user_id = validated_token[api_settings.USER_ID_CLAIM]
        except KeyError:
            raise InvalidToken(
                "Token contained no recognizable user identification"
            )

        try:
            user = User.objects.get(
                **{api_settings.USER_ID_FIELD: user_id},
                clientprofile__user_id=user_id,
            )
        except User.DoesNotExist:
            raise AuthenticationFailed("User not found", code="invalid_user")

        if not user.is_active:
            raise AuthenticationFailed("User is inactive", code="invalid_user")

        return user


class OpenApiMyAuthentication(  # type:ignore
    OpenApiAuthenticationExtension
):
    target_class = "app.helpers.api.auth.MyAuthentication"
    name = "jwtAuth"
    matches_subclass = True

    def get_security_definition(self, auto_schema: Any) -> Dict[str, str]:
        return {
            "type": "http",
            "scheme": "bearer",
            "bearerFormat": "Bearer",
        }

I tried to move the OpenApiMyAuthentication to outside of this file, but I get an erro when validate open api, like this:

Warning #0: Myclass: could not resolve authenticator <class 'app.helpers.api.auth.MyAuthentication'>. There was no OpenApiAuthenticationExtension registered for that class. Try creating one by subclassing it. Ignoring for now.

@tfranzel
Copy link
Owner

i dug into this further. i changed 2 imports in this release. the other one looked equally innocent: 1ec133a#diff-d727722caf6fe5f5507940c9efbcbe843fcca1dec9ac64402f074b08dd1c294fR29

@jairhenrique i was able to reproduce your exact error. however this PR was not enough. the other import failed afterwards. can you confirm this?

@blueyed is your case completely fixed by this PR?

@tfranzel tfranzel merged commit b744034 into tfranzel:master Jun 15, 2021
@tfranzel
Copy link
Owner

@jairhenrique @blueyed i fixed the second import error (hopefully). could you test if the current master works for you both? if yes we can do a hotfix release. thanks a lot.

@jairhenrique
Copy link
Contributor

@tfranzel looks good for me after changes!

@blueyed blueyed deleted the fix-circular-import branch June 15, 2021 15:33
@blueyed
Copy link
Contributor Author

blueyed commented Jun 15, 2021

@tfranzel master works for me also. Thanks for fixing it so fast!

@tfranzel
Copy link
Owner

awesome! fyi, i just released 0.17.2

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

Successfully merging this pull request may close these issues.

3 participants