fix: validate_parameters and query #16241

betodealmeida · 2021-08-12T20:02:53Z

SUMMARY

When calling validate_parameters we're passing query as a string, not an object. This breaks the validation of parameters.

We still have 2 attributes with mixed types, we should fix them to prevent bugs like this:

superset/superset-frontend/src/views/CRUD/data/database/types.ts

Lines 77 to 80 in 1715143

    
           engine_params?: { 
        
             catalog: Record<any, any> | string; 
        
           }; 
        
           metadata_params?: {} | string;

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

betodealmeida · 2021-08-12T20:03:27Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx

+                  id="cost_estimate_enabled"
                  indeterminate={false}
-                  checked={!!db?.extra_json?.cost_query_enabled}
+                  checked={!!db?.extra_json?.cost_estimate_enabled}


This was incorrect.

betodealmeida · 2021-08-12T20:04:00Z

superset-frontend/src/views/CRUD/data/database/types.ts

-    // todo: ask beto where this should live
-    cost_query_enabled?: boolean; // in SQL Lab
+    version?: string;
+    cost_estimate_enabled?: boolean; // in SQL Lab


Fixed the name here too.

codecov · 2021-08-12T20:22:04Z

Codecov Report

Merging #16241 (c2db60e) into master (a1e18ed) will increase coverage by 0.01%.
The diff coverage is 58.33%.

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

@@            Coverage Diff             @@
##           master   #16241      +/-   ##
==========================================
+ Coverage   76.65%   76.67%   +0.01%     
==========================================
  Files         996      996              
  Lines       53149    53144       -5     
  Branches     6770     6763       -7     
==========================================
+ Hits        40743    40746       +3     
+ Misses      12176    12168       -8     
  Partials      230      230

Flag	Coverage Δ
javascript	`71.03% <44.44%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...set-frontend/src/views/CRUD/data/database/types.ts	`100.00% <ø> (ø)`
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`44.55% <28.57%> (+1.19%)`	⬆️
.../database/DatabaseModal/DatabaseConnectionForm.tsx	`53.92% <100.00%> (+0.45%)`	⬆️
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`93.18% <100.00%> (ø)`
superset/db_engine_specs/base.py	`88.37% <100.00%> (ø)`

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 67c4c01...285c098. Read the comment docs.

eschutho · 2021-08-12T21:10:08Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

@@ -319,6 +320,18 @@ function dbReducer(
        [action.payload.name]: action.payload.json,
      };
    case ActionType.textChange:
+      if (action.payload.name === 'query_input') {


in this case, because it has a different return value, should we just make a different action name?

Ah, good point. Yeah, makes sense.

eschutho · 2021-08-12T21:50:23Z

superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx

+      query = action.payload?.parameters?.query || {};
+      query_input = Object.entries(query)
+        .map(([key, value]) => `${key}=${value}`)
+        .join('&');


I think elsewhere we do this conversion at the input level, don't we?

What do you mean with "at the input level"?

I don't think we're doing this anywhere else.

With this PR we convert query to a string and store it in query_input when we fetch the database. Changes to query_input are passed to query, but this is might not survive a roundtrip (a=b& => {a: 'b'} => a=b), so we keep query_input always up to date and use it to populate the form input.

I was thinking of what you did here, where you stored the schemas in state as an array, and then converted to a string on the input: https://github.com/betodealmeida/incubator-superset/blob/e768b3d20a554eb3da2b1f1f6910477fecaab9ee/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx#L377
But maybe that won't work here.. I think it's ok to leave this as is.. I was thinking consistency and keeping the state as simple as possible.

It works for the array because the roundtrip preserves the text in the form:

a => ['a'] => a a, => ['a', ''] => a,

But for query we need to store the intermediary value for cases like this:

a=1&b => {a: 1, b: ''} => a=1&b=

eschutho

Looks great

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py

* fix: validate_parameters and query * add onQueryChange

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

* fix: validate_parameters and query * add onQueryChange

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 1753631)

pull-request-size bot added the size/M label Aug 12, 2021

betodealmeida commented Aug 12, 2021

View reviewed changes

fix: validate_parameters and query

c2db60e

betodealmeida force-pushed the ch22108 branch from 284c668 to c2db60e Compare August 12, 2021 20:26

betodealmeida marked this pull request as ready for review August 12, 2021 20:27

eschutho reviewed Aug 12, 2021

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Aug 12, 2021

betodealmeida force-pushed the ch22108 branch from a73302a to 00e5d16 Compare August 12, 2021 22:26

eschutho approved these changes Aug 12, 2021

View reviewed changes

add onQueryChange

285c098

betodealmeida force-pushed the ch22108 branch from 00e5d16 to 285c098 Compare August 13, 2021 00:44

betodealmeida added the preset:2021.31 label Aug 13, 2021

betodealmeida merged commit 5d3d6b6 into apache:master Aug 13, 2021

stevenuray pushed a commit to preset-io/superset that referenced this pull request Aug 13, 2021

fix: validate_parameters and query (apache#16241)

f2ab391

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

villebro added the v1.3 label Aug 16, 2021

villebro pushed a commit that referenced this pull request Aug 16, 2021

fix: validate_parameters and query (#16241)

52f747b

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

fix: validate_parameters and query (apache#16241)

6ea67e4

* fix: validate_parameters and query * add onQueryChange

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021

fix: validate_parameters and query (apache#16241)

9ab8b8a

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

fix: validate_parameters and query (apache#16241)

03c6199

* fix: validate_parameters and query * add onQueryChange

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021

fix: validate_parameters and query (apache#16241)

b7beee8

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 5d3d6b6)

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix: validate_parameters and query (apache#16241)

3b78fac

* fix: validate_parameters and query * add onQueryChange (cherry picked from commit 1753631)

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

mistercrunch added the 🚢 1.4.0 label Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate_parameters and query #16241

fix: validate_parameters and query #16241

betodealmeida commented Aug 12, 2021 •

edited

Loading

betodealmeida Aug 12, 2021

betodealmeida Aug 12, 2021

codecov bot commented Aug 12, 2021 •

edited

Loading

eschutho Aug 12, 2021

betodealmeida Aug 12, 2021

eschutho Aug 12, 2021

betodealmeida Aug 12, 2021

betodealmeida Aug 12, 2021

eschutho Aug 12, 2021

betodealmeida Aug 13, 2021

eschutho left a comment

	engine_params?: {
	catalog: Record<any, any> \| string;
	};
	metadata_params?: {} \| string;

fix: validate_parameters and query #16241

fix: validate_parameters and query #16241

Conversation

betodealmeida commented Aug 12, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

betodealmeida commented Aug 12, 2021 •

edited

Loading

codecov bot commented Aug 12, 2021 •

edited

Loading