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

Added fix that adds support for PyJWT>=2.0.0a1 while keeping support for PyJWT<=1.7.1 #327

Closed
wants to merge 3 commits into from

Conversation

the-papi
Copy link
Contributor

@the-papi the-papi commented Nov 3, 2020

Fixes #325
Fixes #326

@the-papi
Copy link
Contributor Author

the-papi commented Nov 3, 2020

I also think that you should make hotfix release. Many people have trouble with this change.

@Andrew-Chen-Wang
Copy link
Member

@the-papi Thanks for the quick fix. I'm not the release maker, but it's been on everyone's mind. Sorry!

Co-authored-by: Andrew Chen Wang <60190294+Andrew-Chen-Wang@users.noreply.github.com>
@Andrew-Chen-Wang
Copy link
Member

@the-papi I'd also appreciate a quick test case. Somewhere in test_backends.py like doing this: https://github.com/SimpleJWT/django-rest-framework-simplejwt/blob/f511e0aeabc24ec792c66f720f4a059ab1c4180c/tests/test_backends.py#L84-L97

I THINK? Need a test case to test both when it's bytes and when it isn't.

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

I am not sure how should I test it. I've changed only the implementation and not the API. The encode method still returns str, which is compatible with all tests.

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

We would also need different versions of PyJWT if we want to test both branches.

@Andrew-Chen-Wang
Copy link
Member

@the-papi Ah my mistake. Just took a gander in the PyJWT repository and it looks like it's just a breaking change, no option to not decode(). I think I can approve of this (only side-effect of just decreasing coverage).

Also we just published a new release! (although unfortunately it didn't pick up this PR).

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

Is it possible to include this change in release 4.5.1 asap? Due to this breaking change, we had to freeze a version of PyJWT in my company.

@Andrew-Chen-Wang
Copy link
Member

Yes. Not sure why CircleCI freezes up, but I'll email David again to get a quick patch release. I don't have the power to merge if the status checks aren't completely passing.

@Andrew-Chen-Wang
Copy link
Member

@the-papi Can you confirm on your own development branch that SimpleJWT works fine if this is merged?

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

Yes, everything works as expected.

Maybe someone broke CircleCI configuration. There have been several changes recently.

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

I don't understand CircleCI very much, but test workflow no longer contains jobs for django.

3f3228b#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R118

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented Nov 4, 2020

Yes, but it was a simple name change (like how tox would do it). The tests seem to pass in master, so I think all you need to do is rebase your branch with master.

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

Looks like it didn't help.

// Sry for the force push. Had some trouble with git.

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented Nov 4, 2020

@the-papi I recommend 1. deleting your fork since you made the changes on master 2. close this PR and create a new PR. Let's see what happens then

@the-papi
Copy link
Contributor Author

the-papi commented Nov 4, 2020

Ok... will try

@brunohenriquy
Copy link

Hey guys, when is the 4.5.1 with this fix is coming out?

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.

AttributeError: 'str' object has no attribute 'decode'. jwt.encode returns str instead of bytes in 2.0.0a1
3 participants