-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: rename to schemas_allowed_for_file_upload in dbs.extra #17323
Conversation
There was a problem hiding this 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
superset/migrations/versions/b92d69a6643c_rename_csv_to_file.py
Outdated
Show resolved
Hide resolved
superset/migrations/versions/b92d69a6643c_rename_csv_to_file.py
Outdated
Show resolved
Hide resolved
Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Same here, this should be in a new new migration script. |
There was a problem hiding this 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.
@exemplary-citizen can you confirm that: Also, can you run the script |
yeah upgrade and downgrade work as expected. here are the results for the benchmark:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* 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>
SUMMARY
db migration in #16756 was not suffice. The
dbs.extra
JSON encoded field needs to be updated given that theschemas_allowed_for_csv_upload
field was renamed toschemas_allowed_for_file_upload
.TESTING INSTRUCTIONS
Watch CI and any suggestions from reviewers
ADDITIONAL INFORMATION
cc: @john-bodley