fix: Exception handling for SQL Lab views #30897

michael-s-molina · 2024-11-11T23:24:30Z

SUMMARY

This PR adds the necessary rollback and responses for legacy SQL Lab views. The final solution should be moving these endpoints under the v1 namespace and use the command pattern for each operation with the @transaction annotation.

TESTING INSTRUCTIONS

CI should be sufficient.

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

michael-s-molina · 2024-11-11T23:25:03Z

superset/views/sql_lab/views.py

@@ -84,48 +85,56 @@ class TabStateView(BaseSupersetView):
    @has_access_api
    @expose("/", methods=("POST",))
    def post(self) -> FlaskResponse:
-        query_editor = json.loads(request.form["queryEditor"])


The relevant change is the try/catch block.

codecov · 2024-11-11T23:29:55Z

Codecov Report

Attention: Patch coverage is 29.41176% with 60 lines in your changes missing coverage. Please review.

Project coverage is 83.88%. Comparing base (76d897e) to head (b4db6b0).
Report is 1134 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/views/sql_lab/views.py	29.41%	60 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30897       +/-   ##
===========================================
+ Coverage   60.48%   83.88%   +23.39%     
===========================================
  Files        1931      536     -1395     
  Lines       76236    38952    -37284     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32673    -13441     
+ Misses      28017     6279    -21738     
+ Partials     2105        0     -2105

Flag	Coverage Δ
hive	`48.82% <9.41%> (-0.34%)`	⬇️
javascript	`?`
mysql	`76.71% <29.41%> (?)`
postgres	`76.83% <29.41%> (?)`
presto	`53.30% <9.41%> (-0.50%)`	⬇️
python	`83.88% <29.41%> (+20.39%)`	⬆️
sqlite	`76.29% <29.41%> (?)`
unit	`60.83% <1.17%> (+3.21%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

villebro · 2024-11-11T23:42:09Z

superset/views/sql_lab/views.py

-        if client_id := fields.get("latest_query_id"):
-            query = db.session.query(Query).filter_by(client_id=client_id).one_or_none()
-            if not query:
-                return self.json_response({"error": "Bad request"}, status=400)


I don't fully grasp why this was here in the first place (it would probably have been a good idea to add a comment here if this was in fact needed), but do you have any idea why

this was here?

it can be removed?

This was an additional validation for when a client ID is no longer valid, which would generate a 500 response due to a foreign key error. Now the logic will return the same 400 error but without the extra expensive query.

michael-s-molina · 2024-11-12T00:24:57Z

tests/integration_tests/core_tests.py

@@ -1013,7 +1013,6 @@ def test_tabstate_update(self):
        data = {"sql": json.dumps("select 1"), "latest_query_id": json.dumps(client_id)}
        response = self.client.put(f"/tabstateview/{tab_state_id}", data=data)
        assert response.status_code == 400
-        assert response.json["error"] == "Bad request"


Validating the status_code here is sufficient as now the error will contain database specific and more rich exceptions.

villebro

LGTM

villebro · 2024-11-12T00:35:48Z

tests/integration_tests/core_tests.py

@@ -1013,7 +1013,6 @@ def test_tabstate_update(self):
        data = {"sql": json.dumps("select 1"), "latest_query_id": json.dumps(client_id)}
        response = self.client.put(f"/tabstateview/{tab_state_id}", data=data)
        assert response.status_code == 400
-        assert response.json["error"] == "Bad request"


(cherry picked from commit c2885a1)

* fix: Exception handling for SQL Lab views (apache#30897) * fix: don't show metadata for embedded dashboards (apache#30875) * feat: add logging durations for screenshot async service (apache#30884) * refactor(Avatar): Migrate Avatar to Ant Design 5 (apache#30740) * refactor(input): Migrate Input component to Ant Design 5 (apache#30730) * fix(empty dashboards): Allow downloading a screenshot of an empty dashboard (apache#30767) Co-authored-by: Geido <60598000+geido@users.noreply.github.com> * chore(ci): add tai and michael to helm owners (apache#30925) * fix(helm): use submodule on helm release action (apache#30924) * docs: add Free2Move to INTHEWILD.md (apache#30930) * fix: blocks UI elements on right side (apache#30886) Co-authored-by: Evan Rusackas <evan@preset.io> * chore(deps): Migrate from `crate[sqlalchemy]` to `sqlalchemy-cratedb` (apache#29243) * chore: update change log, UPDATING.md and bug-report.yml for 4.1 release (apache#30915) * chore(docs): Update list of supported databases to include CrateDB (apache#30946) * feat(trino,presto): add missing time grains (apache#30926) * fix(Dashboard): Exclude edit param in async screenshot (apache#30962) * fix(Card): Use correct class names for Ant Design 5 Card component (apache#30964) * chore(Accessibility): Fix accessibility for 'Show x entries' dropdown in tables (apache#30818) * build(deps): bump cross-spawn from 7.0.3 to 7.0.6 in /superset-frontend/cypress-base (apache#30969) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(release validation): scripts now support RSA and EDDSA keys. (apache#30967) * build(deps): bump cross-spawn from 7.0.3 to 7.0.6 in /docs (apache#30970) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: add performance information to tooltip (apache#30948) * fix(helm-chart): Fix broken PodDisruptionBudget due to introduction of extraLabels. (apache#30966) * chore(helm): bumping app version to 4.1.1 in helm chart (apache#30918) * chore: add unit tests for `is_mutating()` (apache#31021) * chore(🦾): bump python numexpr 2.10.0 -> 2.10.1 (apache#31006) Co-authored-by: GitHub Action <action@github.com> * chore(🦾): bump python cffi 1.16.0 -> 1.17.1 (apache#31002) Co-authored-by: GitHub Action <action@github.com> * chore(🦾): bump python flask-babel subpackage(s) (apache#31000) Co-authored-by: GitHub Action <action@github.com> * fix(Dashboard): Retain colors when color scheme not set (apache#30646) * fix(explore): verified props is not updated (apache#31008) * chore: publish wheels (apache#30981) * docs: Embedded sdk (apache#30972) * fix(imports): import query_context for imports with charts (apache#30887) * docs: Update doc about CSV upload (apache#30867) Co-authored-by: Evan Rusackas <evan@preset.io> * chore(🦾): bump python cattrs 23.2.3 -> 24.1.2 (apache#30998) Co-authored-by: GitHub Action <action@github.com> * fix(dataset): use sqlglot for DML check (apache#31024) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> Co-authored-by: Maxime Beauchemin <maximebeauchemin@gmail.com> Co-authored-by: Mehmet Salih Yavuz <salih.yavuz@proton.me> Co-authored-by: Geido <60598000+geido@users.noreply.github.com> Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Co-authored-by: Paolo Terzi <PaoloTerzi@users.noreply.github.com> Co-authored-by: Sukuna <90980311+samarsrivastav@users.noreply.github.com> Co-authored-by: Evan Rusackas <evan@preset.io> Co-authored-by: Andreas Motl <andreas.motl@elmyra.de> Co-authored-by: Andreas Motl <andreas.motl@crate.io> Co-authored-by: Levis Mbote <111055098+LevisNgigi@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Christoph Keller <github@christophkeller.cc> Co-authored-by: lodu <48859312+lodu@users.noreply.github.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> Co-authored-by: David Hotham <david.hotham@microsoft.com> Co-authored-by: Giampaolo Capelli <giampaolo.capelli@gmail.com> Co-authored-by: Linden <zairrow@gmail.com> Co-authored-by: Seiya <20365512+seiyab@users.noreply.github.com> Co-authored-by: Asher Manangan <amanangan@powercosts.com>

(cherry picked from commit c2885a1)

fix: Exception handling for SQL Lab views

e8f66f9

michael-s-molina requested review from justinpark and villebro November 11, 2024 23:24

pull-request-size bot added the size/L label Nov 11, 2024

dosubot bot added the sqllab Namespace | Anything related to the SQL Lab label Nov 11, 2024

michael-s-molina commented Nov 11, 2024

View reviewed changes

villebro reviewed Nov 11, 2024

View reviewed changes

michael-s-molina force-pushed the fix-views-sql-lab branch from 907e452 to e8f66f9 Compare November 12, 2024 00:10

Just check the returned status

b4db6b0

michael-s-molina commented Nov 12, 2024

View reviewed changes

villebro approved these changes Nov 12, 2024

View reviewed changes

michael-s-molina merged commit c2885a1 into apache:master Nov 12, 2024
37 checks passed

michael-s-molina added a commit that referenced this pull request Nov 19, 2024

fix: Exception handling for SQL Lab views (#30897)

b1c7b81

(cherry picked from commit c2885a1)

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2024

fix: Exception handling for SQL Lab views (#30897)

b6eb5b6

(cherry picked from commit c2885a1)

sadpandajoe added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Dec 4, 2024

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2024

fix: Exception handling for SQL Lab views (#30897)

db95934

(cherry picked from commit c2885a1)

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2024

fix: Exception handling for SQL Lab views (#30897)

988aa61

(cherry picked from commit c2885a1)

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2024

fix: Exception handling for SQL Lab views (#30897)

fa52c3a

(cherry picked from commit c2885a1)

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2024

fix: Exception handling for SQL Lab views (#30897)

5b99354

(cherry picked from commit c2885a1)

sadpandajoe pushed a commit that referenced this pull request Dec 5, 2024

fix: Exception handling for SQL Lab views (#30897)

1c08d5e

(cherry picked from commit c2885a1)

nyohasstium pushed a commit to Webgains/superset that referenced this pull request Jan 2, 2025

fix: Exception handling for SQL Lab views (apache#30897)

4bc9e31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Exception handling for SQL Lab views #30897

fix: Exception handling for SQL Lab views #30897

michael-s-molina commented Nov 11, 2024

michael-s-molina Nov 11, 2024

codecov bot commented Nov 11, 2024 •

edited

Loading

villebro Nov 11, 2024

michael-s-molina Nov 12, 2024 •

edited

Loading

michael-s-molina Nov 12, 2024 •

edited

Loading

villebro Nov 12, 2024

villebro left a comment

villebro Nov 12, 2024

fix: Exception handling for SQL Lab views #30897

fix: Exception handling for SQL Lab views #30897

Conversation

michael-s-molina commented Nov 11, 2024

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina Nov 11, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

villebro Nov 11, 2024

Choose a reason for hiding this comment

michael-s-molina Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

villebro Nov 12, 2024

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

villebro Nov 12, 2024

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2024 •

edited

Loading

michael-s-molina Nov 12, 2024 •

edited

Loading

michael-s-molina Nov 12, 2024 •

edited

Loading