openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
170 stars 75 forks source link

Fix Web API after Flask dependencies updates #1109

Closed sandcha closed 2 years ago

sandcha commented 2 years ago

Connected to openfisca/openfisca-france#1808 where test-api job fails on GitHub Actions.

Technical changes

Details

This PR fixes two issues:

🐛itsdangerous revision fixed by flask upgrade

This first one is about itsdangerous and was detected by the test-api job:

ImportError: OpenFisca is missing some dependencies to run the Web API: 'cannot import name 'json' from 'itsdangerous' (/opt/hostedtoolcache/Python/3.7.9/x64/lib/python3.7/site-packages/itsdangerous/__init__.py)'. To install them, run `pip install openfisca_core[web-api]`.

While, thanks to pipdeptree library we can see that:

$ pipdeptree -r --packages itsdangerous
(...)
itsdangerous==2.1.0 ⬅
  - Flask==1.1.2 [requires: itsdangerous>=0.24] ⬅
    - Flask-Cors==3.0.10 [requires: Flask>=0.9]

This was checked on master branch, openfisca-core v35.7.6.

Updating Flask to its last v1.x.x revision which is 1.1.4 resolves the issue (as described by this post in another context)

itsdangerous max revision is fixed and 2.1.0 becomes 1.1.0:

$ pipdeptree -r --packages itsdangerous
(...)
itsdangerous==1.1.0 ⬅
  - Flask==1.1.4 [requires: itsdangerous>=0.24,<2.0] ⬅
    - Flask-Cors==3.0.10 [requires: Flask>=0.9]

But Flask 1.1.2 to 1.1.4 raises a new MarkupSafe error.

🐛 markupsafe revision

The following error was detected after Flask update and by this job of this PR:

ImportError: Error importing plugin "tests.fixtures.appclient": OpenFisca is missing some dependencies to run the Web API: 'cannot import name 'soft_unicode' from 'markupsafe' (/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/markupsafe/__init__.py)'. To install them, run `pip install openfisca_core[web-api]`.

Knowing that markupsafe is a subdependency of flask:

$ pipdeptree -r --packages markupsafe
(...)
MarkupSafe==2.1.0 ⬅
  - Jinja2==2.11.3 [requires: MarkupSafe>=0.23]
    - Flask==1.1.4 [requires: Jinja2>=2.10.1,<3.0] ⬅
      - Flask-Cors==3.0.10 [requires: Flask>=0.9]

So, the question might be: should we bump markupsafe or jinja?

As the previous Flask==1.1.2 also requires MarkupSafe==2.1.0 through Jinja2==3.0.3:

$ pipdeptree -r --packages markupsafe
Warning!!! Possibly conflicting dependencies found:
(...)
MarkupSafe==2.1.0
  - Jinja2==3.0.3 [requires: MarkupSafe>=2.0]
    - Flask==1.1.2 [requires: Jinja2>=2.10.1]
      - Flask-Cors==3.0.10 [requires: Flask>=0.9]

The solution might neither be in returning to flask v1.1.2 nor bumping jinja revision to v3. So, this PR suggests to fix markupsafe revision for now (as suggested by this markupsafe issue) and removing this fix when flask is updated to v2+.

Finally you might ask why we didn't update Flask to v2+. This was suggested by dependabot here. This major bump might require syntax checks and the CI fails on another error (pytest: not found 🙃). This looks like a misleading old cache. So the choice made here was to fix Flask 1.x.x. usage and to tackle the Flask 2.x.x / CI issue in a next PR.

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 78.92% when pulling 06936a388b396baf2413779b1cb61ef5781e4b01 on bumpto-latest-flask-v1 into 752c3befde4ee2d5acb36cf5ae9a35ea2e9f976a on master.

sandcha commented 2 years ago

Thank you @eraviart!