fix: Incorrect Token, TokenBackend typing#889
fix: Incorrect Token, TokenBackend typing#889khvn26 wants to merge 3 commits intojazzband:masterfrom
Token, TokenBackend typing#889Conversation
Andrew-Chen-Wang
left a comment
Andrew-Chen-Wang
left a comment
There was a problem hiding this comment.
Thank you for the PR
| except ImportError: | ||
| JWK_CLIENT_AVAILABLE = False | ||
|
|
||
| RawToken = Union[bytes, str] |
There was a problem hiding this comment.
hm i wish there was a way to customize the typing based on the version of JWT.
Specifically, I wonder if we can import something consistent from PyJWT itself that represents the token. Using a union is also not entirely correct (better than what it was before for sure).
There was a problem hiding this comment.
Using a union is also not entirely correct
It is annotated as this exact union in the recent PyJWT versions.
FWIW, bytes | str is also technically supported since 1.7.1 despite the formal jwt: str annotation — see https://github.com/jpadilla/pyjwt/blob/b65e1ac6dc4d11801f3642eaab34ae6a54162c18/jwt/api_jws.py#L171-L177
rest_framework_simplejwt/backends.py
Outdated
There was a problem hiding this comment.
do both v1.7.1 and v2.x versions of JWT expect the token to be string? I can understand v2, but how about v1?
There was a problem hiding this comment.
1.7.1 does not implement a PyJWKClient, forcing self.jwks_client to always be None.
Annoyingly, 2.x PyJWKClient.get_signing_key_from_jwt has an incorrect annotation and can actually handle the union type as it wraps code referenced in my other comment. To avoid an extra decode call, I'd just cast it to str until the upstream issue is resolved.
Andrew-Chen-Wang
left a comment
Andrew-Chen-Wang
left a comment
There was a problem hiding this comment.
Fwiw, this makes sense to fix mypy. Thanks for fixing this + the investigation!
|
TODO it seems like we don't have a mypy and pyright type checking step in the CI. It can be added in two ways:
I think either approach is fine, though pre-commit seems easier. This doesn't need to be added to this PR |
|
I will need to run this on mypy merging. |
2 participants
Closes #787.
This PR aims to fix typing for the user-facing parts of
tokens.Token,backends.TokenBackendclasses:RawTokentype to replace the erroneous"Token"annotation.bytes | str, given the JWT backends support both.I also added annotations for importable settings.