You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by en...@apache.org on 2026/08/27 13:13:00 UTC
(superset) 09/46: fix(dashboard-v2): fail open on missing/non-string picker data, drop dead metric control
This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch enxdev/poc/dashboard-v2-editing-ui-flow
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 27640658e97b6f364b4bb4d8272ffd41af23a63d
Author: Enzo Martellucci <en...@gmail.com>
AuthorDate: Thu Aug 20 17:01:06 2026 +0200
fix(dashboard-v2): fail open on missing/non-string picker data, drop dead metric control
Cross-cutting cleanup from the final whole-branch review of the
dataset-aware widget controls work:
- ColumnControl/ColumnMultiControl now fall back to CodeControl when
a value isn't a string (mirroring MetricMultiControl's existing
guard), instead of crashing the builder on a hand-authored non-string
value with no ErrorBoundary to catch it.
- All column/metric controls fail open to CodeControl when no dataset
is bound yet or the dataset metadata fetch errors, instead of
silently rendering an uneditable field (the design spec's fail-open
requirement was previously unimplemented).
- Removed MetricControl and its `x-control: "metric"` registry entry:
no backend field ever declares it, so it was unreachable dead code
with zero coverage.
- Gave ReferenceMultiList's trailing "add" Select an accessible name
so tests can target a specific multi-list, and rewrote the nested
dataBinding.* schema test to actually drive dimensions and metrics
through their real write-back paths instead of sidestepping them.
---
.../DashboardBuilderV2/SchemaControlPanel.test.tsx | 156 +++++++++++++++++++--
.../DashboardBuilderV2/schemaControlRenderers.tsx | 90 +++++++-----
2 files changed, 203 insertions(+), 43 deletions(-)
diff --git a/superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.test.tsx
index 257f380beca..bae8e0bf11a 100644
--- a/superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.test.tsx
+++ b/superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.test.tsx
@@ -17,6 +17,7 @@
* under the License.
*/
import { SupersetClient } from '@superset-ui/core';
+import type { dashboard as dashboardApi } from '@apache-superset/core';
import {
render,
screen,
@@ -29,6 +30,8 @@ import { fetchQueryData } from 'src/core/dashboard/chartData';
import { resetDatasetMetadataCacheForTests } from 'src/core/dashboard/datasetMetadata';
import SchemaControlPanel from './SchemaControlPanel';
+type DataBindingSpec = dashboardApi.DataBindingSpec;
+
jest.mock('src/core/dashboard/chartData', () => ({
fetchQueryData: jest.fn().mockResolvedValue({ columns: [], rows: [] }),
}));
@@ -307,6 +310,130 @@ test('falls back to the raw JSON editor when an existing metric entry is an ad-h
expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
});
+test('falls back to the raw JSON editor when a column value is not a string', async () => {
+ postSpy.mockResolvedValue({
+ json: {
+ result: {
+ type: 'object',
+ properties: {
+ colorDimension: {
+ type: 'string',
+ title: 'Color dimension',
+ 'x-control': 'column',
+ },
+ },
+ },
+ },
+ } as never);
+ getSpy.mockResolvedValue({
+ json: {
+ result: {
+ columns: [{ column_name: 'gender', type_generic: 1 }],
+ metrics: [],
+ },
+ },
+ } as never);
+
+ // Hand-authored through the Inspector's JSON tab: not a string, so antd
+ // `Select` can't take it as a `value` — this must fall back to the raw
+ // JSON editor rather than crash the whole builder.
+ mount('balloons', {
+ dataBinding: { datasetId: 1, metrics: ['count'] },
+ colorDimension: { expressionType: 'SIMPLE' },
+ });
+
+ expect(await screen.findByText('Color dimension')).toBeInTheDocument();
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+});
+
+test('falls back to the raw JSON editor when a column-multi entry is not a string', async () => {
+ postSpy.mockResolvedValue({
+ json: {
+ result: {
+ type: 'object',
+ properties: {
+ dimensions: {
+ type: 'array',
+ title: 'Dimensions',
+ 'x-control': 'column-multi',
+ },
+ },
+ },
+ },
+ } as never);
+ getSpy.mockResolvedValue({
+ json: {
+ result: {
+ columns: [{ column_name: 'gender', type_generic: 1 }],
+ metrics: [],
+ },
+ },
+ } as never);
+
+ mount('balloons', {
+ dataBinding: { datasetId: 1, metrics: ['count'] },
+ dimensions: ['gender', { expressionType: 'SIMPLE' }],
+ });
+
+ expect(await screen.findByText('Dimensions')).toBeInTheDocument();
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+});
+
+test('falls back to the raw JSON editor when no dataset is bound yet', async () => {
+ postSpy.mockResolvedValue({
+ json: {
+ result: {
+ type: 'object',
+ properties: {
+ dimensions: {
+ type: 'array',
+ title: 'Dimensions',
+ 'x-control': 'column-multi',
+ },
+ },
+ },
+ },
+ } as never);
+
+ mount('balloons', { dataBinding: {} });
+
+ expect(await screen.findByText('Dimensions')).toBeInTheDocument();
+ // No dataset bound at all: the field must stay editable via the raw JSON
+ // editor rather than render an empty, permanently uneditable picker.
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+ expect(getSpy).not.toHaveBeenCalled();
+});
+
+test('falls back to the raw JSON editor when the dataset metadata fetch fails', async () => {
+ postSpy.mockResolvedValue({
+ json: {
+ result: {
+ type: 'object',
+ properties: {
+ colorDimension: {
+ type: 'string',
+ title: 'Color dimension',
+ 'x-control': 'column',
+ },
+ },
+ },
+ },
+ } as never);
+ getSpy.mockRejectedValue(new Error('network error'));
+
+ mount('balloons', { dataBinding: { datasetId: 1, metrics: ['count'] } });
+
+ // Both assertions must be checked together on every retry: before the
+ // fetch settles, `ColumnControl` briefly renders the picker instead (whose
+ // `Form.Item` label also reads "Color dimension"), so checking the label
+ // and the missing combobox separately could pass on that transient,
+ // pre-failure render instead of the final, fallen-back one.
+ await waitFor(() => {
+ expect(screen.getByText('Color dimension')).toBeInTheDocument();
+ expect(screen.queryByRole('combobox')).not.toBeInTheDocument();
+ });
+});
+
test('the real balloons schema shape (dataBinding nested under $defs) renders pickers for dimensions, metrics, and colorDimension', async () => {
postSpy.mockResolvedValue({
json: {
@@ -350,17 +477,28 @@ test('the real balloons schema shape (dataBinding nested under $defs) renders pi
},
} as never);
- // `metrics`/`dimensions` are pre-filled with the only known metric/column
- // so neither's "Add field" select renders — otherwise `selectOption`
- // would find three comboboxes (dimensions' add-select, metrics'
- // add-select, and colorDimension's) instead of the one it expects.
- const id = mount('balloons', {
- dataBinding: { datasetId: 1, metrics: ['count'], dimensions: ['gender'] },
- });
+ // Nothing pre-filled: all three picks below drive their real, nested
+ // `dataBinding.*` write-back paths.
+ const id = mount('balloons', { dataBinding: { datasetId: 1 } });
- await screen.findByText('Color dimension');
+ await screen.findByText('Dimensions');
+ // Each multi-list's trailing "add" Select carries an accessible name
+ // derived from its field label (`Add ${label}`), so it can be targeted
+ // directly even with multiple unnamed comboboxes on screen at once.
+ const getBinding = () =>
+ provider.getNode(id)?.props?.dataBinding as DataBindingSpec | undefined;
+
+ await selectOption('gender', 'Add Dimensions');
+ await waitFor(() => expect(getBinding()?.dimensions).toContain('gender'));
+
+ await selectOption('Count', 'Add Metrics');
+ await waitFor(() => expect(getBinding()?.metrics).toContain('count'));
+
+ // The dataset mock has exactly one column and one metric, so both
+ // multi-lists' "add" selects are now gone (nothing left to add) — leaving
+ // `colorDimension`'s plain Select as the only combobox on screen, so the
+ // unnamed pick below still resolves to a single element.
await selectOption('gender');
-
await waitFor(() =>
expect(provider.getNode(id)?.props?.colorDimension).toBe('gender'),
);
diff --git a/superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx b/superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx
index c6d218ef8ed..cbf57501e0d 100644
--- a/superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx
+++ b/superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx
@@ -24,7 +24,6 @@
* - `x-control: color` → a native color swatch (e.g. per-series balloon color).
* - `x-control: column` → a single column-reference picker.
* - `x-control: column-multi` → an ordered, reorderable list of column references.
- * - `x-control: metric` → a single metric-reference picker.
* - `x-control: metric-multi` → an ordered, reorderable list of metric references.
*
* `code` and `color` fall back to the field's schema `default`, since
@@ -143,6 +142,21 @@ function useBoundDatasetId(props: ControlProps): number | undefined {
return dataBinding?.datasetId;
}
+/**
+ * True when a column/metric-reference control should fail open to the raw
+ * JSON editor (`CodeControl`) rather than render its picker: no dataset is
+ * bound yet, or the dataset fetch failed. Deliberately does NOT cover the
+ * in-flight loading state (`metadata` still `null`, no `error` yet) — that's
+ * the normal case while a bound dataset's metadata is fetched, and the
+ * picker renders as usual with its own `loading` flag set.
+ */
+function shouldFallBackToCode(
+ datasetId: number | undefined,
+ error: string | null,
+): boolean {
+ return datasetId === undefined || error !== null;
+}
+
interface ReferenceOption {
value: string;
label: ReactNode;
@@ -282,6 +296,7 @@ function ReferenceMultiList({
<Select
value={undefined}
placeholder={t('Add field')}
+ ariaLabel={t('Add %s', label)}
options={available}
loading={loading}
disabled={disabled}
@@ -293,13 +308,27 @@ function ReferenceMultiList({
);
}
-/** `x-control: "column"` — a single column reference. */
+/**
+ * `x-control: "column"` — a single column reference. Falls back to the raw
+ * JSON editor when no dataset is bound (or its fetch failed), or when the
+ * existing value isn't a string — e.g. an object hand-authored into the
+ * field through the Inspector's JSON tab, which `Select` can't render as a
+ * `value` and JsonForms would otherwise crash on.
+ */
function ColumnControl(props: ControlProps): ReactElement {
const datasetId = useBoundDatasetId(props);
- const { metadata, loading } = useDatasetMetadata(datasetId);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
const allowedTypes = (props.schema as Record<string, unknown>)[
'x-column-types'
] as string[] | undefined;
+
+ if (
+ shouldFallBackToCode(datasetId, error) ||
+ (props.data !== undefined && typeof props.data !== 'string')
+ ) {
+ return <CodeControl {...props} />;
+ }
+
return (
<ReferenceSelect
label={props.label}
@@ -312,18 +341,30 @@ function ColumnControl(props: ControlProps): ReactElement {
);
}
-/** `x-control: "column-multi"` — an ordered list of column references. */
+/**
+ * `x-control: "column-multi"` — an ordered list of column references. Falls
+ * back to the raw JSON editor when no dataset is bound (or its fetch
+ * failed), or when an existing entry isn't a string — e.g. an object
+ * hand-authored into the field through the Inspector's JSON tab, which
+ * `ReferenceMultiList` can't render as a list entry.
+ */
function ColumnMultiControl(props: ControlProps): ReactElement {
const datasetId = useBoundDatasetId(props);
- const { metadata, loading } = useDatasetMetadata(datasetId);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
const allowedTypes = (props.schema as Record<string, unknown>)[
'x-column-types'
] as string[] | undefined;
- const values = Array.isArray(props.data) ? (props.data as string[]) : [];
+ const values = Array.isArray(props.data) ? (props.data as unknown[]) : [];
+ const hasNonStringEntry = values.some(value => typeof value !== 'string');
+
+ if (shouldFallBackToCode(datasetId, error) || hasNonStringEntry) {
+ return <CodeControl {...props} />;
+ }
+
return (
<ReferenceMultiList
label={props.label}
- values={values}
+ values={values as string[]}
options={columnOptions(metadata, allowedTypes)}
loading={loading}
disabled={!props.enabled}
@@ -333,8 +374,8 @@ function ColumnMultiControl(props: ControlProps): ReactElement {
}
/**
- * Metric options for a `metric`/`metric-multi` control: the dataset's saved
- * metrics, shown with the same Sigma icon Explore's metric picker uses.
+ * Metric options for a `metric-multi` control: the dataset's saved metrics,
+ * shown with the same Sigma icon Explore's metric picker uses.
*/
export function metricOptions(
metadata: DatasetMetadata | null,
@@ -364,31 +405,16 @@ export function hasAdvancedMetric(
return values.some(value => typeof value !== 'string' || !known.has(value));
}
-/** `x-control: "metric"` — a single metric reference. */
-function MetricControl(props: ControlProps): ReactElement {
- const datasetId = useBoundDatasetId(props);
- const { metadata, loading } = useDatasetMetadata(datasetId);
- return (
- <ReferenceSelect
- label={props.label}
- value={props.data as string | undefined}
- options={metricOptions(metadata)}
- loading={loading}
- disabled={!props.enabled}
- onChange={value => props.handleChange(props.path, value)}
- />
- );
-}
-
/**
* `x-control: "metric-multi"` — an ordered list of metric references. Falls
- * back to the raw JSON editor (`CodeControl`) whenever an existing entry
- * isn't expressible as a saved-metric pick, e.g. an ad-hoc aggregate object
- * authored through the JSON tab.
+ * back to the raw JSON editor (`CodeControl`) whenever no dataset is bound
+ * (or its fetch failed), or an existing entry isn't expressible as a
+ * saved-metric pick, e.g. an ad-hoc aggregate object authored through the
+ * JSON tab.
*/
function MetricMultiControl(props: ControlProps): ReactElement {
const datasetId = useBoundDatasetId(props);
- const { metadata, loading } = useDatasetMetadata(datasetId);
+ const { metadata, loading, error } = useDatasetMetadata(datasetId);
const values = Array.isArray(props.data) ? (props.data as unknown[]) : [];
// Before `metadata` loads, `hasAdvancedMetric` can't be run (it needs the
@@ -400,7 +426,7 @@ function MetricMultiControl(props: ControlProps): ReactElement {
? hasAdvancedMetric(values, metadata)
: values.some(value => typeof value !== 'string');
- if (isAdvanced) {
+ if (shouldFallBackToCode(datasetId, error) || isAdvanced) {
return <CodeControl {...props} />;
}
@@ -435,10 +461,6 @@ export const schemaControlRenderers = [
tester: rankWith(1000, xControlIs('column-multi')),
renderer: withJsonFormsControlProps(ColumnMultiControl),
},
- {
- tester: rankWith(1000, xControlIs('metric')),
- renderer: withJsonFormsControlProps(MetricControl),
- },
{
tester: rankWith(1000, xControlIs('metric-multi')),
renderer: withJsonFormsControlProps(MetricMultiControl),