Compatibility changes for upcoming flask version 2.3 #493

jrast · 2022-08-07T12:58:58Z

This PR addresses the issues mentioned in #492

Created a copy of the deprecated JSONEncoder used by flask and use this as a default for PyJWT. This might break for some installations which further extended the flask provided JSONEncoder. To get this working again the extension must be adjusted such that a custom Encoder can be configured.

vimalloc · 2022-08-07T16:34:36Z

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 · 2022-08-07T18:43:54Z

Actually many other extensions will also run into problems: Flask-SQLALchemy (pallets-eco/flask-sqlalchemy#1068), Flask-Security / Flask-Security-Too (pallets/flask#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.

tgross35

Added notes on some little canonicalization things

flask_jwt_extended/utils.py

flask_jwt_extended/config.py

vimalloc

Sorry this took me so long to look at, it's been a crazy week over here.

flask_jwt_extended/utils.py

flask_jwt_extended/config.py

jrast · 2022-08-12T15:47:35Z

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 · 2022-08-12T15:49:10Z

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 · 2022-08-12T16:21:00Z

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.

flask_jwt_extended/json_encoder.py

jrast · 2022-08-12T20:28:38Z

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 · 2022-08-12T20:32:04Z

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 · 2022-08-12T20:36:08Z

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 · 2022-08-12T20:37:05Z

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!! 👍

Respect the json_provider_class when available and stay compatible with flask < 2.2. Extended tox configuration to also test with flask < 2.2.

jrast · 2022-08-12T20:47:17Z

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

flask_jwt_extended/view_decorators.py

flask_jwt_extended/internal_utils.py

vimalloc · 2022-08-12T20:50:56Z

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

The code in question was a runtime check which is no longer necesary.

- Added tests to check the config.json_encoder - Don't create the JSONEncoder class on the fly, instead get the JSON provider class at runtime.

Seluj78

LGTM ! Can't wait to get this release :D

vimalloc

LGTM

Seluj78 · 2022-08-15T16:10:38Z

Amazing ! Could you release this soon ? :)

vimalloc · 2022-08-15T16:41:44Z

It's up as version 4.4.4. Cheers!

Switched from _request_ctx_stack.top to flask.g

eb738d9

jrast marked this pull request as draft August 7, 2022 12:59

jrast marked this pull request as ready for review August 7, 2022 13:59

tgross35 mentioned this pull request Aug 11, 2022

_request_ctx_stack is deprecated as of flask 2.2 #494

Closed

tgross35 reviewed Aug 11, 2022

View reviewed changes

flask_jwt_extended/utils.py Outdated Show resolved Hide resolved

flask_jwt_extended/config.py Outdated Show resolved Hide resolved

flask_jwt_extended/config.py Outdated Show resolved Hide resolved

vimalloc reviewed Aug 12, 2022

View reviewed changes

flask_jwt_extended/utils.py Outdated Show resolved Hide resolved

flask_jwt_extended/config.py Outdated Show resolved Hide resolved

jrast added 2 commits August 12, 2022 18:10

Moved JSONEncoder to separate file

a21a336

Switched from getattribute(g, ...) to g.get(...)

fcea059

vimalloc reviewed Aug 12, 2022

View reviewed changes

flask_jwt_extended/json_encoder.py Outdated Show resolved Hide resolved

vimalloc reviewed Aug 12, 2022

View reviewed changes

flask_jwt_extended/json_encoder.py Outdated Show resolved Hide resolved

Respect the json_provider_class

3f8c4b6

Respect the json_provider_class when available and stay compatible with flask < 2.2. Extended tox configuration to also test with flask < 2.2.

jrast force-pushed the flask23-compat branch from 8ff52ee to 3f8c4b6 Compare August 12, 2022 20:45

vimalloc reviewed Aug 12, 2022

View reviewed changes

flask_jwt_extended/view_decorators.py Outdated Show resolved Hide resolved

flask_jwt_extended/internal_utils.py Outdated Show resolved Hide resolved

jrast added 3 commits August 12, 2022 22:53

fixed tox ini: Test against Flask < 2.2 and latest flask

48db40b

Removed TODO including code in question

6076e2c

The code in question was a runtime check which is no longer necesary.

Added tests and a more sane way for the custom JSONEncoder

ed86799

- Added tests to check the config.json_encoder - Don't create the JSONEncoder class on the fly, instead get the JSON provider class at runtime.

Seluj78 approved these changes Aug 14, 2022

View reviewed changes

vimalloc approved these changes Aug 15, 2022

View reviewed changes

vimalloc merged commit 88a628e into vimalloc:master Aug 15, 2022

jrast deleted the flask23-compat branch August 15, 2022 20:03

dpgaspar mentioned this pull request May 13, 2024

fix: jwt extended broken by flask bump apache/superset#28451

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility changes for upcoming flask version 2.3 #493

Compatibility changes for upcoming flask version 2.3 #493

jrast commented Aug 7, 2022

vimalloc commented Aug 7, 2022 •

edited

Loading

jrast commented Aug 7, 2022

tgross35 left a comment

vimalloc left a comment

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

Seluj78 left a comment

vimalloc left a comment

Seluj78 commented Aug 15, 2022

vimalloc commented Aug 15, 2022

Compatibility changes for upcoming flask version 2.3 #493

Compatibility changes for upcoming flask version 2.3 #493

Conversation

jrast commented Aug 7, 2022

vimalloc commented Aug 7, 2022 • edited Loading

jrast commented Aug 7, 2022

tgross35 left a comment

Choose a reason for hiding this comment

vimalloc left a comment

Choose a reason for hiding this comment

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

jrast commented Aug 12, 2022

vimalloc commented Aug 12, 2022

Seluj78 left a comment

Choose a reason for hiding this comment

vimalloc left a comment

Choose a reason for hiding this comment

Seluj78 commented Aug 15, 2022

vimalloc commented Aug 15, 2022

vimalloc commented Aug 7, 2022 •

edited

Loading