Skip to content

chore: Bump flask libs#22355

Merged
dpgaspar merged 9 commits into
apache:masterfrom
EugeneTorap:chore/bump-python-libs
Jan 9, 2023
Merged

chore: Bump flask libs#22355
dpgaspar merged 9 commits into
apache:masterfrom
EugeneTorap:chore/bump-python-libs

Conversation

@EugeneTorap
Copy link
Copy Markdown
Contributor

@EugeneTorap EugeneTorap commented Dec 7, 2022

SUMMARY

The PR bump the next flask libs:

  • flask to 2.1.3
  • flask-compress to 1.13
  • flask-login to 0.6.0
  • flask-talisman to 1.0.0
  • flask-wtf to 1.0.1
  • werkzeug to 2.1.2

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Comment thread requirements/base.txt Outdated
Comment thread requirements/base.txt Outdated
Comment thread requirements/base.txt Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 7, 2022

Codecov Report

Merging #22355 (a5fa09d) into master (6e4d6e5) will increase coverage by 0.00%.
The diff coverage is 96.00%.

❗ Current head a5fa09d differs from pull request most recent head 3f85e77. Consider uploading reports for the commit 3f85e77 to get more accurate results

@@           Coverage Diff           @@
##           master   #22355   +/-   ##
=======================================
  Coverage   67.00%   67.01%           
=======================================
  Files        1859     1859           
  Lines       71019    71037   +18     
  Branches     7766     7766           
=======================================
+ Hits        47587    47605   +18     
  Misses      21410    21410           
  Partials     2022     2022           
Flag Coverage Δ
hive 52.46% <43.47%> (-0.01%) ⬇️
mysql 78.00% <95.65%> (+0.01%) ⬆️
postgres 78.07% <95.65%> (+0.01%) ⬆️
presto 52.36% <43.47%> (-0.01%) ⬇️
python 81.33% <95.65%> (+<0.01%) ⬆️
sqlite 76.45% <95.65%> (+0.01%) ⬆️
unit 51.46% <43.47%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
superset/views/core.py 74.96% <75.00%> (-0.02%) ⬇️
...c/components/Chart/DrillDetail/DrillDetailPane.tsx 76.04% <100.00%> (ø)
...mponents/DataTablesPane/components/SamplesPane.tsx 97.67% <100.00%> (ø)
superset/config.py 91.66% <100.00%> (+0.12%) ⬆️
superset/dashboards/api.py 92.57% <100.00%> (ø)
superset/views/filters.py 100.00% <100.00%> (ø)
superset/views/datasource/schemas.py 97.22% <0.00%> (-2.78%) ⬇️
superset/views/base.py 76.97% <0.00%> (+0.68%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Dec 7, 2022
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Dec 7, 2022
Comment thread tests/integration_tests/core_tests.py Outdated
Copy link
Copy Markdown
Contributor Author

@EugeneTorap EugeneTorap Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Werkzeug 2.1.0 changelog:

Response.autocorrect_location_header is disabled by default. The Location header URL will remain relative, and exclude the scheme and domain, by default. pallets/werkzeug#2352

When Response.autocorrect_location_header was added in 2011 pallets/werkzeug@0ec643b, it was documented as "correct the location header to be RFC conformant". It was referring to RFC 2616. That was superseded by RFC 7231 in 2014, which allows relative URLs. MDN lists all browsers as compliant with this. Switch autocorrect_location_header to be disabled by default.

Copy link
Copy Markdown
Contributor Author

@EugeneTorap EugeneTorap Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Werkzeug 2.1.0 changelog:

Request.get_json() will raise a 400 BadRequest error if the Content-Type header is not application/json. This makes a very common source of confusion more visible. pallets/werkzeug#2339

If there's no json param or json=None then 400 http status will be thrown

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Dec 8, 2022
@dpgaspar
Copy link
Copy Markdown
Member

dpgaspar commented Dec 16, 2022

Seeing that GHSA-7wxw-4483-3m34 is a disputed CVE, and only occurs when running Flask with the provided dev server, something you should never do on production for several reasons.

regarding flask-caching I don't see on the CVE any submitted patches, GHSA-656c-6cxf-hvcv. This CVE is disputed also, since an attacker would have to be able to write to the cache itself, something that would cause severe issues all around not just by deserializing content with Pickle.

Can you please just bump Pillow and resolve the current PR conflicts? Thank you

@EugeneTorap EugeneTorap changed the title chore: Bump flask, cachelib and Pillow chore: Bump flask libs Dec 21, 2022
@EugeneTorap
Copy link
Copy Markdown
Contributor Author

@dpgaspar I've created a separate PR for Pillow - #22489
This PR will be for updating flask libs.

# Conflicts:
#	tests/integration_tests/thumbnails_tests.py
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Dec 24, 2022
Copy link
Copy Markdown
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, final comments

app.config.get("SQLALCHEMY_EXAMPLES_URI")
or app.config["SQLALCHEMY_DATABASE_URI"]
)
backend = db_uri.split("://")[0]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an unrelated change, no?

Copy link
Copy Markdown
Contributor Author

@EugeneTorap EugeneTorap Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the related change. When you want to run the local tests with SQLALCHEMY_EXAMPLES_URI = None you will have an exception here and I fix the issue.

URI to database storing the example data, points to
SQLALCHEMY_DATABASE_URI by default if set to None

)
rv = test_client.post(uri, json=None)
assert rv.status_code == 200
assert rv.status_code == 400
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this changed to a 400 now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Werkzeug 2.1.0 changelog:

Request.get_json() will raise a 400 BadRequest error if the Content-Type header is not application/json. This makes a very common source of confusion more visible. pallets/werkzeug#2339

If there's no json param or json=None then 400 http status will be thrown

@EugeneTorap
Copy link
Copy Markdown
Contributor Author

@dpgaspar @villebro I have fixed that 400 'Bad Request' problem for datasource/samples [POST] API.
Can you take a look and I guess we can merge the PR.

@dpgaspar
Copy link
Copy Markdown
Member

dpgaspar commented Jan 9, 2023

Great work! @EugeneTorap

@dpgaspar dpgaspar merged commit 30dab3a into apache:master Jan 9, 2023
@EugeneTorap EugeneTorap deleted the chore/bump-python-libs branch January 9, 2023 11:04
@cwegener
Copy link
Copy Markdown
Contributor

@EugeneTorap @dpgaspar Seeing that the compiled .txt lock files are manually edited in this change as well, is it correct to assume that pip-compile-multi is now deprecated and no longer used? Will it be replaced with an alternative any time soon?

@EugeneTorap
Copy link
Copy Markdown
Contributor Author

Hi @cwegener! I want to prepare a PR with poetry instead of pip-compile-multi usage for dependency management.

@villebro
Copy link
Copy Markdown
Member

@cwegener @EugeneTorap the lock files should preferably be updated via pip-compile-multi, and IMO it does work quite ok. Having said that, I'd love to see us move over to poetry, but I think it's a pretty big task and requires proper coordination to avoid unnecessary regressions or general back and forth.

@cwegener
Copy link
Copy Markdown
Contributor

I find that poetry's dependency management is not quite reliable enough for large codebases.

Anyway. Sounds like there is a need for either discussion of the Python dependency management options, or a better checking of consistent pip-compile-multi usage.

@villebro
Copy link
Copy Markdown
Member

I find that poetry's dependency management is not quite reliable enough for large codebases.

Anyway. Sounds like there is a need for either discussion of the Python dependency management options, or a better checking of consistent pip-compile-multi usage.

We should probably add a check to to our linting workflow ensure the compiled lock files are correct. Should be easy.

@john-bodley john-bodley mentioned this pull request Jul 11, 2023
9 tasks
@mistercrunch mistercrunch added the 🚢 2.1.3 First shipped in 2.1.3 label Feb 18, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 First shipped in 2.1.0 and removed 🚢 2.1.3 First shipped in 2.1.3 labels Mar 13, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0 First shipped in 2.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants