fix: validate_parameters and query (apache#16241)

* fix: validate_parameters and query * add onQueryChange
opus-42 · Nov 14, 2021 · 6ea67e4 · 6ea67e4
1 parent 7ee07a3
commit 6ea67e4
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 53 deletions.
diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
@@ -61,6 +61,8 @@ interface FieldPropTypes {
   onParametersUploadFileChange: (value: any) => string;
   changeMethods: { onParametersChange: (value: any) => string } & {
     onChange: (value: any) => string;
+  } & {
+    onQueryChange: (value: any) => string;
   } & { onParametersUploadFileChange: (value: any) => string } & {
     onAddTableCatalog: () => void;
     onRemoveTableCatalog: (idx: number) => void;
@@ -417,15 +419,15 @@ const queryField = ({
   db,
 }: FieldPropTypes) => (
   <ValidatedInput
-    id="query"
-    name="query"
+    id="query_input"
+    name="query_input"
     required={required}
-    value={db?.parameters?.query}
+    value={db?.query_input || ''}
     validationMethods={{ onBlur: getValidation }}
     errorMessage={validationErrors?.query}
     placeholder="e.g. param1=value1&param2=value2"
     label="Additional Parameters"
-    onChange={changeMethods.onParametersChange}
+    onChange={changeMethods.onQueryChange}
     helpText={t('Add additional custom parameters')}
   />
 );
@@ -477,6 +479,7 @@ const DatabaseConnectionForm = ({
   dbModel: { parameters },
   onParametersChange,
   onChange,
+  onQueryChange,
   onParametersUploadFileChange,
   onAddTableCatalog,
   onRemoveTableCatalog,
@@ -498,6 +501,9 @@ const DatabaseConnectionForm = ({
   onChange: (
     event: FormEvent<InputProps> | { target: HTMLInputElement },
   ) => void;
+  onQueryChange: (
+    event: FormEvent<InputProps> | { target: HTMLInputElement },
+  ) => void;
   onParametersUploadFileChange?: (
     event: FormEvent<InputProps> | { target: HTMLInputElement },
   ) => void;
@@ -525,6 +531,7 @@ const DatabaseConnectionForm = ({
             changeMethods: {
               onParametersChange,
               onChange,
+              onQueryChange,
               onParametersUploadFileChange,
               onAddTableCatalog,
               onRemoveTableCatalog,

diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/ExtraOptions.tsx
@@ -169,9 +169,9 @@ const ExtraOptions = ({
             <StyledInputContainer css={no_margin_bottom}>
               <div className="input-container">
                 <IndeterminateCheckbox
-                  id="cost_query_enabled"
+                  id="cost_estimate_enabled"
                   indeterminate={false}
-                  checked={!!db?.extra_json?.cost_query_enabled}
+                  checked={!!db?.extra_json?.cost_estimate_enabled}
                   onChange={onExtraInputChange}
                   labelText={t('Enable query cost estimation')}
                 />

diff --git a/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx b/superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
@@ -146,6 +146,7 @@ enum ActionType {
   extraEditorChange,
   addTableCatalogSheet,
   removeTableCatalogSheet,
+  queryChange,
 }
 
 interface DBReducerPayloadType {
@@ -163,6 +164,7 @@ type DBReducerActionType =
         | ActionType.extraEditorChange
         | ActionType.extraInputChange
         | ActionType.textChange
+        | ActionType.queryChange
         | ActionType.inputChange
         | ActionType.editorChange
         | ActionType.parametersChange;
@@ -205,7 +207,8 @@ function dbReducer(
   const trimmedState = {
     ...(state || {}),
   };
-  let query = '';
+  let query = {};
+  let query_input = '';
   let deserializeExtraJSON = {};
   let extra_json: DatabaseObject['extra_json'];
 
@@ -318,6 +321,15 @@ function dbReducer(
         ...trimmedState,
         [action.payload.name]: action.payload.json,
       };
+    case ActionType.queryChange:
+      return {
+        ...trimmedState,
+        parameters: {
+          ...trimmedState.parameters,
+          query: Object.fromEntries(new URLSearchParams(action.payload.value)),
+        },
+        query_input: action.payload.value,
+      };
     case ActionType.textChange:
       return {
         ...trimmedState,
@@ -339,16 +351,17 @@ function dbReducer(
         };
       }
 
+      // convert query to a string and store in query_input
+      query = action.payload?.parameters?.query || {};
+      query_input = Object.entries(query)
+        .map(([key, value]) => `${key}=${value}`)
+        .join('&');
+
       if (
         action.payload.backend === 'bigquery' &&
         action.payload.configuration_method ===
           CONFIGURATION_METHOD.DYNAMIC_FORM
       ) {
-        // convert query into URI params string
-        query = new URLSearchParams(
-          action?.payload?.parameters?.query as string,
-        ).toString();
-
         return {
           ...action.payload,
           engine: action.payload.backend,
@@ -360,6 +373,7 @@ function dbReducer(
             ),
             query,
           },
+          query_input,
         };
       }
 
@@ -381,37 +395,18 @@ function dbReducer(
             name: e,
             value: engineParamsCatalog[e],
           })),
+          query_input,
         } as DatabaseObject;
       }
 
-      if (action.payload?.parameters?.query) {
-        // convert query into URI params string
-        query = new URLSearchParams(
-          action.payload.parameters.query as string,
-        ).toString();
-
-        return {
-          ...action.payload,
-          encrypted_extra: action.payload.encrypted_extra || '',
-          engine: action.payload.backend || trimmedState.engine,
-          configuration_method: action.payload.configuration_method,
-          extra_json: deserializeExtraJSON,
-          parameters: {
-            ...action.payload.parameters,
-            query,
-          },
-        };
-      }
-
       return {
         ...action.payload,
         encrypted_extra: action.payload.encrypted_extra || '',
         engine: action.payload.backend || trimmedState.engine,
         configuration_method: action.payload.configuration_method,
         extra_json: deserializeExtraJSON,
-        parameters: {
-          ...action.payload.parameters,
-        },
+        parameters: action.payload.parameters,
+        query_input,
       };
 
     case ActionType.dbSelected:
@@ -539,17 +534,6 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     const dbToUpdate = JSON.parse(JSON.stringify(update));
 
     if (dbToUpdate.configuration_method === CONFIGURATION_METHOD.DYNAMIC_FORM) {
-      if (dbToUpdate?.parameters?.query) {
-        // convert query params into dictionary
-        const urlParams = new URLSearchParams(dbToUpdate?.parameters?.query);
-        dbToUpdate.parameters.query = Object.fromEntries(urlParams);
-      } else if (
-        dbToUpdate?.parameters?.query === '' &&
-        'query' in dbModel.parameters.properties
-      ) {
-        dbToUpdate.parameters.query = {};
-      }
-
       // Validate DB before saving
       await getValidation(dbToUpdate, true);
       if (validationErrors && !isEmpty(validationErrors)) {
@@ -991,6 +975,12 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             value: target.value,
           })
         }
+        onQueryChange={({ target }: { target: HTMLInputElement }) =>
+          onChange(ActionType.queryChange, {
+            name: target.name,
+            value: target.value,
+          })
+        }
         onAddTableCatalog={() =>
           setDB({ type: ActionType.addTableCatalogSheet })
         }
@@ -1112,6 +1102,12 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   value: target.value,
                 })
               }
+              onQueryChange={({ target }: { target: HTMLInputElement }) =>
+                onChange(ActionType.queryChange, {
+                  name: target.name,
+                  value: target.value,
+                })
+              }
               onAddTableCatalog={() =>
                 setDB({ type: ActionType.addTableCatalogSheet })
               }
@@ -1258,6 +1254,12 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                   onAddTableCatalog={() => {
                     setDB({ type: ActionType.addTableCatalogSheet });
                   }}
+                  onQueryChange={({ target }: { target: HTMLInputElement }) =>
+                    onChange(ActionType.queryChange, {
+                      name: target.name,
+                      value: target.value,
+                    })
+                  }
                   onRemoveTableCatalog={(idx: number) => {
                     setDB({
                       type: ActionType.removeTableCatalogSheet,

diff --git a/superset-frontend/src/views/CRUD/data/database/types.ts b/superset-frontend/src/views/CRUD/data/database/types.ts
@@ -45,15 +45,12 @@ export type DatabaseObject = {
     password?: string;
     encryption?: boolean;
     credentials_info?: string;
-    query?: string | object;
-    catalog?: {};
+    query?: Record<string, string>;
+    catalog?: Record<string, string>;
   };
   configuration_method: CONFIGURATION_METHOD;
   engine?: string;
 
-  // Gsheets temporary storage
-  catalog?: Array<CatalogObject>;
-
   // Performance
   cache_timeout?: string;
   allow_run_async?: boolean;
@@ -85,11 +82,14 @@ export type DatabaseObject = {
     allows_virtual_table_explore?: boolean; // in SQL Lab
     schemas_allowed_for_csv_upload?: string[]; // in Security
     cancel_query_on_windows_unload?: boolean; // in Performance
-    version?: string;
 
-    // todo: ask beto where this should live
-    cost_query_enabled?: boolean; // in SQL Lab
+    version?: string;
+    cost_estimate_enabled?: boolean; // in SQL Lab
   };
+
+  // Temporary storage
+  catalog?: Array<CatalogObject>;
+  query_input?: string;
   extra?: string;
 };