chore: migrate `sqllab_viz` endpoint to api v1 #23729

hughhhh · 2023-04-18T22:13:02Z

SUMMARY

Migrating legacy endpoint that creates datasets (superset/sqllab_viz) to api v1

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

…-apiv1-sqllab-viz

codecov · 2023-04-19T00:34:16Z

Codecov Report

Merging #23729 (a428ecc) into master (e8121b1) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

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

@@            Coverage Diff             @@
##           master   #23729      +/-   ##
==========================================
- Coverage   68.02%   68.02%   -0.01%     
==========================================
  Files        1936     1936              
  Lines       74929    74931       +2     
  Branches     8141     8141              
==========================================
+ Hits        50970    50971       +1     
- Misses      21871    21872       +1     
  Partials     2088     2088

Flag	Coverage Δ
hive	`53.01% <100.00%> (+<0.01%)`	⬆️
javascript	`54.28% <71.42%> (-0.01%)`	⬇️
mysql	`78.80% <100.00%> (+<0.01%)`	⬆️
postgres	`78.87% <100.00%> (+<0.01%)`	⬆️
presto	`52.92% <100.00%> (+<0.01%)`	⬆️
python	`82.67% <100.00%> (+<0.01%)`	⬆️
sqlite	`77.39% <100.00%> (+<0.01%)`	⬆️
unit	`52.81% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`50.00% <0.00%> (ø)`
superset-frontend/src/SqlLab/actions/sqlLab.js	`66.33% <50.00%> (-0.17%)`	⬇️
...frontend/src/explore/actions/datasourcesActions.ts	`100.00% <100.00%> (ø)`
superset/views/core.py	`75.33% <100.00%> (+0.01%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jfrag1 · 2023-04-20T16:21:25Z

superset/datasets/api.py

@@ -989,3 +1001,120 @@ def get_or_create_dataset(self) -> Response:
                exc_info=True,
            )
            return self.response_422(message=ex.message)
+
+    @expose("/sqllab_viz/", methods=["POST"])


Instead of creating a new endpoint, could we instead re-use the POST /api/v1/dataset/ endpoint? It seems to serve the same purpose and has the validation being done in this endpoint automatically built-in

jfrag1

Nice, glad this approach works! There's still one place the endpoint needs to be updated, and some cleanup

jfrag1 · 2023-04-21T16:39:36Z

superset-frontend/src/SqlLab/actions/sqlLab.js

+        schema: schema,
+        table_name: datasourceName,
+        sql: sql,
+        owners: [1],


the owners key can probably just be omitted from the payload here, then the backend should default to making the current user owner I believe

jfrag1 · 2023-04-21T16:41:29Z

superset-frontend/src/explore/actions/datasourcesActions.ts

@@ -49,7 +49,7 @@ export function saveDataset({
      const {
        json: { data },
      } = await SupersetClient.post({
-        endpoint: '/superset/sqllab_viz/',
+        endpoint: '/api/v1/dataset/sqllab_viz/',


This one will need be updated as well

Also since this is explore, the response payload from POST /api/v1/dataset/ maybe be missing keys that explore expects to be there in order to work properly. After this post we may need to make a request to GET /api/v1/dataset/{id}, which will return all needed keys. See here in a recent PR of mine where I had to do something similar: https://github.com/apache/superset/pull/23678/files#diff-ccfa293ffd1a07eab82f8646fcb2732ff5d001fec273656b18064db5115ac9fbR165

jfrag1 · 2023-04-21T16:46:48Z

superset/datasets/schemas.py

+    is_dttm = fields.Boolean(required=True, description="Name of table")
+
+
+class SqllabVizSchema(Schema):


these can be removed

jfrag1 · 2023-04-21T16:48:09Z

tests/integration_tests/sqllab_tests.py

@@ -485,102 +485,6 @@ def test_pa_conversion_dict(self):
        self.assertEqual(len(data), results.size)
        self.assertEqual(len(cols), len(results.columns))

-    def test_sqllab_viz(self):


nit: I think it's fine to leave in tests for deprecated endpoints, we can just delete them when they are eventually removed for good

jfrag1

LGTM, just one small thing

jfrag1 · 2023-04-24T18:30:02Z

superset/datasets/api.py

@@ -95,6 +95,7 @@ class DatasetRestApi(BaseSupersetModelRestApi):
        "related_objects",
        "duplicate",
        "get_or_create_dataset",
+        "sqllab_viz",


this can be removed

Antonio-RiveroMartnez

LGTM! Concerns were addressed

betodealmeida

Looks good, just a comment on the Length() validation that needs to be fixed.

superset/datasets/schemas.py

This reverts commit fa8f984.

villebro · 2023-06-20T07:08:59Z

@hughhhh @betodealmeida @jfrag1 @Antonio-RiveroMartnez this PR appears to have caused a nasty regression, namely it's now impossible to save a chart from a query that hasn't been saved as a dataset first: saving triggers an "Unknown field." error:

Interestingly the "Save as dataset" flow works:

This is happening, because in the main "Save" workflow we're sending a POST to /api/v1/dataset/ with the same type of payload that we were sending previously to /superset/sqllab_viz/:

As can be seen here, the schema doesn't support columns, nor template_params:

superset/superset/datasets/schemas.py

Lines 79 to 87 in 3e76736

    
           class DatasetPostSchema(Schema): 
        
               database = fields.Integer(required=True) 
        
               schema = fields.String(allow_none=True, validate=Length(0, 250)) 
        
               table_name = fields.String(required=True, allow_none=False, validate=Length(1, 250)) 
        
               sql = fields.String(allow_none=True) 
        
               owners = fields.List(fields.Integer()) 
        
               is_managed_externally = fields.Boolean(allow_none=True, dump_default=False) 
        
               external_url = fields.String(allow_none=True)

I think we need to DRY up these flows, as it feels unnecessary to have a different code path for saving the dataset in "Save as dataset" vs the main "Save" button..

hughhhh added 2 commits April 7, 2023 11:07

save

718db12

Merge branch 'master' of https://github.com/apache/superset into hxgh…

b6e7d39

…-apiv1-sqllab-viz

pull-request-size bot added the size/L label Apr 18, 2023

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from ae7b376 to 21a4833 Compare April 19, 2023 00:26

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from 21a4833 to 2912bb1 Compare April 20, 2023 00:18

lintitng

8038701

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from 2912bb1 to 8038701 Compare April 20, 2023 00:21

update test

f76c4d2

jfrag1 reviewed Apr 20, 2023

View reviewed changes

hughhhh added 2 commits April 20, 2023 18:49

update frontend to use new FAB POST dataset/add

aa6e6eb

remove sqllab_viz test

ff5edc1

jfrag1 reviewed Apr 21, 2023

View reviewed changes

remote old endpoint

cbac36a

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch 2 times, most recently from cd01015 to 0c8ae6a Compare April 21, 2023 18:46

refactor lint

0ce4316

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from 0c8ae6a to 0ce4316 Compare April 21, 2023 18:47

hughhhh added 3 commits April 21, 2023 14:50

ok

193343a

fix lint

febcf67

ok

c7c05a7

jfrag1 approved these changes Apr 24, 2023

View reviewed changes

address concerns

3ea64e6

Antonio-RiveroMartnez approved these changes Apr 24, 2023

View reviewed changes

betodealmeida approved these changes Apr 24, 2023

View reviewed changes

superset/datasets/schemas.py Outdated Show resolved Hide resolved

superset/datasets/schemas.py Outdated Show resolved Hide resolved

pull-request-size bot added size/M and removed size/L labels Apr 24, 2023

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from bf1f48f to 3ea64e6 Compare April 25, 2023 02:46

pull-request-size bot added size/L and removed size/M labels Apr 25, 2023

fix check

cca9d8a

pull-request-size bot added size/M and removed size/L labels Apr 25, 2023

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from 1ec7906 to cca9d8a Compare April 25, 2023 16:03

pull-request-size bot added size/L size/M and removed size/M size/L labels Apr 25, 2023

hughhhh force-pushed the hxgh-apiv1-sqllab-viz branch from 4c988c7 to cca9d8a Compare April 25, 2023 23:33

pull-request-size bot added size/L and removed size/M labels Apr 25, 2023

Merge branch 'master' into hxgh-apiv1-sqllab-viz

7411635

pull-request-size bot added size/M and removed size/L labels Apr 25, 2023

hughhhh merged commit fa8f984 into master Apr 26, 2023
54 checks passed

sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023

chore: migrate sqllab_viz endpoint to api v1 (apache#23729)

c2650a0

sadpandajoe added a commit to preset-io/superset that referenced this pull request Jun 1, 2023

Revert "chore: migrate sqllab_viz endpoint to api v1 (apache#23729)"

139614a

This reverts commit fa8f984.

sadpandajoe added a commit to preset-io/superset that referenced this pull request Jun 12, 2023

Revert "chore: migrate sqllab_viz endpoint to api v1 (apache#23729)"

46984ee

This reverts commit fa8f984.

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

mistercrunch deleted the hxgh-apiv1-sqllab-viz branch March 26, 2024 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate `sqllab_viz` endpoint to api v1 #23729

chore: migrate `sqllab_viz` endpoint to api v1 #23729

hughhhh commented Apr 18, 2023 •

edited

codecov bot commented Apr 19, 2023 •

edited

jfrag1 Apr 20, 2023

jfrag1 left a comment

jfrag1 Apr 21, 2023

jfrag1 Apr 21, 2023

jfrag1 Apr 21, 2023

jfrag1 Apr 21, 2023

jfrag1 Apr 21, 2023

jfrag1 left a comment

jfrag1 Apr 24, 2023

Antonio-RiveroMartnez left a comment

betodealmeida left a comment

villebro commented Jun 20, 2023

		is_dttm = fields.Boolean(required=True, description="Name of table")


		class SqllabVizSchema(Schema):

chore: migrate sqllab_viz endpoint to api v1 #23729

chore: migrate sqllab_viz endpoint to api v1 #23729

Conversation

hughhhh commented Apr 18, 2023 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Apr 19, 2023 • edited

Codecov Report

jfrag1 Apr 20, 2023

Choose a reason for hiding this comment

jfrag1 left a comment

Choose a reason for hiding this comment

jfrag1 Apr 21, 2023

Choose a reason for hiding this comment

jfrag1 Apr 21, 2023

Choose a reason for hiding this comment

jfrag1 Apr 21, 2023

Choose a reason for hiding this comment

jfrag1 Apr 21, 2023

Choose a reason for hiding this comment

jfrag1 Apr 21, 2023

Choose a reason for hiding this comment

jfrag1 left a comment

Choose a reason for hiding this comment

jfrag1 Apr 24, 2023

Choose a reason for hiding this comment

Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

villebro commented Jun 20, 2023

chore: migrate `sqllab_viz` endpoint to api v1 #23729

chore: migrate `sqllab_viz` endpoint to api v1 #23729

hughhhh commented Apr 18, 2023 •

edited

codecov bot commented Apr 19, 2023 •

edited