freedomofpress / securedrop

GitHub repository for the SecureDrop whistleblower platform. Do not submit tips here!
https://securedrop.org/
Other
3.63k stars 686 forks source link

Update to Flask 3.0.0 #6963

Open legoktm opened 1 year ago

legoktm commented 1 year ago

Flask 3.0.0 is out! This is likely going to trigger updates for a number of other dependencies too...some notes inline.

legoktm commented 1 year ago

It might make sense to jump to Flask 2.2.0 as an interim step, since that seems to introduce a lot of new functionality (JSON provider, some config attributes) where the old way is being removed in 3.0.

Flask-Babel also has a good number of changes that seem incompatible with how we use it, specifically that various lookups can now no longer be done during setup...we may need to ask upstream for some tweaks.

I've pushed a branch with how far I got by just quickly trying it: https://github.com/freedomofpress/securedrop/tree/flask-3

TkTech commented 1 year ago

Flask-Babel also has a good number of changes that seem incompatible with how we use it, specifically that various lookups can now no longer be done during setup...we may need to ask upstream for some tweaks.

I don't see anything from a quick skim of this project that would be fundamentally incompatible. Just switch to using Babel.init_app to pass your settings in your i18n.py and you should be good to go.

legoktm commented 1 year ago

Hi @TkTech! Thanks for your work on Flask-Babel and for being proactive in finding this issue :) I already made a few changes along those lines (see https://github.com/freedomofpress/securedrop/commit/08d6704bc592d12c1d7306c2fd4f9e6dcf6d3796) but here's what I ran into using the flask-3 branch of this repo:

$ make dev
...
Traceback (most recent call last):
  File "./manage.py", line 452, in <module>
    _run_from_commandline()
  File "./manage.py", line 442, in _run_from_commandline
    rc = args.func(args)
  File "./manage.py", line 99, in reset
    with context or app_context():
  File "/usr/lib/python3.8/contextlib.py", line 113, in __enter__
    return next(self.gen)
  File "/home/user/github/freedomofpress/securedrop/securedrop/management/__init__.py", line 11, in app_context
    with journalist_app.create_app(config).app_context():
  File "/home/user/github/freedomofpress/securedrop/securedrop/journalist_app/__init__.py", line 93, in create_app
    i18n.configure(config, app)
  File "/home/user/github/freedomofpress/securedrop/securedrop/i18n.py", line 199, in configure
    usable_locales = validate_locale_configuration(config, babel)
  File "/home/user/github/freedomofpress/securedrop/securedrop/i18n.py", line 137, in validate_locale_configuration
    available = set(babel.list_translations())
  File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask_babel/__init__.py", line 188, in list_translations
    for dirname in get_babel().translation_directories:
  File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask_babel/__init__.py", line 44, in get_babel
    if not hasattr(app, 'extensions'):
  File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/werkzeug/local.py", line 311, in __get__
    obj = instance._get_current_object()
  File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/werkzeug/local.py", line 508, in _get_current_object
    raise RuntimeError(unbound_message) from None
RuntimeError: Working outside of application context.

So in the process of creating our app, we call babel.list_translations(), which following https://github.com/python-babel/flask-babel/commit/9d12cc979c8829ac1270dd049954a7be5f2370d6, calls get_babel(), which tries to get the current app. Except no app exists yet, because we're still creating it.

I was going to verify that my analysis was correct before proposing to you that list_translations() optionally take app, which could be passed through get_babel() so it could avoid trying to access the current app. Does that seem reasonable? I also haven't tested whether such a fix would be sufficient for us, or if we're going to hit some further roadblock. Also if you have a different suggestion on how we could implement our validate_locale_configuration(), that would be appreciated!

TkTech commented 1 year ago

I see. Ideally you shouldn't need to do that check, but to get it working you can probably do either:

  1. It only needs an app context, so do with app.app_context() before your call, since you have a valid app object at this point.
  2. Take a look at the app.extensions['babel'] object, which is a BabelConfiguration object that has the list of translation directories on it. Since this is an implementation detail, I'd recommend using 1.
legoktm commented 1 year ago

so do with app.app_context() before your call, since you have a valid app object at this point.

Oh, duh. This worked perfectly, thanks :)

legoktm commented 1 month ago

I started picking this up again to see if it'll help with the noble migration. I don't think it's strictly necessary, but there are a lot of deprecation warnings on Python 3.12 with our current set of dependencies. I'm planning to keep slowly picking away at this but it's not a priority.

I've been pushing stuff to the flask-3 branch, and pulling it out into separate PRs if it doesn't actually require the version bumps (e.g. #7252, #7264).

The part that I'm most worried about right now is that there are changes that affect our custom session implementation. I think the impact is just in tests, but AFAICT it requires tweaking how our sessions are constructed, which I tried to do in https://github.com/freedomofpress/securedrop/commit/2587e56130d6b26dbfb6fca82f4b32918ea933ed, but that's causing other issues and I'm sure there are logic paths I didn't fully implement correctly.