fix: cannot delete a database if team member has SQL editor tab that uses that db #19243

diegomedina248 · 2022-03-17T19:24:59Z

SUMMARY

The TabState model has foreign key constraints on it that are not properly handled when the referred rows are deleted.
This causes, for instance, that a database fails to be deleted if the user has an SQL Lab editor opened with a query referencing that database.

This PR adds a database migration with ondelete instructions to properly handle cascading deletes when needed.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Team member A previously has a SQL editor tab with DB1
Team member B (or same user might work as well) try to delete DB1
When prompted, confirm the deletion

ADDITIONAL INFORMATION

Has associated issue:
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

Runtime estimates results (benchmark_migration.py)

Migration for 1000+ entities took: 2.53 seconds

Results:

Current: 2.51 s
10+: 2.27 s
100+: 2.24 s
1000+: 2.53 s

codecov · 2022-03-17T20:03:03Z

Codecov Report

Merging #19243 (0f79c12) into master (90dbe8d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 0f79c12 differs from pull request most recent head ffe55cb. Consider uploading reports for the commit ffe55cb to get more accurate results

@@            Coverage Diff             @@
##           master   #19243      +/-   ##
==========================================
- Coverage   66.58%   66.56%   -0.03%     
==========================================
  Files        1677     1677              
  Lines       64238    64238              
  Branches     6538     6538              
==========================================
- Hits        42773    42758      -15     
- Misses      19766    19781      +15     
  Partials     1699     1699

Flag	Coverage Δ
hive	`52.69% <100.00%> (ø)`
mysql	`81.90% <100.00%> (ø)`
postgres	`81.95% <100.00%> (ø)`
presto	`52.54% <100.00%> (ø)`
python	`82.34% <100.00%> (-0.05%)`	⬇️
sqlite	`?`

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

Impacted Files	Coverage Δ
superset/models/sql_lab.py	`91.55% <100.00%> (ø)`
superset/connectors/sqla/utils.py	`89.13% <0.00%> (-5.44%)`	⬇️
superset/db_engine_specs/sqlite.py	`91.89% <0.00%> (-5.41%)`	⬇️
superset/utils/celery.py	`86.20% <0.00%> (-3.45%)`	⬇️
superset/result_set.py	`96.77% <0.00%> (-1.62%)`	⬇️
superset/databases/commands/test_connection.py	`98.57% <0.00%> (-1.43%)`	⬇️
superset/utils/cache.py	`73.58% <0.00%> (-0.95%)`	⬇️
superset/models/slice.py	`84.95% <0.00%> (-0.49%)`	⬇️
superset/db_engine_specs/base.py	`88.00% <0.00%> (-0.34%)`	⬇️

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 90dbe8d...ffe55cb. Read the comment docs.

github-actions · 2022-03-17T21:02:47Z

⚠️ @diegomedina248 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2022-03-29T17:29:45Z

⚠️ @diegomedina248 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2022-03-30T05:34:32Z

⚠️ @diegomedina248 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

eschutho

LGTM. Can you add the migration script runtime values to the description too?

diegomedina248 · 2022-03-30T20:49:47Z

LGTM. Can you add the migration script runtime values to the description too?

@eschutho Sorry about that, added to the description.
There's a failing test on just one of the integrations (sqlite), which I don't know why it's happening, if you can take a look that'd be great.
Thanks!

github-actions · 2022-03-31T00:27:58Z

⚠️ @diegomedina248 Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

eschutho

question about the down revision change in the other migration.

superset/migrations/versions/58df9d617f14_add_on_saved_query_delete_tab_state_.py

eschutho · 2022-04-02T00:28:10Z

superset/migrations/versions/8b841273bec3_sql_lab_models_database_constraint_updates.py

+        )
+
+    with op.batch_alter_table("table_schema") as batch_op:
+        batch_op.drop_constraint(


for the sqlite errors here, it's because sqlite doesn't support foreign key deletion and actually these foreign key lookups are returning None. So you could either check that the foreign key exists before deleting it or only run these foreign key deletions if the db is not sqlite like:

if not isinstance(bind.dialect, SQLiteDialect):

so, in order to support sqlite, can we do that? Or do we need to rename this table, create the new one with the proper constraints and then delete the old one?

You can just not run the drop_constraint if the dialect is sqlite or if the constraint isn't found. I prefer the latter b/c it's more specific and descriptive, but either would work. There are a lot of instance of checking for SQLiteDialect in the codebase already. If you go with the latter, though, it would be something like this:

table_schema_id_constraint = generic_find_fk_constraint_name("table_schema", {"id"}, "dbs", insp) if table_schema_id_constraint: batch_op.drop_constraint(table_schema_id_constraint, type_="foreignkey")

The issue in this case is that I'm dropping the constraint here to add it again, with a different setup.
Will it work if the field has more than one constraint? seems odd to me, but if it does, then will do that.

Yeah, Sqlite has foreign keys turned off by default, so if you do the lookup and it's not found, then you should be fine.

eschutho · 2022-04-06T15:39:44Z

Looks great. Thanks @diegomedina248!

diegomedina248 · 2022-04-06T15:40:33Z

Looks great. Thanks @diegomedina248!

Thank you!

…uses that db (apache#19243) * fix: update sql lab models constraints to support db deletion * update with master * solve db migration conflict * fix tests (cherry picked from commit 350f21d)

sadpandajoe · 2022-04-06T18:22:06Z

🏷️ preset:2022.13

…uses that db (apache#19243) * fix: update sql lab models constraints to support db deletion * update with master * solve db migration conflict * fix tests

diegomedina248 requested a review from a team as a code owner March 17, 2022 19:25

superset-github-bot bot added the Preset-Patch label Mar 17, 2022

pull-request-size bot added the size/L label Mar 17, 2022

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch from f036a6c to c176a4e Compare March 17, 2022 19:38

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch from c176a4e to 9e670ae Compare March 17, 2022 20:14

diegomedina248 changed the title ~~fix: cannot delete a database if team member has SQL editor tab that uses that db~~ [WIP] fix: cannot delete a database if team member has SQL editor tab that uses that db Mar 17, 2022

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch 3 times, most recently from 899eea4 to c138e95 Compare March 17, 2022 20:51

eschutho approved these changes Mar 30, 2022

View reviewed changes

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch from c138e95 to 65985bf Compare March 30, 2022 20:42

diegomedina248 changed the title ~~[WIP] fix: cannot delete a database if team member has SQL editor tab that uses that db~~ fix: cannot delete a database if team member has SQL editor tab that uses that db Mar 30, 2022

eschutho requested changes Apr 1, 2022

View reviewed changes

superset/migrations/versions/58df9d617f14_add_on_saved_query_delete_tab_state_.py Outdated Show resolved Hide resolved

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch 2 times, most recently from e862de8 to 0f79c12 Compare April 2, 2022 00:14

eschutho reviewed Apr 2, 2022

View reviewed changes

diegomedina248 added 4 commits April 4, 2022 18:57

fix: update sql lab models constraints to support db deletion

ed0a61d

update with master

8e91afa

solve db migration conflict

20f2906

fix tests

7dc0fe0

diegomedina248 force-pushed the fix/sql-lab-update-models-to-support-db-deletion branch from ffe55cb to 7dc0fe0 Compare April 4, 2022 21:57

eschutho approved these changes Apr 6, 2022

View reviewed changes

eschutho merged commit 350f21d into apache:master Apr 6, 2022

superset-github-bot bot added the preset:2022.13 label Apr 6, 2022

mistercrunch added the 🚢 2.0.1 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 and removed 🚢 2.0.1 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cannot delete a database if team member has SQL editor tab that uses that db #19243

fix: cannot delete a database if team member has SQL editor tab that uses that db #19243

diegomedina248 commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022

github-actions bot commented Mar 29, 2022

github-actions bot commented Mar 30, 2022

eschutho left a comment

diegomedina248 commented Mar 30, 2022

github-actions bot commented Mar 31, 2022

eschutho left a comment

eschutho Apr 2, 2022

diegomedina248 Apr 2, 2022

eschutho Apr 4, 2022

diegomedina248 Apr 4, 2022

eschutho Apr 4, 2022

eschutho commented Apr 6, 2022

diegomedina248 commented Apr 6, 2022

sadpandajoe commented Apr 6, 2022

fix: cannot delete a database if team member has SQL editor tab that uses that db #19243

fix: cannot delete a database if team member has SQL editor tab that uses that db #19243

Conversation

diegomedina248 commented Mar 17, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Runtime estimates results (benchmark_migration.py)

codecov bot commented Mar 17, 2022 • edited Loading

Codecov Report

github-actions bot commented Mar 17, 2022

github-actions bot commented Mar 29, 2022

github-actions bot commented Mar 30, 2022

eschutho left a comment

Choose a reason for hiding this comment

diegomedina248 commented Mar 30, 2022

github-actions bot commented Mar 31, 2022

eschutho left a comment

Choose a reason for hiding this comment

eschutho Apr 2, 2022

Choose a reason for hiding this comment

diegomedina248 Apr 2, 2022

Choose a reason for hiding this comment

eschutho Apr 4, 2022

Choose a reason for hiding this comment

diegomedina248 Apr 4, 2022

Choose a reason for hiding this comment

eschutho Apr 4, 2022

Choose a reason for hiding this comment

eschutho commented Apr 6, 2022

diegomedina248 commented Apr 6, 2022

sadpandajoe commented Apr 6, 2022

diegomedina248 commented Mar 17, 2022 •

edited

Loading

codecov bot commented Mar 17, 2022 •

edited

Loading