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

fix: Pin itsdangerous #14627

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented May 13, 2021

SUMMARY

Version 2.0.0 of the itsdangerous package was recently released (May 12, 2021) which replaces the simplejson with json which cannot decode decimal.Decimal objects, i.e.,

Traceback (most recent call last):
...
  File "/srv/superset-internal/superset-fork/superset/utils/core.py", line 1407, in time_function
    response = func(*args, **kwargs)
  File "/usr/local/lib/python3.8/dist-packages/flask_appbuilder/api/__init__.py", line 1499, in get_list_headless
    return self.response(200, **_response)
...
  File "/usr/lib/python3.8/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type Decimal is not JSON serializable

Eventually when we update Flask we will need to deal with this as it also removes simplejson (pallets/flask#3555) and doesn't rely on on itsdangerous for defining which JSON package to use per pallets/flask#3562.

I tried using the logic they suggested (to help future proof ourselves), i.e., in superset/app.py

from simplejson import JSONEncoder


app = Flask(__name__)
app.json_encoder = JSONEncoder

however I ran into a somewhat similar problem,

Traceback (most recent call last):
...
  File "/srv/superset-internal/superset-fork/superset/utils/core.py", line 1407, in time_function
    response = func(*args, **kwargs)
  File "/usr/local/lib/python3.8/dist-packages/flask_appbuilder/api/__init__.py", line 1499, in get_list_headless
    return self.response(200, **_response)
...
  File "/usr/local/lib/python3.8/dist-packages/simplejson/encoder.py", line 272, in default
    raise TypeError('Object of type %s is not JSON serializable' %
TypeError: Object of type Table is not JSON serializable

which is somewhat perplexing, i.e., I'm not sure why this is problematic and I couldn't find any JSON encoders in Superset, Flask, or Flask-SQLAlchemy which handled serializing SQLAlchemy Table objects. For now to remedy any issues resulting from pip installing Apache Superset I thought there was merit in simply restricting the version of itsdangerous.

Note the generated requirements were updated via,

pip-compile-multi --only-name base --upgrade-package itsdangerous 

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #14627 (2c8bc91) into master (3a81e6a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14627   +/-   ##
=======================================
  Coverage   77.38%   77.38%           
=======================================
  Files         959      959           
  Lines       48467    48467           
  Branches     5678     5678           
=======================================
  Hits        37508    37508           
  Misses      10759    10759           
  Partials      200      200           
Flag Coverage Δ
hive 80.96% <ø> (ø)
mysql 81.24% <ø> (ø)
postgres 81.26% <ø> (ø)
python 81.63% <ø> (ø)
sqlite 80.88% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a81e6a...2c8bc91. Read the comment docs.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Agree, I think it's good to freeze this before it breaks anything.

@john-bodley john-bodley changed the title fix: Use simplejson for Flask JSON encoding fix: Pin itsdangerous May 13, 2021
@john-bodley john-bodley force-pushed the john-bodley--pin-itsdangerous branch from 2c8bc91 to da70b3f Compare May 14, 2021 01:36
@john-bodley john-bodley force-pushed the john-bodley--pin-itsdangerous branch from da70b3f to 1504e51 Compare May 14, 2021 02:01
@john-bodley john-bodley merged commit 2bd0b62 into apache:master May 14, 2021
@john-bodley john-bodley deleted the john-bodley--pin-itsdangerous branch May 14, 2021 03:14
john-bodley added a commit to airbnb/superset-fork that referenced this pull request May 14, 2021
Co-authored-by: John Bodley <john.bodley@airbnb.com>
(cherry picked from commit 2bd0b62)
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
Co-authored-by: John Bodley <john.bodley@airbnb.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
Co-authored-by: John Bodley <john.bodley@airbnb.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Co-authored-by: John Bodley <john.bodley@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
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/XS 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants