Compatibility changes for upcoming flask version 2.3

vimalloc / flask-jwt-extended

An open source Flask extension that provides JWT support (with batteries included)!

http://flask-jwt-extended.readthedocs.io/en/stable/

MIT License

1.56k stars 239 forks source link

Compatibility changes for upcoming flask version 2.3 #493

Closed jrast closed 2 years ago

jrast commented 2 years ago

This PR addresses the issues mentioned in https://github.com/vimalloc/flask-jwt-extended/issues/492

vimalloc commented 2 years ago

Thanks for looking into this!

~Another option that might be worth considering is to follow suite with flask and depreciate the json encoder options as part of this extension, and mark that release as requiring flask >= 2.3. But that's just a thought I had when I woke up and without any caffeine so I want to think on it some more 😄~

Edit: Should have had my coffee. I didn't look at this closely enough to realize why we have this option, it's to bridge the gap between flask and pyjwt.

jrast commented 2 years ago

Actually many other extensions will also run into problems: Flask-SQLALchemy (https://github.com/pallets-eco/flask-sqlalchemy/issues/1068), Flask-Security / Flask-Security-Too (https://github.com/pallets/flask/discussions/4711) just to name the few I use most. Flask 2.3 will require quite a bit of changes in extensions as so many things will be removed / restructured / change. Maybe it's worth to keep an eye on other extensions and see how they handle the issues.

jrast commented 2 years ago

I'm right now working on this to fix the reviewd changes. Don't expect the optimal solution but I try to at least reach these goals:

Be compatible with Flask < 2.2 where flask.json.JSONEncoder exists
Be forward compatible with Flask >= 2.2 where the JSONEncoder generates deprecation warnings and be prepared once the decoder is removed.

I think this would be a good starting point to going further.

vimalloc commented 2 years ago

I'm right now working on this to fix the reviewd changes. Don't expect the optimal solution but I try to at least reach these goals:
* Be compatible with Flask < 2.2 where flask.json.JSONEncoder exists

* Be forward compatible with Flask >= 2.2 where the JSONEncoder generates deprecation warnings and be prepared once the decoder is removed.
I think this would be a good starting point to going further.

Sounds great to me! I'll be around to help this get merged and a new release cut, and then digging into a more optimal solution can happen after. Thanks for your work on this!! 🥳 👍

jrast commented 2 years ago

I pushed some new code:

Works with flask <2.2 and flask >= 2.2
Does currently not respect the flask.json_provider_class setting and uses conditional imports to get a valid encoder (see json_encoder.py)
Coverage Test might fail as coverage is only run with one flask version and misses the conditional import code for the other flask version.

jrast commented 2 years ago

Damn, now you where faster than me... I was working on a simmilar idea, with the additional benefit that the new app.json_provider_class was respected in case of Flask >= 2.2....

vimalloc commented 2 years ago

Damn, now you where faster than me... I was working on a simmilar idea, with the additional benefit that the new app.json_provider_class was respected in case of Flask >= 2.2....

Sorry, didn't mean to step on your toes here, just had some time so figured I would take a stab at it! If you have a different idea please feel free to revert that last commit and push your stuff up!

jrast commented 2 years ago

No problem at all! Nice to see other motivated devs working on this! Things in my latest commit:

respect app.json_provider_class. However this requires some hacky way to generate a JSONEncoder class on the fly...
moved from json_encoder.py to internal_utils.py
Documented the stuff I did.

If this sound reasonable I will push the changes. I have also updated tox.ini to test with flask < 2.2 and flask > 2.2, I can also push this if you want.

vimalloc commented 2 years ago

No problem at all! Nice to see other motivated devs working on this! Things in my latest commit:
* respect `app.json_provider_class`. However this requires some hacky way to generate a JSONEncoder class on the fly...

* moved from `json_encoder.py` to `internal_utils.py`

* Documented the stuff I did.
If this sound reasonable I will push the changes. I have also updated tox.ini to test with flask < 2.2 and flask > 2.2, I can also push this if you want.

That would be awesome! Thank you for the very thorough work you're doing on this!! 👍

jrast commented 2 years ago

Ok, I force-pushed my changes. Let's see what you think.

vimalloc commented 2 years ago

Looks good. Left one small thought that I think is probably worth changing just to be safe, but overall I like the approach. Thanks!

Seluj78 commented 2 years ago

Amazing ! Could you release this soon ? :)

vimalloc commented 2 years ago

It's up as version 4.4.4. Cheers!