chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1 #22910

diegomedina248 · 2023-01-30T16:34:50Z

SUMMARY

Continuing the effort on deprecating all /superset/ REST API endpoints
Deprecates /superset/estimate_query_cost for /api/v1/sqllab/estimate/

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

…to API v1

…stimate_query_cost-to-v1

codecov · 2023-01-30T17:36:19Z

Codecov Report

Merging #22910 (3f0c20b) into master (efe6d22) will decrease coverage by 11.25%.
The diff coverage is 54.79%.

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

@@             Coverage Diff             @@
##           master   #22910       +/-   ##
===========================================
- Coverage   67.45%   56.21%   -11.25%     
===========================================
  Files        1878     1878               
  Lines       72140    72218       +78     
  Branches     7866     7872        +6     
===========================================
- Hits        48663    40595     -8068     
- Misses      21458    29603     +8145     
- Partials     2019     2020        +1

Flag	Coverage Δ
hive	`?`
mysql	`?`
postgres	`?`
presto	`52.62% <55.55%> (+<0.01%)`	⬆️
python	`58.85% <55.55%> (-23.32%)`	⬇️
sqlite	`?`
unit	`52.51% <55.55%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/actions/sqlLab.js	`63.49% <0.00%> (-0.56%)`	⬇️
superset-frontend/src/SqlLab/reducers/sqlLab.js	`36.87% <ø> (ø)`
superset/sqllab/commands/estimate.py	`50.00% <50.00%> (ø)`
superset/sqllab/api.py	`84.84% <56.25%> (-15.16%)`	⬇️
superset/sqllab/schemas.py	`100.00% <100.00%> (ø)`
superset/views/core.py	`36.08% <100.00%> (-38.56%)`	⬇️
superset/utils/dashboard_import_export.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/tags/core.py	`4.54% <0.00%> (-95.46%)`	⬇️
superset/key_value/commands/update.py	`0.00% <0.00%> (-90.91%)`	⬇️
superset/key_value/commands/delete.py	`0.00% <0.00%> (-87.88%)`	⬇️
... and 343 more

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

dpgaspar · 2023-01-31T09:17:24Z

superset/sqllab/commands/estimate.py

+ self._schema = params.get("schema", "")
+
+ def validate(self) -> None:
+ self._database = db.session.query(Database).get(self._database_id)


this should go to the DAO to get all possible security constraints applied to it

dpgaspar · 2023-01-31T09:18:20Z

superset/sqllab/api.py

 ExecutePayloadSchema,
 QueryExecutionResponseSchema,
 )

+ @expose("/estimate/", methods=["POST"])


What do you think about placing this one on /api/v1/database/ instead?

It's a balancing act here, I agree.
I think the database resource is bloated already, and while I could see usages outside SQL lab, we don't at the moment nor in the medium term (afaik).
I could see arguments on both sides and I don't feel particularly strong about either of them

dpgaspar · 2023-01-31T09:19:08Z

superset/sqllab/commands/estimate.py

+ _schema: str
+ _database: Database
+
+ def __init__(self, params: EstimateQueryCostSchema) -> None:


user should be passed in, because there are some security constraints that should be applied

dpgaspar · 2023-01-31T09:23:51Z

superset/sqllab/schemas.py

@@ -25,6 +25,13 @@
 }


+class EstimateQueryCostSchema(Schema):
+ database_id = fields.Integer(required=True)
+ sql = fields.String(required=True)


let's add some nice descriptions here

dpgaspar · 2023-01-31T09:33:37Z

tests/integration_tests/sql_lab/commands_tests.py

+ sqllab_timeout=app.config["SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT"],
+ )
+
+ def test_run_success(self) -> None:


add some tests for users that don't have access to the database (missing database access on [some-db.id])

…stimate_query_cost-to-v1

diegomedina248 · 2023-02-07T02:01:27Z

@dpgaspar the remaining comments here I believe will be addressed by you as a follow up right?

chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ …

f929d56

…to API v1

pull-request-size bot added the size/L label Jan 30, 2023

diegomedina248 force-pushed the dm/migrate-estimate_query_cost-to-v1 branch from 7c4cceb to 99ce414 Compare January 30, 2023 16:51

pull-request-size bot added size/XL and removed size/L labels Jan 30, 2023

diegomedina248 force-pushed the dm/migrate-estimate_query_cost-to-v1 branch 2 times, most recently from 492488c to ee7d1d3 Compare January 30, 2023 16:59

Merge branch 'master' of github.com:apache/superset into dm/migrate-e…

a4ec247

…stimate_query_cost-to-v1

diegomedina248 force-pushed the dm/migrate-estimate_query_cost-to-v1 branch from ee7d1d3 to a4ec247 Compare January 30, 2023 17:07

cleanup

a7234a8

dpgaspar requested review from john-bodley and villebro January 31, 2023 09:03

dpgaspar reviewed Jan 31, 2023

View reviewed changes

Merge branch 'master' of github.com:apache/superset into dm/migrate-e…

efc3e0e

…stimate_query_cost-to-v1

diegomedina248 force-pushed the dm/migrate-estimate_query_cost-to-v1 branch from 8e262dd to 7ab30e8 Compare February 7, 2023 01:48

PR comments

c660773

diegomedina248 force-pushed the dm/migrate-estimate_query_cost-to-v1 branch from 7ab30e8 to c660773 Compare February 7, 2023 01:49

hughhhh mentioned this pull request Feb 28, 2023

chore: Hugh/migrate estimate query cost to v1 #23226

Merged

9 tasks

hughhhh closed this Feb 28, 2023

sfirke mentioned this pull request Feb 15, 2024

fix: Plain error message when visiting a dashboard via permalink without permissions #27132

Merged

9 tasks

mistercrunch deleted the dm/migrate-estimate_query_cost-to-v1 branch March 26, 2024 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1 #22910

chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1 #22910

diegomedina248 commented Jan 30, 2023

codecov bot commented Jan 30, 2023 •

edited

Loading

dpgaspar Jan 31, 2023

dpgaspar Jan 31, 2023

diegomedina248 Feb 7, 2023

dpgaspar Jan 31, 2023

dpgaspar Jan 31, 2023

dpgaspar Jan 31, 2023

diegomedina248 commented Feb 7, 2023

chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1 #22910

chore: Migrate /superset/estimate_query_cost/<database_id>/<schema>/ to API v1 #22910

Conversation

diegomedina248 commented Jan 30, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

dpgaspar Jan 31, 2023

Choose a reason for hiding this comment

dpgaspar Jan 31, 2023

Choose a reason for hiding this comment

diegomedina248 Feb 7, 2023

Choose a reason for hiding this comment

dpgaspar Jan 31, 2023

Choose a reason for hiding this comment

dpgaspar Jan 31, 2023

Choose a reason for hiding this comment

dpgaspar Jan 31, 2023

Choose a reason for hiding this comment

diegomedina248 commented Feb 7, 2023

codecov bot commented Jan 30, 2023 •

edited

Loading