You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/03/20 15:45:57 UTC
(superset) 01/03: fix(explore): add matrixify_enable guard to prevent stale validators on pre-revamp charts
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch fix-echart-save-issue
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d307100dbb3617d8032791a5ede840a7e06a7a39
Author: Joe Li <jo...@preset.io>
AuthorDate: Thu Mar 19 15:28:00 2026 -0700
fix(explore): add matrixify_enable guard to prevent stale validators on pre-revamp charts
PR #38519 dropped the matrixify_enable check from isMatrixifyVisible(),
causing hidden validators to fire on charts saved before the revamp.
Old charts have stale matrixify_mode defaults (e.g. 'dimensions') that
make isMatrixifyVisible() return true, injecting validators that silently
block save.
- Add matrixify_enable !== true guard as first check in isMatrixifyVisible()
- Add form_data migration in handleDeprecatedControls() to reset stale
matrixify modes when matrixify_enable key is absent (legacy charts)
- Export isMatrixifyVisible for direct unit testing
- Add 25 tests covering guard, validator injection, migration, and
dashboard hydration paths
Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
---
.../src/shared-controls/matrixifyControls.tsx | 9 +-
.../shared-controls/matrixifyControls.test.tsx | 236 ++++++++++++++++++++
superset-frontend/src/explore/store.test.tsx | 242 +++++++++++++++++----
superset-frontend/src/explore/store.ts | 12 +
4 files changed, 449 insertions(+), 50 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx
index 48dcaa3dafc..94291f4bd0b 100644
--- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx
@@ -29,13 +29,18 @@ import { defineSavedMetrics } from '../utils';
* Controls for transforming charts into matrix/grid layouts
*/
-// Utility function to check if matrixify controls should be visible
+// Utility function to check if matrixify controls should be visible.
+// Controls both visibility callbacks and validator injection via mapStateToProps.
+// The matrixify_enable guard prevents hidden validators from firing on
+// pre-revamp charts with stale matrixify_mode defaults (fix for #38519).
const isMatrixifyVisible = (
controls: any,
axis: 'rows' | 'columns',
mode?: 'metrics' | 'dimensions',
selectionMode?: 'members' | 'topn' | 'all',
) => {
+ if (controls?.matrixify_enable?.value !== true) return false;
+
const modeControl = `matrixify_mode_${axis}`;
const selectionModeControl = `matrixify_dimension_selection_mode_${axis}`;
@@ -372,4 +377,4 @@ matrixifyControls.matrixify_show_column_headers = {
visibility: ({ controls }) => isMatrixifyVisible(controls, 'columns'),
};
-export { matrixifyControls };
+export { matrixifyControls, isMatrixifyVisible };
diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx
new file mode 100644
index 00000000000..048afb69009
--- /dev/null
+++ b/superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx
@@ -0,0 +1,236 @@
+/**
+ * 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.
+ */
+
+/**
+ * Tests for the matrixify_enable guard in isMatrixifyVisible() and
+ * validator injection via mapStateToProps on real matrixify control definitions.
+ *
+ * These are TDD tests for the fix to apache/superset#38519 regression:
+ * isMatrixifyVisible() must check matrixify_enable before evaluating mode,
+ * otherwise pre-revamp charts with stale matrixify_mode defaults trigger
+ * hidden validators that block save.
+ */
+
+import {
+ matrixifyControls,
+ isMatrixifyVisible,
+} from '../../src/shared-controls/matrixifyControls';
+import type { ControlPanelState } from '../../src/types';
+
+// Helper: build a minimal controls object for ControlPanelState
+const buildControls = (overrides: Record<string, any> = {}) => {
+ const controls: Record<string, { value: any }> = {};
+ Object.entries(overrides).forEach(([key, value]) => {
+ controls[key] = { value };
+ });
+ return controls;
+};
+
+// Helper: build a minimal ControlPanelState for mapStateToProps.
+// Only provides fields that isMatrixifyVisible and mapStateToProps actually read.
+const buildState = (
+ controlValues: Record<string, any> = {},
+ formData: Record<string, any> = {},
+) =>
+ ({
+ controls: buildControls(controlValues),
+ datasource: { columns: [], type: 'table' },
+ form_data: formData,
+ common: {},
+ metadata: {},
+ slice: { slice_id: 0 },
+ }) as unknown as ControlPanelState;
+
+// ============================================================
+// Validator injection tests via real mapStateToProps (rows)
+// ============================================================
+
+// --- matrixify_dimension_rows ---
+
+test('matrixify_dimension_rows: validators empty when matrixify_enable is falsy', () => {
+ const control = matrixifyControls.matrixify_dimension_rows;
+ const state = buildState(
+ {
+ matrixify_enable: undefined,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'members',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators).toEqual([]);
+});
+
+test('matrixify_dimension_rows: validators present when matrixify_enable is true', () => {
+ const control = matrixifyControls.matrixify_dimension_rows;
+ const state = buildState(
+ {
+ matrixify_enable: true,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'members',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators.length).toBeGreaterThan(0);
+});
+
+// --- matrixify_topn_value_rows ---
+
+test('matrixify_topn_value_rows: validators empty when matrixify_enable is falsy', () => {
+ const control = matrixifyControls.matrixify_topn_value_rows;
+ const state = buildState(
+ {
+ matrixify_enable: undefined,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'topn',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators).toEqual([]);
+});
+
+test('matrixify_topn_value_rows: validators present when matrixify_enable is true', () => {
+ const control = matrixifyControls.matrixify_topn_value_rows;
+ const state = buildState(
+ {
+ matrixify_enable: true,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'topn',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators.length).toBeGreaterThan(0);
+});
+
+// --- matrixify_topn_metric_rows ---
+
+test('matrixify_topn_metric_rows: validators empty when matrixify_enable is falsy', () => {
+ const control = matrixifyControls.matrixify_topn_metric_rows;
+ const state = buildState(
+ {
+ matrixify_enable: undefined,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'topn',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators).toEqual([]);
+});
+
+test('matrixify_topn_metric_rows: validators present when matrixify_enable is true', () => {
+ const control = matrixifyControls.matrixify_topn_metric_rows;
+ const state = buildState(
+ {
+ matrixify_enable: true,
+ matrixify_mode_rows: 'dimensions',
+ matrixify_dimension_selection_mode_rows: 'topn',
+ },
+ { matrixify_mode_rows: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators.length).toBeGreaterThan(0);
+});
+
+// ============================================================
+// Validator injection tests via real mapStateToProps (columns)
+// ============================================================
+
+test('matrixify_dimension_columns: validators empty when matrixify_enable is falsy', () => {
+ const control = matrixifyControls.matrixify_dimension_columns;
+ const state = buildState(
+ {
+ matrixify_enable: undefined,
+ matrixify_mode_columns: 'dimensions',
+ matrixify_dimension_selection_mode_columns: 'members',
+ },
+ { matrixify_mode_columns: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators).toEqual([]);
+});
+
+test('matrixify_dimension_columns: validators present when matrixify_enable is true', () => {
+ const control = matrixifyControls.matrixify_dimension_columns;
+ const state = buildState(
+ {
+ matrixify_enable: true,
+ matrixify_mode_columns: 'dimensions',
+ matrixify_dimension_selection_mode_columns: 'members',
+ },
+ { matrixify_mode_columns: 'dimensions' },
+ );
+
+ const result = control.mapStateToProps!(state, {} as any);
+ expect(result.validators.length).toBeGreaterThan(0);
+});
+
+// ============================================================
+// Direct isMatrixifyVisible guard tests
+// ============================================================
+
+test.each([
+ ['undefined', undefined],
+ ['null', null],
+ ['false', false],
+ ['0', 0],
+])(
+ 'isMatrixifyVisible returns false when matrixify_enable is %s',
+ (_, value) => {
+ const controls = buildControls({
+ matrixify_enable: value,
+ matrixify_mode_rows: 'dimensions',
+ });
+ expect(isMatrixifyVisible(controls, 'rows')).toBe(false);
+ },
+);
+
+test('isMatrixifyVisible returns true when matrixify_enable is true and mode matches', () => {
+ const controls = buildControls({
+ matrixify_enable: true,
+ matrixify_mode_rows: 'dimensions',
+ });
+ expect(isMatrixifyVisible(controls, 'rows', 'dimensions')).toBe(true);
+});
+
+test('isMatrixifyVisible returns false when matrixify_enable is true but mode is disabled', () => {
+ const controls = buildControls({
+ matrixify_enable: true,
+ matrixify_mode_rows: 'disabled',
+ });
+ expect(isMatrixifyVisible(controls, 'rows')).toBe(false);
+});
+
+test('isMatrixifyVisible returns true when matrixify_enable is true and any non-disabled mode (no mode filter)', () => {
+ const controls = buildControls({
+ matrixify_enable: true,
+ matrixify_mode_columns: 'metrics',
+ });
+ expect(isMatrixifyVisible(controls, 'columns')).toBe(true);
+});
diff --git a/superset-frontend/src/explore/store.test.tsx b/superset-frontend/src/explore/store.test.tsx
index 602390a4d4f..7df092df274 100644
--- a/superset-frontend/src/explore/store.test.tsx
+++ b/superset-frontend/src/explore/store.test.tsx
@@ -17,55 +17,201 @@
* under the License.
*/
import { getChartControlPanelRegistry } from '@superset-ui/core';
-import { applyDefaultFormData } from 'src/explore/store';
-
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('store', () => {
- beforeAll(() => {
- getChartControlPanelRegistry().registerValue('test-chart', {
- controlPanelSections: [
- {
- label: 'Test section',
- expanded: true,
- controlSetRows: [['row_limit']],
- },
- ],
- });
- });
+import { applyDefaultFormData, getControlsState } from 'src/explore/store';
- afterAll(() => {
- getChartControlPanelRegistry().remove('test-chart');
- });
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+(window as any).featureFlags = {};
- // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
- describe('applyDefaultFormData', () => {
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (window as any).featureFlags = {};
-
- test('applies default to formData if the key is missing', () => {
- const inputFormData = {
- datasource: '11_table',
- viz_type: 'test-chart',
- };
- let outputFormData = applyDefaultFormData(inputFormData);
- expect(outputFormData.row_limit).toEqual(10000);
-
- const inputWithRowLimit = {
- ...inputFormData,
- row_limit: 888,
- };
- outputFormData = applyDefaultFormData(inputWithRowLimit);
- expect(outputFormData.row_limit).toEqual(888);
- });
-
- test('keeps null if key is defined with null', () => {
- const inputFormData = {
- datasource: '11_table',
- viz_type: 'test-chart',
- row_limit: null,
- };
- const outputFormData = applyDefaultFormData(inputFormData);
- expect(outputFormData.row_limit).toBe(null);
- });
+beforeAll(() => {
+ getChartControlPanelRegistry().registerValue('test-chart', {
+ controlPanelSections: [
+ {
+ label: 'Test section',
+ expanded: true,
+ controlSetRows: [['row_limit']],
+ },
+ ],
});
});
+
+afterAll(() => {
+ getChartControlPanelRegistry().remove('test-chart');
+});
+
+// Helper: build ExploreState for getControlsState
+const buildExploreState = (controlOverrides: Record<string, any> = {}) => ({
+ datasource: { type: 'table' },
+ controls: Object.fromEntries(
+ Object.entries(controlOverrides).map(([k, v]) => [k, { value: v }]),
+ ),
+});
+
+// ============================================================
+// Existing applyDefaultFormData tests
+// ============================================================
+
+test('applyDefaultFormData applies default to formData if the key is missing', () => {
+ const inputFormData = {
+ datasource: '11_table',
+ viz_type: 'test-chart',
+ };
+ let outputFormData = applyDefaultFormData(inputFormData);
+ expect(outputFormData.row_limit).toEqual(10000);
+
+ const inputWithRowLimit = {
+ ...inputFormData,
+ row_limit: 888,
+ };
+ outputFormData = applyDefaultFormData(inputWithRowLimit);
+ expect(outputFormData.row_limit).toEqual(888);
+});
+
+test('applyDefaultFormData keeps null if key is defined with null', () => {
+ const inputFormData = {
+ datasource: '11_table',
+ viz_type: 'test-chart',
+ row_limit: null,
+ };
+ const outputFormData = applyDefaultFormData(inputFormData);
+ expect(outputFormData.row_limit).toBe(null);
+});
+
+// ============================================================
+// Migration tests: handleDeprecatedControls cleans stale matrixify modes
+// (TDD: tests for fix to apache/superset#38519 regression)
+// ============================================================
+
+test('getControlsState resets stale matrixify_mode_rows to disabled when matrixify_enable key absent', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_mode_rows: 'dimensions', // stale in form_data
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ const modeControl = result.matrixify_mode_rows as any;
+ expect(modeControl?.value).toBe('disabled');
+});
+
+test('getControlsState resets stale matrixify_mode_columns to disabled when matrixify_enable key absent', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_mode_columns: 'metrics', // stale in form_data
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ const modeControl = result.matrixify_mode_columns as any;
+ expect(modeControl?.value).toBe('disabled');
+});
+
+test('getControlsState preserves matrixify mode values when matrixify_enable is true', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_enable: true,
+ matrixify_mode_rows: 'dimensions',
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ const modeControl = result.matrixify_mode_rows as any;
+ expect(modeControl?.value).toBe('dimensions');
+});
+
+test('getControlsState preserves matrixify mode values when matrixify_enable is explicitly false', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_enable: false,
+ matrixify_mode_rows: 'dimensions',
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ const modeControl = result.matrixify_mode_rows as any;
+ // matrixify_enable key IS present (just false) — migration should NOT fire
+ expect(modeControl?.value).toBe('dimensions');
+});
+
+test('getControlsState is idempotent when matrixify modes already disabled', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_mode_rows: 'disabled',
+ matrixify_mode_columns: 'disabled',
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ expect((result.matrixify_mode_rows as any)?.value).toBe('disabled');
+ expect((result.matrixify_mode_columns as any)?.value).toBe('disabled');
+});
+
+test('getControlsState handles form_data with no matrixify keys', () => {
+ const state = buildExploreState();
+ const formData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ };
+
+ const result = getControlsState(state as any, formData as any);
+ // Controls should get their defaults — matrixify_mode defaults to 'disabled'
+ expect((result.matrixify_mode_rows as any)?.value).toBe('disabled');
+ expect((result.matrixify_mode_columns as any)?.value).toBe('disabled');
+});
+
+test('getControlsState round-trip: pre-revamp form_data produces no matrixify validation errors', () => {
+ // Simulate a chart saved before #38519 with stale matrixify defaults
+ // Empty controls: on real first-load hydration, no pre-existing controls exist
+ const state = buildExploreState();
+ const preRevampFormData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ // Stale old defaults — no matrixify_enable key (legacy chart)
+ matrixify_mode_rows: 'dimensions',
+ matrixify_mode_columns: 'metrics',
+ };
+
+ const result = getControlsState(state as any, preRevampFormData as any);
+
+ // Every matrixify control should have zero validation errors
+ const matrixifyControlEntries = Object.entries(result).filter(([name]) =>
+ name.startsWith('matrixify_'),
+ );
+ const controlsWithErrors = matrixifyControlEntries.filter(
+ ([, control]) => (control as any)?.validationErrors?.length > 0,
+ );
+
+ expect(controlsWithErrors).toEqual([]);
+});
+
+// ============================================================
+// Dashboard hydration: applyDefaultFormData with stale form_data
+// ============================================================
+
+test('applyDefaultFormData with pre-revamp form_data: stale modes pass through but are harmless', () => {
+ // Documents dashboard hydration behavior: applyDefaultFormData passes null
+ // state to getAllControlsState, so mapStateToProps is never invoked and
+ // validators are never injected. Stale matrixify modes pass through unchanged
+ // but cannot cause validation errors on this path. The runtime guard in
+ // isMatrixifyVisible covers the case where controls ARE evaluated with state
+ // (e.g., re-render after dashboard hydration).
+ const preRevampFormData = {
+ datasource: '1__table',
+ viz_type: 'test-chart',
+ matrixify_mode_rows: 'dimensions',
+ matrixify_mode_columns: 'metrics',
+ // No matrixify_enable key — legacy chart
+ };
+
+ const outputFormData = applyDefaultFormData(preRevampFormData as any);
+
+ // Stale values pass through (no handleDeprecatedControls on this path)
+ expect(outputFormData.matrixify_mode_rows).toBe('dimensions');
+ expect(outputFormData.matrixify_mode_columns).toBe('metrics');
+ // matrixify_enable gets its default (false) from the control definition
+ expect(outputFormData.matrixify_enable).toBe(false);
+});
diff --git a/superset-frontend/src/explore/store.ts b/superset-frontend/src/explore/store.ts
index b1189efff44..906916ed110 100644
--- a/superset-frontend/src/explore/store.ts
+++ b/superset-frontend/src/explore/store.ts
@@ -41,6 +41,9 @@ type FormData = QueryFormData & {
y_axis_zero?: boolean;
y_axis_bounds?: [number | null, number | null];
datasource?: string;
+ matrixify_enable?: boolean;
+ matrixify_mode_rows?: string;
+ matrixify_mode_columns?: string;
};
function handleDeprecatedControls(formData: FormData): void {
@@ -51,6 +54,15 @@ function handleDeprecatedControls(formData: FormData): void {
if (formData.y_axis_zero) {
formData.y_axis_bounds = [0, null];
}
+
+ // #38519: reset stale matrixify mode defaults for legacy charts.
+ // Pre-revamp charts have matrixify_mode_rows/columns set to non-disabled
+ // values but no matrixify_enable key. Without this cleanup, the stale modes
+ // cause isMatrixifyVisible() to inject validators that silently block save.
+ if (!('matrixify_enable' in formData)) {
+ formData.matrixify_mode_rows = 'disabled';
+ formData.matrixify_mode_columns = 'disabled';
+ }
}
export function getControlsState(