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: DB exported with incorrect type #16037

Merged
merged 2 commits into from
Aug 3, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Aug 2, 2021

SUMMARY

Some databases were saved with schemas_allowed_for_csv_upload as a JSON string, instead of a JSON object. When these databases are exported the ZIP payload has the incorrect schema, and can't be imported.

This PR fixes the export, so that these databases are exported with the correct schema; and the import, so that any databases that were eventually exported can be imported.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Before applying this PR, create a DB using the new custom form. Check that the DB is saved with incorrect extra:

 {"metadata_params":{},"engine_params":{},"schemas_allowed_for_csv_upload":"[]"}

Note that the payload is incorrect becase schemas_allowed_for_csv_upload should be an array, not a string. One way to save a DB with the incorrect format like above is to add a schema, save the DB, remove the schema, and save the DB again.

Export the DB, the YAML will look like this:

database_name: MySQL
sqlalchemy_uri: mysql+mysqldb://root@localhost:3306/MySQL
cache_timeout: null
expose_in_sqllab: true
allow_run_async: false
allow_ctas: false
allow_cvas: false
allow_csv_upload: false
extra:
  metadata_params: {}
  engine_params: {}
  schemas_allowed_for_csv_upload: '[]'
uuid: 8bf58f76-c884-49d2-8612-d94d3ca1d1aa
version: 1.0.0

When importing the file it will fail the validation, because schemas_allowed_for_csv_upload is a string.

With this PR, the import will work. Additionally, when exporting the incorrectly saved database the output YAML will look like this:

database_name: MySQL
sqlalchemy_uri: mysql+mysqldb://root@localhost:3306/MySQL
cache_timeout: null
expose_in_sqllab: true
allow_run_async: false
allow_ctas: false
allow_cvas: false
allow_csv_upload: false
extra:
  metadata_params: {}
  engine_params: {}
  schemas_allowed_for_csv_upload: []
uuid: 8bf58f76-c884-49d2-8612-d94d3ca1d1aa
version: 1.0.0

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

self, data: Dict[str, Any], **kwargs: Any
) -> Dict[str, Any]:
"""
Fix ``schemas_allowed_for_csv_upload`` being a string.
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to run a migration/script to update the data in the table directly? I assume the size of these tables is relatively small.

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure! But I'm planning to work on a separate PR that fixes schemas_allowed_for_csv_upload being stored incorrectly, I thought of doing the migration there.

Copy link
Member

@eschutho eschutho Aug 2, 2021

Choose a reason for hiding this comment

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

also, are we fixing the fe bug separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! :)

Comment on lines +42 to +47
# Fix for DBs saved with an invalid ``schemas_allowed_for_csv_upload``
schemas_allowed_for_csv_upload = extra.get("schemas_allowed_for_csv_upload")
if isinstance(schemas_allowed_for_csv_upload, str):
extra["schemas_allowed_for_csv_upload"] = json.loads(
schemas_allowed_for_csv_upload
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't be needed after we apply the migration fixing the DB, but for now we need it.

@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #16037 (7d7192b) into master (c8a8347) will increase coverage by 0.00%.
The diff coverage is 72.22%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #16037   +/-   ##
=======================================
  Coverage   76.66%   76.67%           
=======================================
  Files         995      995           
  Lines       52784    52820   +36     
  Branches     6695     6695           
=======================================
+ Hits        40467    40498   +31     
- Misses      12092    12097    +5     
  Partials      225      225           
Flag Coverage Δ
mysql 81.57% <72.22%> (-0.04%) ⬇️
postgres 81.64% <72.22%> (+<0.01%) ⬆️
python 81.73% <72.22%> (+<0.01%) ⬆️
sqlite 81.28% <72.22%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
superset/databases/commands/export.py 90.47% <66.66%> (-3.81%) ⬇️
superset/databases/schemas.py 98.38% <83.33%> (-0.38%) ⬇️
superset/common/query_context.py 90.74% <0.00%> (-0.47%) ⬇️
superset/config.py 91.24% <0.00%> (ø)
superset/models/slice.py 86.18% <0.00%> (ø)
superset/connectors/sqla/models.py 88.06% <0.00%> (+0.06%) ⬆️
superset/utils/webdriver.py 81.81% <0.00%> (+2.33%) ⬆️

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 c8a8347...7d7192b. Read the comment docs.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

LGTM!

@betodealmeida betodealmeida merged commit 7b15b76 into apache:master Aug 3, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* fix: DB exported with incorrect type

* Fix export as well
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* fix: DB exported with incorrect type

* Fix export as well
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* fix: DB exported with incorrect type

* Fix export as well
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* fix: DB exported with incorrect type

* Fix export as well
@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 13, 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/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants