-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: pass if table is already removed on upgrade #30017
fix: pass if table is already removed on upgrade #30017
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #30017 +/- ##
===========================================
+ Coverage 60.48% 83.62% +23.13%
===========================================
Files 1931 529 -1402
Lines 76236 38277 -37959
Branches 8568 0 -8568
===========================================
- Hits 46114 32010 -14104
+ Misses 28017 6267 -21750
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if connection.dialect.name != "sqlite": | ||
drop_fks_for_table("sl_dataset_users") | ||
op.drop_table("sl_dataset_users") | ||
except sa.exc.NoSuchTableError: |
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.
@sadpandajoe Would you mind adding a method called has_table
to https://github.com/apache/superset/blob/master/superset/migrations/shared/utils.py to be reused in these and future migrations?
def has_table(table_name: str) -> bool:
"""
Check if a table exists in the database.
:param table_name: The table name
:returns: True iff the table exists
"""
insp = inspect(op.get_context().bind)
return insp.has_table(table_name)
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.
yes, and/or change drop_fks_for_table
to receive a fail_when_missing=False
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.
Even if we updated drop_fks_for_table
we'd have to write something to drop table too, else it'll fail won't it?
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.
Whatever makes sense, maybe drop_table(table_name, including_fks=[], only_if_it_exists=True)
or maybe it's for another PR, but a common issue is a migration that fails at step 2 and when you rerun it it fails at step one since step one can only run once (say a create table). Personally think we could use more of the "drop-table-but-only-if-it-exists" and "create-table-but-drop-it-first-if-it-exists" type utils to avoid getting people tangled up. We've had many people open issue with the message of "step 1 one of the migration fails, here's the stacktrace", only to realize that the issue was with a subsequent step.
op.drop_table("sl_columns") | ||
|
||
try: | ||
if connection.dialect.name != "sqlite": |
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.
also seems like the special handling for sqlite could be handled within drop_fks_for_table
. More generally would be great to have migrations that are simpler / self-healing / free-of-engine-specific-logic through better utils.
superset/migrations/shared/utils.py
Outdated
insp = inspect(op.get_context().bind) | ||
table_exists = insp.has_table(table_name) | ||
|
||
print(f"Table {table_name} exist: {table_exists}") |
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.
Maybe delete this print? If not, then:
print(f"Table {table_name} exist: {table_exists}") | |
print(f"Table {table_name} exists: {table_exists}") |
if connection.dialect.name != "sqlite": | ||
drop_fks_for_table("sl_table_columns") | ||
op.drop_table("sl_table_columns") | ||
table_name = "sl_table_columns" |
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.
Can you move this outside the upgrade
method (below down_revision
variable) and reuse it on downgrade
?
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.
Please do this for all changed migrations.
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.
done
(cherry picked from commit c929f5e)
Not a breaking change since this handles the case where if migration has already been run gracefully. |
SUMMARY
Looks like if you try to run the migrations but the sl_ tables don't already exist the migrations will fail. This will just pass the upgrade check if the table doesn't already exist.
has_table
function to return a boolean if the table exist or notdrop_fks_for_table
to handle sqlite in the functionBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION