fix: pass if table is already removed on upgrade #30017
Conversation
sadpandajoe
requested review from
mistercrunch,
michael-s-molina,
betodealmeida and
eschutho
as code owners
dosubot
bot
added
change:backend
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.
@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.
yes, and/or change drop_fks_for_table to receive a fail_when_missing=False
There was a problem hiding this comment.
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.
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.
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.
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.
Can you move this outside the upgrade method (below down_revision variable) and reuse it on downgrade?
There was a problem hiding this comment.
Please do this for all changed migrations.
sadpandajoe
added
the
v4.1
(cherry picked from commit c929f5e)
sadpandajoe
removed
the
risk:breaking-change
|
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_tablefunction to return a boolean if the table exist or notdrop_fks_for_tableto handle sqlite in the functionBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION