fix(virtual dataset sync): Sync virtual dataset columns when changing…

… the SQL query (#30903)

Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
(cherry picked from commit f3e7c64)

superset-frontend/cypress-base/cypress/e2e/explore/control.test.ts

@@ -51,8 +51,8 @@ describe('Datasource control', () => {
     )
       .first()
       .focus();
-    cy.focused().clear();
-    cy.focused().type(`${newMetricName}{enter}`);
+    cy.focused().clear({ force: true });
+    cy.focused().type(`${newMetricName}{enter}`, { force: true });
 
     cy.get('[data-test="datasource-modal-save"]').click();
     cy.get('.antd5-modal-confirm-btns button').contains('OK').click();

superset-frontend/jest.config.js

@@ -75,4 +75,5 @@ module.exports = {
       },
     ],
   ],
+  testTimeout: 10000,
 };

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

@@ -84,6 +84,12 @@ export interface Dataset {
   filter_select?: boolean;
   filter_select_enabled?: boolean;
   column_names?: string[];
+  catalog?: string;
+  schema?: string;
+  table_name?: string;
+  database?: Record<string, unknown>;
+  normalize_columns?: boolean;
+  always_filter_main_dttm?: boolean;
 }
 
 export interface ControlPanelState {

superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts

@@ -17,7 +17,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { Maybe, QueryFormMetric } from '../../types';
+import { Currency, Maybe, QueryFormMetric } from '../../types';
 import { Column } from './Column';
 
 export type Aggregate =
@@ -65,7 +65,7 @@ export interface Metric {
   certification_details?: Maybe<string>;
   certified_by?: Maybe<string>;
   d3format?: Maybe<string>;
-  currency?: Maybe<string>;
+  currency?: Maybe<Currency>;
   description?: Maybe<string>;
   is_certified?: boolean;
   verbose_name?: string;

superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts

@@ -173,7 +173,7 @@ describe('BigNumberWithTrendline', () => {
               label: 'value',
               metric_name: 'value',
               d3format: '.2f',
-              currency: `{symbol: 'USD', symbolPosition: 'prefix' }`,
+              currency: { symbol: 'USD', symbolPosition: 'prefix' },
             },
           ],
         },

superset-frontend/src/components/Datasource/DatasourceEditor.jsx

@@ -23,7 +23,6 @@ import { Radio } from 'src/components/Radio';
 import Card from 'src/components/Card';
 import Alert from 'src/components/Alert';
 import Badge from 'src/components/Badge';
-import { nanoid } from 'nanoid';
 import {
   css,
   isFeatureEnabled,
@@ -57,6 +56,7 @@ import CurrencyControl from 'src/explore/components/controls/CurrencyControl';
 import CollectionTable from './CollectionTable';
 import Fieldset from './Fieldset';
 import Field from './Field';
+import { fetchSyncedColumns, updateColumns } from './utils';
 
 const DatasourceContainer = styled.div`
   .change-warning {
@@ -140,6 +140,14 @@ const StyledButtonWrapper = styled.span`
   `}
 `;
 
+const sqlTooltipOptions = {
+  placement: 'topRight',
+  title: t(
+    'If changes are made to your SQL query, ' +
+      'columns in your dataset will be synced when saving the dataset.',
+  ),
+};
+
 const checkboxGenerator = (d, onChange) => (
   <CheckboxControl value={d} onChange={onChange} />
 );
@@ -694,116 +702,27 @@ class DatasourceEditor extends PureComponent {
     });
   }
 
-  updateColumns(cols) {
-    // cols: Array<{column_name: string; is_dttm: boolean; type: string;}>
-    const { databaseColumns } = this.state;
-    const databaseColumnNames = cols.map(col => col.column_name);
-    const currentCols = databaseColumns.reduce(
-      (agg, col) => ({
-        ...agg,
-        [col.column_name]: col,
-      }),
-      {},
-    );
-    const finalColumns = [];
-    const results = {
-      added: [],
-      modified: [],
-      removed: databaseColumns
-        .map(col => col.column_name)
-        .filter(col => !databaseColumnNames.includes(col)),
-    };
-    cols.forEach(col => {
-      const currentCol = currentCols[col.column_name];
-      if (!currentCol) {
-        // new column
-        finalColumns.push({
-          id: nanoid(),
-          column_name: col.column_name,
-          type: col.type,
-          groupby: true,
-          filterable: true,
-          is_dttm: col.is_dttm,
-        });
-        results.added.push(col.column_name);
-      } else if (
-        currentCol.type !== col.type ||
-        (!currentCol.is_dttm && col.is_dttm)
-      ) {
-        // modified column
-        finalColumns.push({
-          ...currentCol,
-          type: col.type,
-          is_dttm: currentCol.is_dttm || col.is_dttm,
-        });
-        results.modified.push(col.column_name);
-      } else {
-        // unchanged
-        finalColumns.push(currentCol);
-      }
-    });
-    if (
-      results.added.length ||
-      results.modified.length ||
-      results.removed.length
-    ) {
-      this.setColumns({ databaseColumns: finalColumns });
-    }
-    return results;
-  }
-
-  syncMetadata() {
+  async syncMetadata() {
     const { datasource } = this.state;
-    const params = {
-      datasource_type: datasource.type || datasource.datasource_type,
-      database_name:
-        datasource.database.database_name || datasource.database.name,
-      catalog_name: datasource.catalog,
-      schema_name: datasource.schema,
-      table_name: datasource.table_name,
-      normalize_columns: datasource.normalize_columns,
-      always_filter_main_dttm: datasource.always_filter_main_dttm,
-    };
-    Object.entries(params).forEach(([key, value]) => {
-      // rison can't encode the undefined value
-      if (value === undefined) {
-        params[key] = null;
-      }
-    });
-    const endpoint = `/datasource/external_metadata_by_name/?q=${rison.encode_uri(
-      params,
-    )}`;
     this.setState({ metadataLoading: true });
-
-    SupersetClient.get({ endpoint })
-      .then(({ json }) => {
-        const results = this.updateColumns(json);
-        if (results.modified.length) {
-          this.props.addSuccessToast(
-            t('Modified columns: %s', results.modified.join(', ')),
-          );
-        }
-        if (results.removed.length) {
-          this.props.addSuccessToast(
-            t('Removed columns: %s', results.removed.join(', ')),
-          );
-        }
-        if (results.added.length) {
-          this.props.addSuccessToast(
-            t('New columns added: %s', results.added.join(', ')),
-          );
-        }
-        this.props.addSuccessToast(t('Metadata has been synced'));
-        this.setState({ metadataLoading: false });
-      })
-      .catch(response =>
-        getClientErrorObject(response).then(({ error, statusText }) => {
-          this.props.addDangerToast(
-            error || statusText || t('An error has occurred'),
-          );
-          this.setState({ metadataLoading: false });
-        }),
+    try {
+      const newCols = await fetchSyncedColumns(datasource);
+      const columnChanges = updateColumns(
+        datasource.columns,
+        newCols,
+        this.props.addSuccessToast,
       );
+      this.setColumns({ databaseColumns: columnChanges.finalColumns });
+      this.props.addSuccessToast(t('Metadata has been synced'));
+      this.setState({ metadataLoading: false });
+    } catch (error) {
+      const { error: clientError, statusText } =
+        await getClientErrorObject(error);
+      this.props.addDangerToast(
+        clientError || statusText || t('An error has occurred'),
+      );
+      this.setState({ metadataLoading: false });
+    }
   }
 
   findDuplicates(arr, accessor) {
@@ -1146,6 +1065,7 @@ class DatasourceEditor extends PureComponent {
                         maxLines={Infinity}
                         readOnly={!this.state.isEditMode}
                         resize="both"
+                        tooltipOptions={sqlTooltipOptions}
                       />
                     }
                   />