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(sql lab): deleting the last saved query or the last executed from history #19225

Merged

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

When the SQL Lab has the persistence enabled on the backend (SQLLAB_BACKEND_PERSISTENCE), the queries failed on delete in two cases:

  • From the Query History tab, if the query was the last executed.
  • From the Saved queries, if the query was the last executed in a tab.

The issue is rooted in the TabState table, which has a foreign key to the saved query and to the latest executed one.
So, we need to clear that relation before deleting the query.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Query History

Before:

CleanShot.2021-05-19.at.22.12.13.mp4

After:

Screen.Recording.2022-03-16.at.17.27.35.mov

Saved Query

Before:

Saved.query.mp4

After:

Screen.Recording.2022-03-16.at.17.28.42.mov

TESTING INSTRUCTIONS

Saved query:

  • Go to SQL editor
  • Input a valid sql query using examples or your own db
  • Run query and Save as a new query (don't close the saved query tab in SQL editor)
  • Go to "Saved queries" page
  • Try to delete the saved query
  • See the error

Query History:

  • In SQL editor, run query
  • Navigate to query history
  • Click on trash icon

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

# Before deleting the query, ensure it's not tied to any
# active tab as the last query. If so, replace the query
# with the latest one created in that tab
tab_state_query = db.session.query(TabState).filter_by(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also set the latest_query_id to NULL, the same as saved_query_id, if we don't care about reassign the latest query.

@rusackas
Copy link
Member

Pinging @ktmud for 👀 on this one since he's looking at permanently enabling the SQLLAB_BACKEND_PERSISTENCE flag in #18898 so this may affect that.

@rusackas rusackas requested a review from ktmud March 17, 2022 02:40
@diegomedina248 diegomedina248 force-pushed the fix/sql-lab-delete-saved-history-query branch from 04550d3 to 776a96f Compare March 17, 2022 02:41
@eschutho eschutho requested a review from AAfghahi March 17, 2022 17:23
@AAfghahi
Copy link
Member

This looks good! Ping me when the tests are passing because I wanna do some frontend testing on it :)

@diegomedina248 diegomedina248 force-pushed the fix/sql-lab-delete-saved-history-query branch from 776a96f to db20638 Compare March 17, 2022 19:45
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #19225 (5cd81c7) into master (51061f0) will decrease coverage by 0.17%.
The diff coverage is 33.33%.

❗ Current head 5cd81c7 differs from pull request most recent head a1e2c11. Consider uploading reports for the commit a1e2c11 to get more accurate results

@@            Coverage Diff             @@
##           master   #19225      +/-   ##
==========================================
- Coverage   66.76%   66.59%   -0.18%     
==========================================
  Files        1670     1670              
  Lines       64392    64397       +5     
  Branches     6499     6499              
==========================================
- Hits        42991    42884     -107     
- Misses      19718    19830     +112     
  Partials     1683     1683              
Flag Coverage Δ
hive ?
mysql 81.96% <33.33%> (-0.01%) ⬇️
postgres 82.01% <33.33%> (-0.01%) ⬇️
presto ?
python 82.09% <33.33%> (-0.35%) ⬇️
sqlite 81.76% <33.33%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/views/sql_lab.py 59.33% <20.00%> (-1.36%) ⬇️
superset/models/sql_lab.py 91.55% <100.00%> (ø)
superset/db_engines/hive.py 0.00% <0.00%> (-85.19%) ⬇️
superset/db_engine_specs/hive.py 70.00% <0.00%> (-15.77%) ⬇️
superset/db_engine_specs/presto.py 83.47% <0.00%> (-5.65%) ⬇️
superset/connectors/sqla/models.py 88.87% <0.00%> (-1.24%) ⬇️
superset/db_engine_specs/base.py 89.14% <0.00%> (-0.34%) ⬇️
superset/models/core.py 88.80% <0.00%> (-0.24%) ⬇️
superset/utils/core.py 90.13% <0.00%> (-0.13%) ⬇️

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 51061f0...a1e2c11. Read the comment docs.

@diegomedina248 diegomedina248 force-pushed the fix/sql-lab-delete-saved-history-query branch from db20638 to a1e2c11 Compare March 17, 2022 20:08
@AAfghahi
Copy link
Member

/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=True

@github-actions
Copy link
Contributor

@AAfghahi Ephemeral environment spinning up at http://54.218.227.72:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@AAfghahi AAfghahi left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@rusackas rusackas merged commit aa5c80b into apache:master Mar 17, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@sadpandajoe
Copy link
Contributor

🏷️ preset:2022.11

villebro pushed a commit that referenced this pull request Apr 3, 2022
… history (#19225)

* fix: fix issue when deleting the last saved query or the last executed query

* merge migration

(cherry picked from commit aa5c80b)
@mistercrunch mistercrunch added 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.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 lts-v1 preset:2022.11 Preset-Patch size/M 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants