From 854d3d59e4d4dd86856f2c82b9f278f50f375d55 Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Tue, 6 Oct 2026 14:15:13 -0600 Subject: [PATCH] fix(table): omit dormant grains from semantic aggregate requests (#44455) --- UPDATING.md | 9 + docs/docs/using-superset/exploring-data.mdx | 19 ++ .../plugin-chart-table/src/buildQuery.ts | 46 ++- .../plugin-chart-table/src/controlPanel.tsx | 21 +- .../test/buildQuery.test.ts | 166 ++++++++++ .../test/controlPanel.test.ts | 44 +++ .../gridComponents/Chart/Chart.test.tsx | 288 +++++++++++++++++- .../components/gridComponents/Chart/Chart.tsx | 94 ++++-- .../dashboard/containers/DashboardPage.tsx | 36 ++- .../contexts/DashboardDatasetsContext.ts | 28 ++ .../exploreUtils/exploreUtils.test.tsx | 32 ++ .../src/hooks/apiResources/dashboards.test.ts | 14 + .../src/hooks/apiResources/dashboards.ts | 28 +- 13 files changed, 761 insertions(+), 64 deletions(-) create mode 100644 superset-frontend/src/dashboard/contexts/DashboardDatasetsContext.ts diff --git a/UPDATING.md b/UPDATING.md index aabb7239fb0..8049e0b888a 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -44,6 +44,15 @@ assists people when migrating to a new version. loader rejects semantic bundles; use the chart, dashboard or assets importer instead. +### Semantic-view Table charts without a temporal axis + +Semantic-view Table charts omit recognized dormant time grains from +frontend-generated aggregate queries when no temporal axis is present. The saved +grain and Time Grain control visibility are unchanged. Direct API payloads and +saved chart-data GET requests that bypass frontend rebuilding retain strict +validation; some old stored query contexts can therefore still fail. Deploy +updated frontend assets with this change. + ### SQLite time filters on `DATE` columns On SQLite, Shillelagh and the Superset meta database, a time filter on a `DATE` diff --git a/docs/docs/using-superset/exploring-data.mdx b/docs/docs/using-superset/exploring-data.mdx index 158f43cbbad..01c4bdc1877 100644 --- a/docs/docs/using-superset/exploring-data.mdx +++ b/docs/docs/using-superset/exploring-data.mdx @@ -24,6 +24,25 @@ The following information about each flight is given: - Information about the origin and destination. - The distance between the origin and destination, in kilometers (km). +### Semantic Table time grains + +For a Table chart backed by a semantic view, Time Grain applies when grouping by +a temporal dimension. Removing the last temporal dimension leaves the saved +choice intact, but the frontend omits a recognized dormant grain from aggregate +requests whose selected columns are known to be non-temporal (including no +grouping columns). Adding the temporal dimension again reuses the saved choice. +Unknown columns or grain values are not silently normalized. Ordinary dataset +charts and raw-record queries are unchanged. + +This applies to requests rebuilt by the updated frontend in Explore and +dashboards; it does not rewrite saved charts. Direct API requests and saved +chart-data GET paths that bypass rebuilding remain subject to strict validation, +so old stored query contexts can still fail. Updated frontend assets are required. +Dashboards reuse each chart's saved `temporal_columns_lookup` rather than +recomputing it. A grouped chart saved before this change may lack the +non-temporal column metadata needed to omit a dormant grain, so its dashboard +requests can keep sending that grain until the chart is re-saved in Explore. + ### Enabling Data Upload Functionality You may need to enable the functionality to upload a CSV or Excel file to your database. The following section diff --git a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts index 7e9d80b653b..b771c6e5060 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts @@ -24,6 +24,7 @@ import { QueryFormOrderBy, QueryMode, QueryObject, + TimeGranularity, buildQueryContext, ensureIsArray, getMetricLabel, @@ -41,6 +42,36 @@ import { isEmpty } from 'lodash-es'; import { TableChartFormData } from './types'; import { updateTableOwnState } from './DataTable/utils/externalAPIs'; +// Only recognized duration choices are safe to treat as dormant. Legacy date +// formats and anchored week intervals are not semantic-layer durations. +const DURATION_GRAINS = new Set( + Object.values(TimeGranularity).filter( + grain => grain.startsWith('P') && !grain.includes('/'), + ), +); + +function omitDormantGrain( + query: QueryObject, + temporalColumns: Record | undefined, +): QueryObject { + const grain = query.extras?.time_grain_sqla; + if ( + typeof grain !== 'string' || + !DURATION_GRAINS.has(grain) || + query.granularity || + query.is_timeseries || + !Array.isArray(query.columns) || + !query.columns.every( + column => isPhysicalColumn(column) && temporalColumns?.[column] === false, + ) + ) { + return query; + } + const extras = { ...query.extras }; + delete extras.time_grain_sqla; + return { ...query, extras }; +} + /** * Infer query mode from form data. If `all_columns` is set, then raw records mode, * otherwise defaults to aggregation mode. @@ -85,7 +116,7 @@ export const buildQuery: BuildQuery = ( return acc.concat([metric, ...newMetrics]); }, []); - return buildQueryContext(formDataCopy, baseQueryObject => { + const context = buildQueryContext(formDataCopy, baseQueryObject => { let { metrics, orderby = [], columns = [] } = baseQueryObject; const { extras = {} } = baseQueryObject; const postProcessing: PostProcessingRule[] = []; @@ -401,6 +432,19 @@ export const buildQuery: BuildQuery = ( return [queryObject, ...extraQueries]; }); + + // Normalize only rebuilt frontend requests, never saved form data. GET chart + // data can bypass this builder, so old stored query contexts may still fail + // strict host validation. Classify each final query, including derived ones. + if ( + context.datasource.type === DatasourceType.SemanticView && + queryMode === QueryMode.Aggregate + ) { + context.queries = context.queries.map(query => + omitDormantGrain(query, formData.temporal_columns_lookup), + ); + } + return context; }; // Use this closure to cache changing of external filters, if we have server pagination we need reset page to 0, after diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx index d5e00503a99..2c3001b73d1 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx @@ -295,7 +295,26 @@ const config: ControlPanelConfig = { }, }, }, - 'temporal_columns_lookup', + { + name: 'temporal_columns_lookup', + config: { + ...sharedControls.temporal_columns_lookup, + // A missing entry is unknown, not evidence of a non-temporal + // column when classifying a dormant semantic grain. + initialValue: ( + control: ControlState, + state: ControlPanelState | null, + ) => + Object.fromEntries( + (state?.datasource?.columns ?? []) + .filter(column => typeof column.is_dttm === 'boolean') + .map(column => [ + column.column_name ?? column.name, + column.is_dttm, + ]), + ), + }, + }, ], [ { diff --git a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts index 33d449ae09d..9ca7728d5d4 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts +++ b/superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts @@ -32,6 +32,172 @@ const basicFormData: TableChartFormData = { datasource: '11__table', }; +test('omits dormant grain only for known non-temporal selected columns', () => { + const query = buildQueryUncached({ + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['orders'], + groupby: ['country'], + temporal_columns_lookup: { country: false }, + time_grain_sqla: TimeGranularity.DAY, + }).queries[0]; + expect(query.columns).toEqual(['country']); + expect(query.extras).not.toHaveProperty('time_grain_sqla'); +}); + +test.each>([ + { groupby: ['unknown'], temporal_columns_lookup: {} }, + { groupby: ['unknown'] }, + { + groupby: ['country', 'unknown'], + temporal_columns_lookup: { country: false }, + }, + { + groupby: [ + { expressionType: 'SQL', sqlExpression: 'year(ds)', label: 'year' }, + ], + }, + { granularity_sqla: 'metric_time' }, + { + groupby: ['metric_time', 'other_time'], + temporal_columns_lookup: { metric_time: true, other_time: true }, + }, + { + extra_form_data: { interactive_groupby: ['metric_time'] }, + temporal_columns_lookup: { metric_time: true }, + }, + { datasource: '11__table' }, + { query_mode: QueryMode.Raw }, +])( + 'preserves grain for active, ambiguous or out-of-scope input %j', + overrides => { + const query = buildQueryUncached({ + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['orders'], + groupby: [], + time_grain_sqla: TimeGranularity.DAY, + ...overrides, + }).queries[0]; + expect(query.extras?.time_grain_sqla).toBe(TimeGranularity.DAY); + }, +); + +test.each([ + undefined, + null, + '', + 'invalid', + 'P2D', + 'date', + 'P1W/1970-01-04T00:00:00Z', +])('does not normalize unrecognized or absent grain %s', grain => { + const form = { + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['orders'], + groupby: [], + time_grain_sqla: grain, + } as unknown as TableChartFormData; + expect(buildQueryUncached(form).queries[0].extras?.time_grain_sqla).toBe( + grain, + ); +}); + +test.each([TimeGranularity.DAY, TimeGranularity.MONTH, TimeGranularity.HOUR])( + 'omits dormant semantic grain %s without changing the saved form', + grain => { + const form: TableChartFormData = { + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['count_lifetime_orders'], + groupby: [], + time_grain_sqla: grain, + time_range: '2026-01-01 : 2026-02-01', + row_limit: 123, + }; + const original = structuredClone(form); + Object.freeze(form.groupby); + Object.freeze(form.metrics); + Object.freeze(form); + const result = buildQueryUncached(form); + + expect(result.queries[0].extras).not.toHaveProperty('time_grain_sqla'); + expect(result.queries[0]).toMatchObject({ + columns: [], + metrics: ['count_lifetime_orders'], + time_range: form.time_range, + row_limit: 123, + }); + expect(form).toEqual(original); + expect(result.form_data).toEqual(original); + }, +); + +test('normalizes final dormant override in all semantic derived queries', () => { + const form: TableChartFormData = { + ...basicFormData, + datasource: '2__semantic_view', + groupby: [], + metrics: ['orders'], + percent_metrics: ['orders'], + percent_metric_calculation: 'all_records', + show_totals: true, + server_pagination: true, + server_page_length: 10, + row_limit: 50, + time_grain_sqla: TimeGranularity.DAY, + extra_form_data: Object.freeze({ time_grain_sqla: TimeGranularity.MONTH }), + }; + const original = structuredClone(form); + Object.freeze(form); + const { queries } = buildQueryUncached(form); + + expect(queries).toHaveLength(4); + queries.forEach(query => { + expect(query.extras).not.toHaveProperty('time_grain_sqla'); + }); + expect(queries[1].is_rowcount).toBe(true); + expect(form).toEqual(original); +}); + +test('normalizes totals independently of the active semantic main axis', () => { + const { queries } = buildQueryUncached({ + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['orders'], + groupby: ['metric_time'], + temporal_columns_lookup: { metric_time: true }, + time_grain_sqla: TimeGranularity.MONTH, + show_totals: true, + }); + + expect(queries[0].extras?.time_grain_sqla).toBe(TimeGranularity.MONTH); + expect(queries[1].columns).toEqual([]); + expect(queries[1].extras).not.toHaveProperty('time_grain_sqla'); +}); + +test('keeps the selected semantic grain through axis removal and re-addition', () => { + const form: TableChartFormData = { + ...basicFormData, + datasource: '2__semantic_view', + metrics: ['orders'], + groupby: ['metric_time'], + temporal_columns_lookup: { metric_time: true }, + time_grain_sqla: TimeGranularity.MONTH, + }; + expect(buildQueryUncached(form).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.MONTH, + ); + expect( + buildQueryUncached({ ...form, groupby: [] }).queries[0].extras, + ).not.toHaveProperty('time_grain_sqla'); + expect(buildQueryUncached(form).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.MONTH, + ); + expect(form.time_grain_sqla).toBe(TimeGranularity.MONTH); +}); + const extraQueryFormData: TableChartFormData = { ...basicFormData, time_grain_sqla: TimeGranularity.MONTH, diff --git a/superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts b/superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts index 2cae108e67c..92687c28665 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts +++ b/superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts @@ -17,10 +17,46 @@ import { ControlPanelsContainerProps, ControlState, CustomControlItem, + sharedControls, } from '@superset-ui/chart-controls'; import { ComparisonType, QueryMode } from '@superset-ui/core'; import config from '../src/controlPanel'; +test('Table temporal metadata distinguishes non-temporal from unknown columns', () => { + const item = config.controlPanelSections + .flatMap(section => section?.controlSetRows ?? []) + .flat() + .find(control => + typeof control === 'string' + ? control === 'temporal_columns_lookup' + : control && + 'name' in control && + control.name === 'temporal_columns_lookup', + ); + const control = + typeof item === 'object' && item && 'config' in item + ? item.config + : sharedControls.temporal_columns_lookup; + const { initialValue } = control; + expect(typeof initialValue).toBe('function'); + if (typeof initialValue !== 'function') { + throw new Error('Expected temporal lookup initializer'); + } + const state = { + datasource: { + columns: [ + { column_name: 'metric_time', is_dttm: true }, + { column_name: 'country', is_dttm: false }, + { column_name: 'unknown' }, + ], + }, + } as unknown as Parameters[1]; + expect(initialValue({} as ControlState, state)).toEqual({ + metric_time: true, + country: false, + }); +}); + type VisibilityFn = ( props: ControlPanelsContainerProps, control?: ControlState, @@ -229,6 +265,14 @@ test('time_grain_sqla visibility should be case-insensitive', () => { expect(vis(mkProps(['some_other_col']), controlState)).toBe(false); }); +test('time grain visibility follows axis removal and re-addition', () => { + const visible = getVisibility(config, 'time_grain_sqla'); + expect(visible(mkProps(['ORDERDATE']))).toBe(true); + expect(visible(mkProps([]))).toBe(false); + expect(visible(mkProps(['some_other_col']))).toBe(false); + expect(visible(mkProps(['ORDERDATE']))).toBe(true); +}); + test('time_grain_sqla is hidden in raw records mode', () => { const vis = getVisibility(config, 'time_grain_sqla'); const controlState = {} as ControlState; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx index afc8d505404..42a56ef9645 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx @@ -18,7 +18,12 @@ */ import { useEffect } from 'react'; import { act, fireEvent, render } from 'spec/helpers/testing-library'; -import { FeatureFlag, VizType } from '@superset-ui/core'; +import { + DatasourceType, + FeatureFlag, + TimeGranularity, + VizType, +} from '@superset-ui/core'; import * as redux from 'redux'; import * as exploreUtils from 'src/explore/exploreUtils'; @@ -33,6 +38,12 @@ import { useAutoRefreshContext, } from 'src/dashboard/contexts/AutoRefreshContext'; import Chart from './Chart'; +import { + DashboardDatasetsContext, + type DashboardDatasetsContextValue, +} from 'src/dashboard/contexts/DashboardDatasetsContext'; +import { buildQuery as buildTableQuery } from '../../../../../plugins/plugin-chart-table/src/buildQuery'; +import type { TableChartFormData } from '../../../../../plugins/plugin-chart-table/src/types'; let capturedChartContainerProps: Record = {}; jest.mock('src/components/Chart/ChartContainer', () => { @@ -106,12 +117,23 @@ const defaultState = { function setup( overrideProps: Record = {}, overrideState: Record = {}, + currentDatasets: DashboardDatasetsContextValue | null = null, ) { - return render(, { - useRedux: true, - useRouter: true, - initialState: { ...defaultState, ...overrideState }, - }); + const chart = ; + return render( + currentDatasets ? ( + + {chart} + + ) : ( + chart + ), + { + useRedux: true, + useRouter: true, + initialState: { ...defaultState, ...overrideState }, + }, + ); } function StartAutoRefreshFor({ chartIds }: { chartIds: number[] }) { @@ -204,6 +226,260 @@ test('should render a ChartContainer', () => { expect(getByTestId('chart-container')).toBeInTheDocument(); }); +test('uses current semantic dimensions instead of a saved stale temporal lookup', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '2__semantic_view', + viz_type: VizType.Table, + metrics: ['orders'], + groupby: ['event_time'], + granularity: undefined, + time_grain_sqla: TimeGranularity.DAY, + temporal_columns_lookup: { event_time: false }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + datasources: { + '2__semantic_view': { + ...mockDatasource['7__table'], + type: DatasourceType.SemanticView, + columns: [{ column_name: 'event_time', is_dttm: false }], + }, + }, + dashboardState: { + ...defaultState.dashboardState, + datasetsStatus: 'complete', + }, + }, + { + dashboardId: props.dashboardId, + datasets: [ + { + uid: '2__semantic_view', + type: DatasourceType.SemanticView, + columns: [{ column_name: 'event_time', is_dttm: true }], + }, + ], + }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toEqual({ event_time: true }); + expect(buildTableQuery(formData).queries[0].columns).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + label: 'event_time', + timeGrain: TimeGranularity.DAY, + }), + ]), + ); + expect(buildTableQuery(formData).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.DAY, + ); + expect(savedFormData.temporal_columns_lookup).toEqual({ event_time: false }); +}); + +test('keeps a saved semantic grain when current datasource metadata is unavailable', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '2__semantic_view', + viz_type: VizType.Table, + metrics: ['orders'], + groupby: ['event_time'], + granularity: undefined, + time_grain_sqla: TimeGranularity.DAY, + temporal_columns_lookup: { event_time: false }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + datasources: {}, + }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toBeUndefined(); + expect(buildTableQuery(formData).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.DAY, + ); + expect(savedFormData.temporal_columns_lookup).toEqual({ event_time: false }); +}); + +test('ignores prior dashboard metadata while current datasets are loading', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '2__semantic_view', + viz_type: VizType.Table, + metrics: ['orders'], + groupby: ['event_time'], + granularity: undefined, + time_grain_sqla: TimeGranularity.DAY, + temporal_columns_lookup: { event_time: false }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + datasources: { + '2__semantic_view': { + ...mockDatasource['7__table'], + type: DatasourceType.SemanticView, + columns: [{ column_name: 'event_time', is_dttm: false }], + }, + }, + dashboardState: { + ...defaultState.dashboardState, + datasetsStatus: 'loading', + }, + }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toBeUndefined(); + expect(buildTableQuery(formData).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.DAY, + ); +}); + +test('ignores prior metadata when a completed response omits the semantic source', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '2__semantic_view', + viz_type: VizType.Table, + metrics: ['orders'], + groupby: ['event_time'], + granularity: undefined, + time_grain_sqla: TimeGranularity.DAY, + temporal_columns_lookup: { event_time: false }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + datasources: { + '2__semantic_view': { + ...mockDatasource['7__table'], + type: DatasourceType.SemanticView, + columns: [{ column_name: 'event_time', is_dttm: false }], + }, + }, + dashboardState: { + ...defaultState.dashboardState, + datasetsStatus: 'complete', + }, + }, + { dashboardId: props.dashboardId, datasets: [] }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toBeUndefined(); + expect(buildTableQuery(formData).queries[0].extras?.time_grain_sqla).toBe( + TimeGranularity.DAY, + ); +}); + +test('omits a dormant semantic grain when current metadata proves the axis non-temporal', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '2__semantic_view', + viz_type: VizType.Table, + metrics: ['orders'], + groupby: ['country'], + granularity: undefined, + time_grain_sqla: TimeGranularity.DAY, + temporal_columns_lookup: { country: true }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + datasources: { + '2__semantic_view': { + ...mockDatasource['7__table'], + type: DatasourceType.SemanticView, + columns: [{ column_name: 'country', is_dttm: false }], + }, + }, + dashboardState: { + ...defaultState.dashboardState, + datasetsStatus: 'complete', + }, + }, + { + dashboardId: props.dashboardId, + datasets: [ + { + uid: '2__semantic_view', + type: DatasourceType.SemanticView, + columns: [{ column_name: 'country', is_dttm: false }], + }, + ], + }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toEqual({ country: false }); + expect(buildTableQuery(formData).queries[0].extras).not.toHaveProperty( + 'time_grain_sqla', + ); +}); + +test('leaves saved SQL dataset lookup unchanged on dashboards', () => { + const savedFormData = { + ...chartQueries[queryId].form_data, + datasource: '11__table', + temporal_columns_lookup: { event_time: false }, + }; + setup( + {}, + { + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: savedFormData, + }, + }, + }, + ); + + const formData = capturedChartContainerProps.formData as TableChartFormData; + expect(formData.temporal_columns_lookup).toEqual({ event_time: false }); +}); + const noDescriptionRenderInputs = ([undefined, false, true] as const).flatMap( sliceExpanded => ([undefined, false, true] as const).map(allExpanded => ({ diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx index cf452b9d69f..1e7daf79748 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx @@ -24,11 +24,12 @@ import { useRef, useMemo, useState, + useContext, memo, RefObject, } from 'react'; import type { ChartCustomization, JsonObject } from '@superset-ui/core'; -import { VizType } from '@superset-ui/core'; +import { DatasourceType, VizType } from '@superset-ui/core'; import { styled } from '@apache-superset/core/theme'; import { t } from '@apache-superset/core/translation'; import { debounce } from 'lodash-es'; @@ -83,6 +84,7 @@ import { import getFormDataWithExtraFilters from '../../../util/charts/getFormDataWithExtraFilters'; import { useChartCustomizationFromRedux } from '../../nativeFilters/state'; import { PLACEHOLDER_DATASOURCE } from '../../../constants'; +import { DashboardDatasetsContext } from '../../../contexts/DashboardDatasetsContext'; interface ChartProps { id: number; @@ -170,6 +172,7 @@ const createOwnStateWithChartState = ( }; const Chart = (props: ChartProps) => { + const currentDashboardDatasets = useContext(DashboardDatasetsContext); const dispatch = useDispatch(); const descriptionRef = useRef(null); const headerRef = useRef(null); @@ -469,47 +472,74 @@ const Chart = (props: ChartProps) => { ), ); - const formData = useMemo( - () => - getFormDataWithExtraFilters({ - chart: { id: chart?.id ?? props.id, form_data: chart?.form_data }, // avoid passing the whole chart object - chartConfiguration, - chartCustomizationItems: - chartCustomizationItems as ChartCustomization[], - filters: getAppliedFilterValues(props.id), - colorScheme, - colorNamespace, - sliceId: props.id, - nativeFilters, - allSliceIds, - dataMask, - extraControls: (props.extraControls || {}) as Record< - string, - string | boolean | null - >, - labelsColor, - labelsColorMap, - sharedLabelsColors, - ownColorScheme, - }), - [ - chart?.id, - chart?.form_data, + const formData = useMemo(() => { + const filteredFormData = getFormDataWithExtraFilters({ + chart: { id: chart?.id ?? props.id, form_data: chart?.form_data }, // avoid passing the whole chart object chartConfiguration, - chartCustomizationItems, - props.id, - props.extraControls, + chartCustomizationItems: chartCustomizationItems as ChartCustomization[], + filters: getAppliedFilterValues(props.id), colorScheme, colorNamespace, + sliceId: props.id, nativeFilters, allSliceIds, dataMask, + extraControls: (props.extraControls || {}) as Record< + string, + string | boolean | null + >, labelsColor, labelsColorMap, sharedLabelsColors, ownColorScheme, - ], - ); + }); + if ( + chart?.form_data?.viz_type !== VizType.Table || + !chart.form_data.datasource?.endsWith(`__${DatasourceType.SemanticView}`) + ) { + return filteredFormData; + } + + // Saved dashboard form data may predate the current semantic view schema. + // Missing metadata is unknown, never proof that an axis is non-temporal. + const currentDatasource = + currentDashboardDatasets?.dashboardId === props.dashboardId + ? currentDashboardDatasets.datasets.find( + candidate => + candidate.uid === chart.form_data.datasource && + candidate.type === DatasourceType.SemanticView, + ) + : undefined; + const temporalColumnsLookup = currentDatasource + ? Object.fromEntries( + (currentDatasource.columns ?? []) + .filter(column => typeof column.is_dttm === 'boolean') + .map(column => [column.column_name, column.is_dttm]), + ) + : undefined; + return { + ...filteredFormData, + temporal_columns_lookup: temporalColumnsLookup, + }; + }, [ + chart?.id, + chart?.form_data, + chartConfiguration, + chartCustomizationItems, + props.id, + props.extraControls, + colorScheme, + colorNamespace, + nativeFilters, + allSliceIds, + dataMask, + labelsColor, + labelsColorMap, + sharedLabelsColors, + ownColorScheme, + currentDashboardDatasets, + props.dashboardId, + ]); (formData as JsonObject).dashboardId = dashboardInfo.id; diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index 9f612c40be1..397777166a1 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -31,6 +31,7 @@ import { useDashboardCharts, useDashboardDatasets, } from 'src/hooks/apiResources'; +import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import { hydrateDashboard } from 'src/dashboard/actions/hydrate'; import { clearDashboardHistory } from 'src/dashboard/actions/dashboardLayout'; import { setDatasources } from 'src/dashboard/actions/datasources'; @@ -53,6 +54,7 @@ import { import DashboardContainer from 'src/dashboard/containers/Dashboard'; import CrudThemeProvider from 'src/components/CrudThemeProvider'; import type { DashboardChartStates } from 'src/dashboard/types/chartState'; +import { DashboardDatasetsContext } from 'src/dashboard/contexts/DashboardDatasetsContext'; import { nanoid } from 'nanoid'; import type { ActiveFilters } from '../types'; @@ -152,6 +154,14 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { } = useDashboardDatasets(idOrSlug); const isDashboardHydrated = useRef(false); + const currentDashboardDatasets = useMemo( + () => + status === ResourceStatus.Complete && datasets && dashboard?.id + ? { dashboardId: dashboard.id, datasets } + : null, + [status, datasets, dashboard?.id], + ); + const error = dashboardApiError || chartsApiError; // Only 404 gets a graceful not-found state; a 403 (access denied) still // surfaces through the error boundary. @@ -424,18 +434,20 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { <> - - - - {DashboardBuilderComponent} - - - + + + + + {DashboardBuilderComponent} + + + + ) : ( diff --git a/superset-frontend/src/dashboard/contexts/DashboardDatasetsContext.ts b/superset-frontend/src/dashboard/contexts/DashboardDatasetsContext.ts new file mode 100644 index 00000000000..d60b53cbf24 --- /dev/null +++ b/superset-frontend/src/dashboard/contexts/DashboardDatasetsContext.ts @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { createContext } from 'react'; +import type { Datasource } from 'src/dashboard/types'; + +export interface DashboardDatasetsContextValue { + dashboardId: number; + datasets: Pick[]; +} + +export const DashboardDatasetsContext = + createContext(null); diff --git a/superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx b/superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx index ade6815594c..9dffbe29e0a 100644 --- a/superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx +++ b/superset-frontend/src/explore/exploreUtils/exploreUtils.test.tsx @@ -30,9 +30,41 @@ import * as hostNamesConfig from 'src/utils/hostNamesConfig'; import { ChartMetadata, getChartMetadataRegistry, + getChartBuildQueryRegistry, QueryFormData, SupersetClient, } from '@superset-ui/core'; +import { buildQuery as buildTableQuery } from '../../../plugins/plugin-chart-table/src/buildQuery'; + +test('shared chart request producer rebuilds saved semantic Table without saving', async () => { + const viz = 'semantic_table_dormant_grain_test'; + const registry = getChartBuildQueryRegistry(); + registry.registerValue(viz, buildTableQuery); + const post = jest.spyOn(SupersetClient, 'post'); + const put = jest.spyOn(SupersetClient, 'put'); + const formData = Object.freeze({ + viz_type: viz, + datasource: '2__semantic_view', + slice_id: 17, + metrics: ['orders'], + groupby: [], + time_grain_sqla: 'P1D', + extra_form_data: Object.freeze({ time_grain_sqla: 'P1M' }), + }); + try { + const payload = await buildV1ChartDataPayload({ formData }); + expect(payload.queries[0].extras).not.toHaveProperty('time_grain_sqla'); + expect(payload.form_data).toMatchObject(formData); + expect(formData.time_grain_sqla).toBe('P1D'); + expect(formData.extra_form_data.time_grain_sqla).toBe('P1M'); + expect(post).not.toHaveBeenCalled(); + expect(put).not.toHaveBeenCalled(); + } finally { + post.mockRestore(); + put.mockRestore(); + registry.remove(viz); + } +}); // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks describe('exploreUtils', () => { diff --git a/superset-frontend/src/hooks/apiResources/dashboards.test.ts b/superset-frontend/src/hooks/apiResources/dashboards.test.ts index 1a409ff1cdc..94b26038ff9 100644 --- a/superset-frontend/src/hooks/apiResources/dashboards.test.ts +++ b/superset-frontend/src/hooks/apiResources/dashboards.test.ts @@ -111,4 +111,18 @@ describe('useDashboardDatasets', () => { expect(result.current.result).toEqual(expectedContent); }); }); + + test('keeps transformed datasets stable across unrelated rerenders', async () => { + fetchMock.get('glob:*/api/v1/dashboard/*/datasets', { + result: mockDatasets, + }); + + const { result, rerender } = renderHook(() => useDashboardDatasets(1)); + await waitFor(() => expect(result.current.result).not.toBeNull()); + const datasets = result.current.result; + + rerender(); + + expect(result.current.result).toBe(datasets); + }); }); diff --git a/superset-frontend/src/hooks/apiResources/dashboards.ts b/superset-frontend/src/hooks/apiResources/dashboards.ts index d16a33e1cf6..9eda228d400 100644 --- a/superset-frontend/src/hooks/apiResources/dashboards.ts +++ b/superset-frontend/src/hooks/apiResources/dashboards.ts @@ -70,24 +70,28 @@ export const useDashboard = (idOrSlug: string | number) => { export const useDashboardCharts = (idOrSlug: string | number) => useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/charts`); +// A stable transform keeps dashboard dataset metadata stable across unrelated +// renders, so consumers do not rebuild chart form data while a query is active. +const addCurrencyFormats = (datasets: Datasource[]) => + datasets.map(dataset => ({ + ...dataset, + currencyFormats: Object.fromEntries( + (dataset.metrics ?? []) + .filter(metric => !!metric.currency) + .map((metric): [string, Currency] => [ + metric.metric_name, + metric.currency!, + ]), + ), + })); + // gets the datasets for a dashboard // important: this endpoint only returns the fields in the dataset // that are necessary for rendering the given dashboard export const useDashboardDatasets = (idOrSlug: string | number) => useTransformedResource( useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/datasets`), - datasets => - datasets.map(dataset => ({ - ...dataset, - currencyFormats: Object.fromEntries( - (dataset.metrics ?? []) - .filter(metric => !!metric.currency) - .map((metric): [string, Currency] => [ - metric.metric_name, - metric.currency!, - ]), - ), - })), + addCurrencyFormats, ); export const useEmbeddedDashboard = (idOrSlug: string | number) =>