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: rename to schemas_allowed_for_file_upload in dbs.extra #17323

Merged

Conversation

exemplary-citizen
Copy link
Contributor

@exemplary-citizen exemplary-citizen commented Nov 2, 2021

SUMMARY

db migration in #16756 was not suffice. The dbs.extra JSON encoded field needs to be updated given that the schemas_allowed_for_csv_upload field was renamed to schemas_allowed_for_file_upload.

TESTING INSTRUCTIONS

Watch CI and any suggestions from reviewers

ADDITIONAL INFORMATION

  • Has associated issue: Closes insufficient db migration in #16756 #17294
  • 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

cc: @john-bodley

@exemplary-citizen exemplary-citizen requested a review from a team as a code owner November 2, 2021 15:26
@superset-github-bot superset-github-bot bot added the dropbox Dropbox related label Nov 2, 2021
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

@betodealmeida or @villebro any thoughts regarding whether we should be augmenting a previous migration vs. creating a new one given it’s been about a week since the original migration was authored? I’m likely leaning towards the later.

for database in session.query(Database).all():
try:
extra = json.loads(database.extra)
except json.decoder.JSONDecodeError as ex:
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm is this the only exception that json.loads(…) can raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could technically raise a TypeError if database.extra isn't one of str, bytes or bytearray

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #17323 (0f375da) into master (8756c90) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17323      +/-   ##
==========================================
+ Coverage   76.95%   77.02%   +0.07%     
==========================================
  Files        1039     1039              
  Lines       56033    56033              
  Branches     7735     7735              
==========================================
+ Hits        43120    43162      +42     
+ Misses      12655    12613      -42     
  Partials      258      258              
Flag Coverage Δ
hive 81.49% <ø> (ø)
mysql 81.92% <ø> (ø)
postgres 81.93% <ø> (ø)
presto 81.79% <ø> (?)
python 82.42% <ø> (+0.14%) ⬆️
sqlite 81.60% <ø> (ø)

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

Impacted Files Coverage Δ
superset/models/core.py 90.00% <0.00%> (+0.73%) ⬆️
superset/connectors/sqla/models.py 87.97% <0.00%> (+1.38%) ⬆️
superset/db_engine_specs/presto.py 89.95% <0.00%> (+5.64%) ⬆️

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 8756c90...0f375da. Read the comment docs.

@betodealmeida
Copy link
Member

@betodealmeida or @villebro any thoughts regarding whether we should be augmenting a previous migration vs. creating a new one given it’s been about a week since the original migration was authored? I’m likely leaning towards the later.

Same here, this should be in a new new migration script.

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.

Let's do this in a separate migration, in case someone has already applied the first one.

@betodealmeida
Copy link
Member

@exemplary-citizen can you confirm that:

Screen Shot 2021-11-12 at 6 21 45 PM

Also, can you run the script https://github.com/apache/superset/blob/master/scripts/benchmark_migration.py to benchmark the migration?

@exemplary-citizen
Copy link
Contributor Author

yeah upgrade and downgrade work as expected. here are the results for the benchmark:

Importing migration script: superset/migrations/versions/0ca9e5f1dacd_rename_to_schemas_allowed_for_file_.py
Migration goes from b92d69a6643c to 0ca9e5f1dacd
Current version of the DB is 0ca9e5f1dacd

Identifying models used in the migration:
- ab_user (0 rows in table ab_user)
- dbs (0 rows in table dbs)
Benchmarking migration
Migration on current DB took: 0.31 seconds
Running with at least 10 entities of each model
- Adding 10 entities to the ab_user model
- Adding 10 entities to the dbs model
Migration for 10+ entities took: 0.28 seconds
Running with at least 100 entities of each model
- Adding 90 entities to the ab_user model
- Adding 90 entities to the dbs model
Migration for 100+ entities took: 0.29 seconds
Running with at least 1000 entities of each model
- Adding 900 entities to the ab_user model
- Adding 900 entities to the dbs model
Migration for 1000+ entities took: 0.49 seconds

Results:

Current: 0.31 s
10+: 0.28 s
100+: 0.29 s
1000+: 0.49 s
Cleaning up DB

@betodealmeida betodealmeida added the risk:db-migration PRs that require a DB migration label Nov 14, 2021
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.

Thanks!

@betodealmeida betodealmeida merged commit 0ca4312 into apache:master Nov 14, 2021
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* rename to schemas_allowed_for_file_upload in dbs.extra

* black

* I should really setup pre-commit hooks

* Apply suggestions

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>

* move changes to a seperate migration

* fix spaces

* black

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.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 dropbox Dropbox related risk:db-migration PRs that require a DB migration size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

insufficient db migration in #16756
4 participants