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

before_(app_)first_request deprecation and removal in Flask v2.3 #171

Closed
slint opened this issue Jan 12, 2023 · 12 comments · Fixed by #172 or inveniosoftware/invenio-app#88
Closed
Assignees

Comments

@slint
Copy link
Member

slint commented Jan 12, 2023

In Flask v2.2 before_(app_)first_request was deprecated and will be removed in v2.3.

We are currently using this through various modules for registering menus or other special cases:

We should either:

  • Pin Flask < 2.3 in this module
  • Refactor the code not to use these decorators. This is pretty difficult since the proposed alternative is to "Run setup code when creating the application instead."... This means we have to hook in the invenio-app lifecycle to introduce a similar mechanism.
@jrcastro2 jrcastro2 self-assigned this Jan 13, 2023
jrcastro2 added a commit to jrcastro2/invenio-base that referenced this issue Jan 13, 2023
* allows to pass entry points to execute
  functions just after the app is instantiated
* closes inveniosoftware#171
jrcastro2 added a commit to jrcastro2/invenio-app that referenced this issue Jan 13, 2023
jrcastro2 added a commit to jrcastro2/invenio-pages that referenced this issue Jan 13, 2023
@kpsherva kpsherva assigned ntarocco and utnapischtim and unassigned ntarocco and jrcastro2 Mar 27, 2023
@utnapischtim
Copy link
Contributor

NOTES:

  • would that solution also makes @blueprint.record_once decorators obsolete?

@slint
Copy link
Member Author

slint commented Apr 6, 2023

  • would that solution also makes @blueprint.record_once decorators obsolete?

I would say yes (at least from a quick search). The main difference with the before_(app_)first_request usage, is that it doesn't depend on an application context being pushed, but has access to the app via state.app. It still assumes that all extensions have been initialized though.

I think whenever we're using a current_XYZ in these function, we're assuming app context, but it's not necessary actually... If you have the app object, you can usually access any of these current_XYZ objects via app.extensions['<XYZ-module>'].XYZ.

@utnapischtim
Copy link
Contributor

  • would that solution also makes @blueprint.record_once decorators obsolete?

I would say yes (at least from a quick search). The main difference with the before_(app_)first_request usage, is that it doesn't depend on an application context being pushed, but has access to the app via state.app. It still assumes that all extensions have been initialized though.

I think whenever we're using a current_XYZ in these function, we're assuming app context, but it's not necessary actually... If you have the app object, you can usually access any of these current_XYZ objects via app.extensions['<XYZ-module>'].XYZ.

Good to know. thank you.

Should i include those examples in the affected list?

Is the proposed method to fix that DeprecationWarning still under discussion or can i go further and apply it?

@slint
Copy link
Member Author

slint commented Apr 17, 2023

Should i include those examples in the affected list?

Yes, I think they're good candidates for this type of logic refactoring.

Is the proposed method to fix that DeprecationWarning still under discussion or can i go further and apply it?

#172 and inveniosoftware/invenio-app#88 are the "leading" implementations. Personally, I'm still a bit hesitant about the naming (I like more the finalize_init_app naming proposed in flask), but otherwise, I think that's the direction we're going with.

An alternative to adding yet another entrypoint, would be to move the logic in the extensions and introduce an optional finalize_init_app() method, which when exists gets called for all the extensions defined in the original inenio_app.apps and invenio_app.api_apps.

@utnapischtim
Copy link
Contributor

utnapischtim commented May 6, 2023

@slint
i need some advice/decision for flask-menu.

#i-would-like-to-refactore
The package was not refactored for a long time. I would like to apply the common invenio structure with the ext.py and proxies.py files. Migrate to setup.cfg and black. Further, as i researched, the classy.py file is not used within inveniosoftware, should we keep it or could we remove it?

#if-we-not-refactore-and-remove-classy
how do we proceed with with the before_first_request decorator in classy.py? In my opinion it is not worth the time to keep that functionality. We could release a new major version which indicates that the classy.py has been removed.

#rename-entrypoint
i would like to rename it to finalize_app_entry_points and then the entrypoint: invenio_base.finalize_app. in my eyes it describes the best what it should do. what do you think?

#structural-problem
there is a problem with the register_menu decorator. This decorator uses the before_first_request decorator and the register_menu decorator is used in 4-5 inveniosoftware packages. In my opinion the register_menu decorator should be removed and the usage of the decorator has to be refactored to use the newly introduced finalize_app_entry_points. My point is, if we keep the register_menu decorator we would have to find a solution where the decorator could still be used, but what the decorator does is done on initializing the app, which seems not possible in my eyes.

SIDENOTE: the register_breadcrumb from flask-breadcrumps is a register_menu wrapper, which means, this has also to be refactored (in the sense of removed)

This change would affect code like in invenio-accounts

SIDENOTE: is this in invenio-administration the example that it is not possible to replace all before_*_request decorators with the entrypoint method?

@slint
Copy link
Member Author

slint commented May 8, 2023

#i-would-like-to-refactore The package was not refactored for a long time. I would like to apply the common invenio structure with the ext.py and proxies.py files. Migrate to setup.cfg and black. Further, as i researched, the classy.py file is not used within inveniosoftware, should we keep it or could we remove it?

+1 for updating the structure and modernizing. What we have to keep in mind is that this is Flask-* package and not invenio-*, so we have to assume that there might be users in the wild. The best would be in any case to release a major bump with breaking changes if that's easier.

#if-we-not-refactore-and-remove-classy how do we proceed with with the before_first_request decorator in classy.py? In my opinion it is not worth the time to keep that functionality. We could release a new major version which indicates that the classy.py has been removed.

We can remove classy.py and support for Flask-Classy, and make a major version bump.

#rename-entrypoint i would like to rename it to finalize_app_entry_points and then the entrypoint: invenio_base.finalize_app. in my eyes it describes the best what it should do. what do you think?

I have a feeling that entrypoints are a bit "esoteric" at this point to how we do things in Invenio... Would it make sense to explore more towards what the discussion in Flask also suggested, i.e. a finalize_init_app method as part of the extensions "interface/convention"? We could then just call that method (if it exists) for each extension in the existing inenio_app.apps and invenio_app.api_apps entrypoints.

#structural-problem there is a problem with the register_menu decorator. This decorator uses the before_first_request decorator and the register_menu decorator is used in 4-5 inveniosoftware packages. In my opinion the register_menu decorator should be removed and the usage of the decorator has to be refactored to use the newly introduced finalize_app_entry_points. My point is, if we keep the register_menu decorator we would have to find a solution where the decorator could still be used, but what the decorator does is done on initializing the app, which seems not possible in my eyes.

This is a bigger problem in the way we integrate Flask-* modules in general in Invenio, since we usually initialize the extensions inside our own InvenioXYZ extensions, and thus deny using the common pattern of accessing methods of an un-initialized extension. See Flask-Talisman as an example, which is technically unusable in its advertised decorator-style usage.

I think we should/could solve this problem as well as part of this refactoring here.

SIDENOTE: the register_breadcrumb from flask-breadcrumps is a register_menu wrapper, which means, this has also to be refactored (in the sense of removed)
This change would affect code like in invenio-accounts

Yes, these two modules are very tightly coupled, and as an extension any module that adds to the user setting pages. I think before releasing any breaking changes we should first pin the dependency in invenio-theme (this is the first place where it's introduced, so thus a "coordinator module").

SIDENOTE: is this in invenio-administration the example that it is not possible to replace all before_*_request decorators with the entrypoint method?

This one looks replaceable actually, but depends on some "local" variables (i.e. view_class), so it might be a bit tricky.

@utnapischtim
Copy link
Contributor

#about-flask-menu
flask-menu depends on flask-classy which is last updated 2014 and superseded by flask-classful (last update 12.2021). flask-classful is not compatible with werkzeug >2.3.0. keeping the classy.py would increase the amount of time to refactore the whole code and make it compatible with werkzeug >=2.3.0 and flask >=2.3.0

the rest tomorrow

@utnapischtim
Copy link
Contributor

utnapischtim commented May 13, 2023

@slint
do we still support invenio-admin and invenio-deposit. as i know invenio-admin is deprecated in favor of invenio-administration and invenio-deposit had the last release 2019. i saw that invenio-deposit is a dependency of invenio-github

@ntarocco
Copy link

@slint do we still support invenio-admin and invenio-deposit. as i know invenio-admin is deprecated in favor of invenio-administration and invenio-deposit had the last release 2019. i saw that invenio-deposit is a dependency of invenio-github.

Correct: no more support to invenio-admin and invenio-deposit.
About invenio-github, I suspect it will be refactored soon (current or next sprint) to enable GitHub integration in the new ZenodoRDM. invenio-deposit will disappear.

If it is easy to fix invenio-admin, I would fix it anyway because we have other running services that still use invenio-admin :)

@utnapischtim
Copy link
Contributor

@slint
how should we do with flask-security-invenio? as the name indicates it is a flask package, but if i add an entrypoint to invenio_base it is not more independent to invenio-* packages.

@slint slint closed this as completed in aed5cd5 Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants