Copilot commented on code in PR #43588: URL: https://github.com/apache/superset/pull/43588#discussion_r3893200989
########## superset-frontend/src/pages/DashboardBuilderV2/controlValueValidation.ts: ########## @@ -0,0 +1,111 @@ +/** + * 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. + */ + +/** + * The one path either half of the Properties panel (the schema-driven form + * and the JSON editor) writes a schema-controlled widget's control values + * through. + * + * Both representations edit the same `node.props`, but neither may write to + * it directly: a candidate is merged, sent to the backend's + * `Widget.validate_control_values` gate — the same gate the + * `set_widget_control_values` MCP tool commits through, reached here via its + * REST wrapper rather than a second, frontend-authored copy of its rules — + * and only committed to the store once that gate accepts it. A rejected + * candidate returns its errors and touches nothing: `node.props` is read + * here, never written to, until validation has already succeeded. + */ +import { SupersetClient } from '@superset-ui/core'; +import { provider } from 'src/core/dashboard/store'; + +export type ControlValidationError = { + loc: (string | number)[]; + message: string; +}; + +export type CommitPropsResult = + | { ok: true; values: Record<string, unknown> } + | { ok: false; errors: ControlValidationError[] }; + +/** + * SupersetClient rejects a non-2xx response with the raw, unparsed `Response` + * object rather than an `Error`, so a plain `String(e)` yields the useless + * "[object Response]". Pull the actual `{message}`/`{errors:[...]}` body + * Superset sends back (same shape `chartData.ts` handles). + */ +export async function describeError(e: unknown): Promise<string> { + if (typeof Response !== 'undefined' && e instanceof Response) { + try { + const body = await e.clone().json(); + const detail = + body?.message ?? + (Array.isArray(body?.errors) + ? body.errors + .map((err: { message?: string }) => err.message) + .join('; ') + : undefined); + return detail + ? `${e.status} ${e.statusText}: ${detail}` + : `${e.status} ${e.statusText}`; + } catch { + return `${e.status} ${e.statusText}`; + } + } + return e instanceof Error ? e.message : String(e); +} + +async function validateControlValues( + widgetType: string, + controlValues: Record<string, unknown>, +): Promise<ControlValidationError[]> { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/widgets/type/${widgetType}/validate`, + jsonPayload: { control_values: controlValues }, + }); + return (json as { result: { errors: ControlValidationError[] } }).result + .errors; +} + +/** + * Merges `delta` onto the node's current props, validates the merged + * candidate, and commits it to `node.props` only if the backend accepts it. + * + * `onBeforeCommit`, if given, runs synchronously immediately before the + * `provider.updateProps` call — not part of the merge/validate/commit + * contract itself, but the one hook a caller needs to set a flag in the + * exact tick its own commit lands (e.g. `SchemaControlPanel` telling its + * own resync effect "this `props` change was mine"), without racing the + * async validation round-trip to do it. + */ +export async function commitWidgetProps( + nodeId: string, + widgetType: string, + delta: Record<string, unknown>, + options?: { onBeforeCommit?: () => void }, +): Promise<CommitPropsResult> { + const node = provider.getNode(nodeId); + const candidate = { ...node?.props, ...delta }; + const errors = await validateControlValues(widgetType, candidate); + if (errors.length > 0) { Review Comment: Validating the complete model on every individual field change prevents configuring a newly placed data-backed widget. `placeBlock` creates it without props, while `DataBinding` requires both `datasetId` and `metrics`; choosing the dataset submits only that field and is rejected for the missing metrics, but the metric picker cannot work until a dataset has been accepted. Seed a valid initial binding or support draft/partial edits and validate at an explicit commit boundary. ########## superset-core/src/superset_core/widgets/enrichment.py: ########## @@ -0,0 +1,150 @@ +# 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. + +""" +Dependency graph and execution engine for a widget's dynamic (``x-dynamic``) +control-schema fields. + +A widget registers one enricher callable per dynamic field path (see +``Widget.enrichers``). Each field's existing ``x-dependsOn`` list (already +used to gate enrichment via ``check_dependencies``) does double duty here: +an entry naming another dynamic field's path becomes an ordering edge (that +field's enricher must run first, and this one receives its result); any +other entry stays a plain truthiness gate against the parsed control values. +Field paths use ``a/b`` dot-path notation, the same convention +``schema_tools.py`` uses for drill-in paths. + +This module only computes the graph and runs enrichers in order — it has no +opinion on where the schema or enrichers come from; ``Widget.get_control_schema`` +wires it up. +""" + +from __future__ import annotations + +from typing import Any, Callable + +from pydantic import BaseModel + +from superset_core.semantic_layers.config import check_dependencies + +# (schema, node, parsed, series, upstream_results) -> Any. +# `schema` is the full document (for cross-$defs lookups, e.g. a sibling +# style definition); `node` is this field's own schema fragment, mutated in +# place. The return value is threaded to enrichers ordered after this one, as +# `upstream_results[path]`. +EnricherFn = Callable[ + [dict[str, Any], dict[str, Any], "BaseModel | None", list[str], dict[str, Any]], + Any, +] + + +def _defs(schema: dict[str, Any]) -> dict[str, Any]: + return schema.get("$defs", {}) or {} + + +def _deref(schema: dict[str, Any], node: dict[str, Any]) -> dict[str, Any]: + if "$ref" not in node: + return node + return _defs(schema).get(node["$ref"].split("/")[-1], {}) + + +def dynamic_field_paths(schema: dict[str, Any]) -> dict[str, dict[str, Any]]: + """Walk a built (pre-enrichment) control schema and return + ``{path: schema_node}`` for every field carrying ``x-dynamic: true``, + using ``a/b`` dot-path notation. Descends into ``properties`` and + resolves ``$ref`` against ``$defs`` along the way; does not descend into + a discovered dynamic field itself (a dynamic field's own internals are + the enricher's concern, not the graph's).""" + fields: dict[str, dict[str, Any]] = {} + + def _walk(node: dict[str, Any], prefix: str) -> None: + resolved = _deref(schema, node) + for name, prop in resolved.get("properties", {}).items(): + path = f"{prefix}/{name}" if prefix else name + prop_resolved = _deref(schema, prop) + if prop_resolved.get("x-dynamic"): + fields[path] = prop_resolved + else: + _walk(prop, path) + + _walk(schema, "") + return fields + + +def build_dependency_graph(fields: dict[str, dict[str, Any]]) -> dict[str, list[str]]: + """``{path: [ordering-edge paths]}`` for every dynamic field in + ``fields``. Only ``x-dependsOn`` entries that name another key of + ``fields`` become edges; every other entry is left as a gate for + ``check_dependencies`` to evaluate at run time, not an edge here.""" + return { + path: [dep for dep in node.get("x-dependsOn", []) if dep in fields] + for path, node in fields.items() + } + + +def toposort_or_raise(graph: dict[str, list[str]], widget_type: str) -> list[str]: + """Kahn's algorithm over ``graph`` (``path -> [paths it depends on]``). + Raises ``ValueError`` naming every field on a cycle when the graph isn't + a DAG.""" + in_degree = dict.fromkeys(graph, 0) + dependents: dict[str, list[str]] = {path: [] for path in graph} + for path, deps in graph.items(): + in_degree[path] = len(deps) + for dep in deps: + dependents[dep].append(path) + + ready = sorted(path for path, degree in in_degree.items() if degree == 0) + order: list[str] = [] + while ready: + path = ready.pop(0) + order.append(path) + for dependent in sorted(dependents[path]): + in_degree[dependent] -= 1 + if in_degree[dependent] == 0: + ready.append(dependent) + + if len(order) != len(graph): + remaining = sorted(set(graph) - set(order)) + raise ValueError( + f"Cyclic control dependency in widget {widget_type!r} among: " + f"{', '.join(remaining)}" + ) + return order + + +def run_enrichers( + schema: dict[str, Any], + fields: dict[str, dict[str, Any]], + order: list[str], + enrichers: dict[str, EnricherFn], + parsed: BaseModel | None, + series: list[str], +) -> None: + """Run each path's registered enricher (if any) in ``order``, skipping + one whose non-edge ``x-dependsOn`` gate(s) aren't satisfied, and + threading each enricher's return value forward as + ``upstream_results[path]`` for anything ordered after it.""" + upstream_results: dict[str, Any] = {} + for path in order: + enricher = enrichers.get(path) + if enricher is None: + continue + node = fields[path] + if parsed is not None and not check_dependencies(node, parsed): + continue + result = enricher(schema, node, parsed, series, upstream_results) Review Comment: Ordering-edge dependencies are still passed to `check_dependencies`, contrary to this function’s contract that only non-edge dependencies are value gates. For a dynamic dependency path such as `group/options`, attribute lookup can never succeed, so the downstream enricher is skipped even after its upstream enricher ran. Filter dynamic paths out before checking the parsed control values. ########## superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx: ########## @@ -154,7 +157,18 @@ export default function SchemaControlPanel({ nodeId }: { nodeId: string }) { const [series, setSeries] = useState<string[]>([]); const [schema, setSchema] = useState<JsonSchema | null>(null); + const [formKey, setFormKey] = useState(0); const [error, setError] = useState<string | null>(null); + const [validationErrors, setValidationErrors] = useState< + ControlValidationError[] + >([]); + // Guards against an earlier, slower validation round-trip landing after a + // later one — e.g. two edits typed close together — and overwriting the + // more recent result with a stale one. + const validateSeqRef = useRef(0); + // A validation error belongs to the widget it was raised for; selecting a + // different one shouldn't leave a stale message on screen. + useEffect(() => setValidationErrors([]), [nodeId]); Review Comment: Changing `nodeId` only clears errors; the schema-loading effect is keyed by `widgetType` and discovered series. Selecting another widget of the same type with the same series key therefore retains the previous widget’s enriched schema—for example, ECharts per-metric override controls from the prior widget. Include `nodeId` in schema invalidation/fetching and invalidate outstanding refreshes on selection changes. ########## superset-frontend/src/pages/DashboardBuilderV2/DataPanel.tsx: ########## @@ -0,0 +1,237 @@ +/** + * 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 { useState } from 'react'; +import type { ReactElement } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { GenericDataType } from '@apache-superset/core/common'; +import { EmptyState, Input } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { ColumnTypeLabel } from '@superset-ui/chart-controls'; + +interface MockColumn { + readonly name: string; + readonly type: GenericDataType; +} + +interface MockDataset { + readonly id: string; + readonly name: string; + readonly columns: readonly MockColumn[]; +} + +/** + * Static placeholder rows. The Data tab does not call the dataset API — + * a later change wires this list, and each dataset's columns, to + * `/api/v1/dataset/`, the same endpoint `datasetMetadata.ts` already reads a + * single bound dataset's columns from. + */ +const MOCK_DATASETS: readonly MockDataset[] = [ + { + id: 'sales', + name: 'sales', + columns: [ + { name: 'order_id', type: GenericDataType.String }, + { name: 'order_date', type: GenericDataType.Temporal }, + { name: 'sales_amount', type: GenericDataType.Numeric }, + { name: 'region', type: GenericDataType.String }, + ], + }, + { + id: 'coffee_sales', + name: 'coffee_sales', + columns: [ + { name: 'product', type: GenericDataType.String }, + { name: 'roast_date', type: GenericDataType.Temporal }, + { name: 'unit_price', type: GenericDataType.Numeric }, + { name: 'is_decaf', type: GenericDataType.Boolean }, + ], + }, +]; + +const matches = (dataset: MockDataset, query: string): boolean => + query.trim() === '' || + dataset.name.toLowerCase().includes(query.trim().toLowerCase()); + +/** + * The panel's own scroll column, set down from the tab bar and in from the + * panel edge — the same step `Palette`'s `Column` and `Outline`'s `Panel` + * take from theirs, so the four tabs of one rail start on one line. + */ +const Column = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 5}px; + min-height: 0; + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit}px 0; + `} +`; + +const List = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + overflow-y: auto; + min-height: 0; + `} +`; + +const DatasetButton = styled.button` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + width: 100%; + padding: ${theme.sizeUnit * 2}px; + border: 1px solid ${theme.colorBorder}; + /* The same radius every other tile in this rail, and the canvas itself, + draws its own card at — one rounding language, not one per panel. */ + border-radius: ${theme.borderRadiusLG}px; + background-color: ${theme.colorFillQuaternary}; + color: ${theme.colorText}; + font-size: ${theme.fontSizeSM}px; + text-align: left; + cursor: pointer; + transition: background-color ${theme.motionDurationMid}; + + .data-panel-chevron { + display: flex; + flex: 0 0 auto; + color: ${theme.colorTextTertiary}; + } + + &:hover { + background-color: ${theme.colorFillTertiary}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimaryBorder}; + outline-offset: -2px; + } + `} +`; + +const ColumnList = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + margin-top: ${theme.sizeUnit}px; + margin-left: ${theme.sizeUnit * 2}px; + padding-left: ${theme.sizeUnit * 3}px; + border-left: 1px solid ${theme.colorBorder}; + `} +`; + +const ColumnRow = styled.div` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit}px ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorText}; + `} +`; + +/** + * Datasets and their columns, to browse rather than to place. + * + * Building Blocks places widgets onto the canvas; this tab answers "what + * data is there to use" without touching any widget's binding — expanding a + * row reads its columns and nothing else happens. + */ +export default function DataPanel(): ReactElement { + const [query, setQuery] = useState(''); + const [expanded, setExpanded] = useState<ReadonlySet<string>>(new Set()); + + const found = MOCK_DATASETS.filter(dataset => matches(dataset, query)); Review Comment: This production component is unreachable: repository references show it is imported only by `DataPanel.test.tsx`, while `EditorPanel` still renders only Building Blocks, Properties, and Outline. Users can never see this dataset browser. Either wire it into the panel/navigation or remove the dead placeholder until it is usable. ########## superset-frontend/src/pages/DashboardBuilderV2/controlValueValidation.ts: ########## @@ -0,0 +1,111 @@ +/** + * 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. + */ + +/** + * The one path either half of the Properties panel (the schema-driven form + * and the JSON editor) writes a schema-controlled widget's control values + * through. + * + * Both representations edit the same `node.props`, but neither may write to + * it directly: a candidate is merged, sent to the backend's + * `Widget.validate_control_values` gate — the same gate the + * `set_widget_control_values` MCP tool commits through, reached here via its + * REST wrapper rather than a second, frontend-authored copy of its rules — + * and only committed to the store once that gate accepts it. A rejected + * candidate returns its errors and touches nothing: `node.props` is read + * here, never written to, until validation has already succeeded. + */ +import { SupersetClient } from '@superset-ui/core'; +import { provider } from 'src/core/dashboard/store'; + +export type ControlValidationError = { + loc: (string | number)[]; + message: string; +}; + +export type CommitPropsResult = + | { ok: true; values: Record<string, unknown> } + | { ok: false; errors: ControlValidationError[] }; + +/** + * SupersetClient rejects a non-2xx response with the raw, unparsed `Response` + * object rather than an `Error`, so a plain `String(e)` yields the useless + * "[object Response]". Pull the actual `{message}`/`{errors:[...]}` body + * Superset sends back (same shape `chartData.ts` handles). + */ +export async function describeError(e: unknown): Promise<string> { + if (typeof Response !== 'undefined' && e instanceof Response) { + try { + const body = await e.clone().json(); + const detail = + body?.message ?? + (Array.isArray(body?.errors) + ? body.errors + .map((err: { message?: string }) => err.message) + .join('; ') + : undefined); + return detail + ? `${e.status} ${e.statusText}: ${detail}` + : `${e.status} ${e.statusText}`; + } catch { + return `${e.status} ${e.statusText}`; + } + } + return e instanceof Error ? e.message : String(e); +} + +async function validateControlValues( + widgetType: string, + controlValues: Record<string, unknown>, +): Promise<ControlValidationError[]> { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/widgets/type/${widgetType}/validate`, + jsonPayload: { control_values: controlValues }, + }); + return (json as { result: { errors: ControlValidationError[] } }).result + .errors; +} + +/** + * Merges `delta` onto the node's current props, validates the merged + * candidate, and commits it to `node.props` only if the backend accepts it. + * + * `onBeforeCommit`, if given, runs synchronously immediately before the + * `provider.updateProps` call — not part of the merge/validate/commit + * contract itself, but the one hook a caller needs to set a flag in the + * exact tick its own commit lands (e.g. `SchemaControlPanel` telling its + * own resync effect "this `props` change was mine"), without racing the + * async validation round-trip to do it. + */ +export async function commitWidgetProps( + nodeId: string, + widgetType: string, + delta: Record<string, unknown>, + options?: { onBeforeCommit?: () => void }, +): Promise<CommitPropsResult> { + const node = provider.getNode(nodeId); + const candidate = { ...node?.props, ...delta }; + const errors = await validateControlValues(widgetType, candidate); + if (errors.length > 0) { + return { ok: false, errors }; + } + options?.onBeforeCommit?.(); + provider.updateProps(nodeId, candidate); Review Comment: Accepted responses can commit out of order. Two quick form edits start two validations; if the newer response commits first and the older response returns later, this unconditional update restores the older candidate. `SchemaControlPanel` checks its sequence only after `commitWidgetProps` has already performed this write, so it does not prevent the stale commit. Add cancellation or a latest-request/version check before updating the provider. ########## superset-frontend/src/pages/DashboardBuilderV2/Inspector.tsx: ########## @@ -435,6 +432,7 @@ const PropsEditor = ({ // area above it). `updateProps` merges, so a key absent from the form's data // is left untouched rather than removed. const schemaControlledTypes = useSchemaControlledWidgetTypes(); + const validated = schemaControlledTypes?.has(widgetType) ?? false; Review Comment: Validation fails open while the widget-type request is loading or after it fails: `validated` becomes false, yet the JSON tab remains usable and commits directly. A user can therefore bypass the new backend gate simply by applying before `/widgets/types` resolves (or after a transient failure). The JSON path should attempt validation independently and fall back only on a confirmed 404/no-schema result. ########## superset-frontend/src/core/dashboard/widgets/echartsStructuredChrome.ts: ########## @@ -0,0 +1,144 @@ +/** + * 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. + */ + +/** + * The `echarts` widget's second, independent structured layer — chart + * chrome (title/legend/tooltip/axis labels) — matching `EchartsChrome`'s + * docstring in `superset/widgets/controls.py`: every field is optional and + * applies (or not) on its own, regardless of `chartType`/`customize`. A + * field left at its default never touches `echartsOptions`; when it does + * apply, it merges onto — rather than replaces — the matching section, so + * an unmanaged sibling property there (e.g. a hand-authored `legend.orient`) + * survives. + * + * `EchartsChromeValue` is deliberately flat, not grouped into + * `title`/`legend`/`tooltip`/`xAxis`/`yAxis` sub-objects — see + * `EchartsChrome`'s own docstring: JsonForms' generated control panel only + * renders one level of nested-object properties, so a two-level-deep + * `chrome.title.text` would render as an empty group with no fields inside. + */ + +export interface EchartsChromeValue { + titleText?: string; + legendShow?: boolean; + legendPosition?: 'top' | 'bottom' | 'left' | 'right' | null; + tooltipTrigger?: 'item' | 'axis' | null; + xAxisName?: string; + xAxisRotate?: number; + xAxisFormat?: string; + yAxisName?: string; + yAxisRotate?: number; + yAxisFormat?: string; +} + +// ECharts has no single "position" property on `legend` — placement comes +// from `top`/`left` (each accepting a keyword or coordinate). This maps the +// friendlier compass-direction picker onto the pair ECharts actually reads. +const LEGEND_POSITION: Record<string, { top: string; left: string }> = { + top: { top: 'top', left: 'center' }, + bottom: { top: 'bottom', left: 'center' }, + left: { top: 'middle', left: 'left' }, + right: { top: 'middle', left: 'right' }, +}; + +function asRecord(value: unknown): Record<string, unknown> { + return value !== null && typeof value === 'object' + ? (value as Record<string, unknown>) + : {}; Review Comment: ECharts permits array-valued `title`, `legend`, `xAxis`, and `yAxis`, but arrays pass this object check and are spread into objects with numeric keys. Applying any structured chrome override to such an existing raw option therefore changes its shape and breaks the promise that unmanaged raw configuration survives. Preserve arrays and merge into the intended entry (typically the first) rather than treating them as records. ########## superset-frontend/src/pages/DashboardBuilderV2/Inspector.tsx: ########## @@ -309,23 +278,51 @@ const PropsJsonEditor = ({ {error} </p> )} + {validationErrors.map(err => ( + <p + key={`${err.loc.join('.')}:${err.message}`} + data-test="inspector-props-validation-error" + style={{ + margin: `0 0 ${theme.sizeUnit}px`, + fontSize: theme.fontSizeSM, + color: theme.colorErrorText, + }} + > + {err.loc.length > 0 ? `${err.loc.join('.')}: ` : ''} + {err.message} + </p> + ))} <div style={{ display: 'flex', gap: theme.sizeUnit }}> <Button buttonSize="xsmall" buttonStyle="primary" data-test="inspector-props-apply" - disabled={parsed === undefined || !dirty} - onClick={() => { + disabled={parsed === undefined || !dirty || submitting} + onClick={async () => { if (parsed === undefined) { return; } const removed = Object.keys(props ?? {}).filter( key => !(key in parsed!), ); - provider.updateProps(nodeId, { + const delta = { ...parsed, ...Object.fromEntries(removed.map(key => [key, undefined])), - }); + }; Review Comment: Applying `{}` removes every key by setting it to `undefined`; JSON serialization then sends an empty `control_values` object. `Widget.validate_control_values` currently returns success for any falsy value, so required fields are never checked and this new delete path commits an invalid widget. The backend must distinguish `None` (“nothing to validate”) from `{}` and validate the latter. ########## superset-core/src/superset_core/semantic_layers/config.py: ########## @@ -17,11 +17,64 @@ from __future__ import annotations -from typing import Any +from typing import Any, get_args, get_origin, Iterator from pydantic import BaseModel +def _iter_nested_models( + annotation: Any, seen: set[type[BaseModel]] +) -> Iterator[type[BaseModel]]: + """Yield every ``BaseModel`` subclass reachable from ``annotation`` + (through generics like ``list[...]``/``... | None``, and recursively + through each found model's own fields), each at most once.""" + origin = get_origin(annotation) + if origin is not None: + for arg in get_args(annotation): + yield from _iter_nested_models(arg, seen) + return + if isinstance(annotation, type) and issubclass(annotation, BaseModel): + model_cls: type[BaseModel] = annotation + if model_cls not in seen: + seen.add(model_cls) + yield model_cls + for field in model_cls.model_fields.values(): + yield from _iter_nested_models(field.annotation, seen) + + +def _resolve_field_order( + model_cls: type[BaseModel], schema_node: dict[str, Any] +) -> list[str]: + """The order ``schema_node["properties"]`` should render in: an explicit + ``field_order: ClassVar[list[str]]`` on ``model_cls`` when declared + (validated as an exact permutation of its own properties), else the + model's field declaration order (by alias).""" + declared_order = getattr(model_cls, "field_order", None) + if declared_order is None: + return [field.alias or name for name, field in model_cls.model_fields.items()] + declared = set(declared_order) + if declared != (actual := set(schema_node.get("properties", {}))): Review Comment: Converting the declared order to a set does not enforce an exact permutation: `['a', 'a', 'b']` passes for properties `a` and `b`. The duplicate is then silently collapsed by `_reorder`, masking an invalid declaration instead of failing registration. Compare lengths as well as set membership. ########## superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx: ########## @@ -93,24 +164,1114 @@ function ColorControl({ path, schema, label, + description, }: ControlProps) { const theme = useTheme(); const value = (data ?? (schema as Record<string, unknown>).default ?? theme.colorText) as string; return ( - <Flex align="center" gap="small"> + <Form.Item label={label} tooltip={description}> <input type="color" value={value} onChange={event => handleChange(path, event.target.value)} /> - <Typography.Text>{label}</Typography.Text> - </Flex> + </Form.Item> + ); +} + +interface SeriesEntryPropertySchema { + type?: string; + title?: string; + default?: unknown; + minimum?: number; + maximum?: number; + 'x-control'?: string; + 'x-step'?: number; +} + +interface SeriesEntrySchema { + properties?: Record<string, SeriesEntryPropertySchema>; +} + +interface SeriesMapSchema { + properties?: Record<string, SeriesEntrySchema>; +} + +type SeriesOverrideValue = Record<string, unknown>; + +/** Turns a camelCase field key into the lowercase, space-separated phrase + * an aria-label reads naturally with (`sizeScale` → `size scale`). */ +function humanizeFieldKey(key: string): string { + return key + .replace(/([A-Z])/g, ' $1') + .toLowerCase() + .trim(); +} + +/** The value a not-yet-customized series entry starts from, read purely off + * its own enriched sub-schema — every widget's per-series entry shape + * (Balloons' `{color, sizeScale}`, echarts' `{color, visible, displayName}`, + * or any future one) declares its own field defaults; this has no + * hard-coded opinion of what those fields are. `fallbackColor` only backs a + * `color` field the backend never defaulted — not expected to happen, but a + * theme token beats a literal black. */ +export function seriesDefaults( + entrySchema: SeriesEntrySchema | undefined, + fallbackColor: string, +): SeriesOverrideValue { + const properties = entrySchema?.properties ?? {}; + const value: SeriesOverrideValue = Object.fromEntries( + Object.entries(properties).map(([key, prop]) => [key, prop.default]), + ); + if (value.color === undefined && 'color' in properties) { + value.color = fallbackColor; + } + return value; +} + +/** One property of a customized series entry, rendered by its schema shape + * — not by a hard-coded field name — so a new per-series entry model (e.g. + * echarts' `{color, visible, displayName}`) needs no renderer of its own: + * - `x-control: "color"` (or the property key `color`) → a color swatch. + * - `type: "boolean"` → a toggle. + * - `type: "number"` / `"integer"` → a bounded numeric input. + * - anything else → a text input. + */ +function SeriesEntryPropertyControl({ + seriesKey, + propKey, + propSchema, + value, + onChange, +}: { + seriesKey: string; + propKey: string; + propSchema: SeriesEntryPropertySchema | undefined; + value: unknown; + onChange: (next: unknown) => void; +}): ReactElement { + const theme = useTheme(); + const fieldLabel = `${seriesKey} ${humanizeFieldKey(propKey)}`; + + if (propSchema?.['x-control'] === 'color' || propKey === 'color') { + const color = (value as string) || theme.colorText; + return ( + <ColorPicker + value={color} + onChange={next => onChange(next.toHexString())} + > + <button + type="button" + aria-label={t('%s color', seriesKey)} + style={{ + width: 20, + height: 20, + borderRadius: 4, + border: '1px solid rgba(0, 0, 0, 0.15)', + background: color, + cursor: 'pointer', + padding: 0, + }} + /> + </ColorPicker> + ); + } + if (propSchema?.type === 'boolean') { + return ( + <Switch + aria-label={fieldLabel} + checked={value !== false} + onChange={onChange} + /> + ); + } + if (propSchema?.type === 'number' || propSchema?.type === 'integer') { + return ( + <InputNumber + size="small" + aria-label={fieldLabel} + value={value as number} + min={propSchema?.minimum} + max={propSchema?.maximum} + step={propSchema?.['x-step'] ?? 1} + style={{ width: 64 }} + onChange={next => onChange(typeof next === 'number' ? next : value)} + /> + ); + } + return ( + <Input + size="small" + aria-label={fieldLabel} + value={(value as string) ?? ''} + onChange={event => onChange(event.target.value)} + style={{ width: 140 }} + /> + ); +} + +/** + * `x-dynamic: true` on a dict-of-objects field (e.g. Balloons' + * `customize.series`, one entry per distinct color-dimension value, or + * echarts' `customize.series`, one entry per `dataBinding` metric): an + * overrides list, collapsed by default, rather than the upstream renderer's + * one always-expanded group per possible entry — which turns a real + * grouping column into thousands of pixels of identical, unstyled controls. + * + * The backend enriches this field's schema with one inlined per-key + * sub-schema (a title and, where the entry shape has one, a palette-defaulted + * `color`), but leaves the *data* untouched until an author actually edits a + * value — so "has an entry in `data`" already means "has been customized", + * with no comparison against the schema's own defaults needed. Which fields + * an entry has, and how each renders, comes entirely from that per-key + * sub-schema (see `SeriesEntryPropertyControl`) — this component has no + * opinion of its own on the entry shape. + */ +function SeriesOverridesControl(props: ControlProps): ReactElement { + const { data, handleChange, path, schema, label, description } = props; + const theme = useTheme(); + const seriesSchema = schema as SeriesMapSchema; + const keys = useMemo( + () => Object.keys(seriesSchema.properties ?? {}), + [seriesSchema], + ); + const values = (data ?? {}) as Record<string, SeriesOverrideValue>; + const customizedKeys = keys.filter(key => values[key] !== undefined); + const availableKeys = keys.filter(key => values[key] === undefined); + + if (keys.length === 0) { + return ( + <Form.Item label={label} tooltip={description}> + <Typography.Text type="secondary"> + {t('No series available to customize yet.')} + </Typography.Text> + </Form.Item> + ); + } + + const write = (next: Record<string, SeriesOverrideValue>) => + handleChange(path, next); + + const removeOverride = (key: string) => { + const next = { ...values }; + delete next[key]; + write(next); + }; + + const addOverride = (key: string) => { + write({ + ...values, + [key]: seriesDefaults(seriesSchema.properties?.[key], theme.colorText), + }); + }; + + return ( + <Form.Item label={label} tooltip={description}> + <Collapse + ghost + size="small" + items={[ + { + key: 'series-overrides', + label: t( + '%s series · %s customized', + keys.length, + customizedKeys.length, + ), + children: ( + <Flex vertical gap="small"> + {customizedKeys.map(key => { + const value = values[key]; + const entryProperties = + seriesSchema.properties?.[key]?.properties ?? {}; + const otherPropKeys = Object.keys(entryProperties).filter( + propKey => propKey !== 'color', + ); + return ( + <Flex key={key} align="center" gap="small"> + {'color' in entryProperties && ( + <SeriesEntryPropertyControl + seriesKey={key} + propKey="color" + propSchema={entryProperties.color} + value={value.color} + onChange={next => + write({ + ...values, + [key]: { ...value, color: next }, + }) + } + /> + )} + <div + style={{ + flex: 1, + minWidth: 0, + whiteSpace: 'nowrap', + overflow: 'hidden', + textOverflow: 'ellipsis', + }} + > + {key} + </div> + {otherPropKeys.map(propKey => ( + <SeriesEntryPropertyControl + key={propKey} + seriesKey={key} + propKey={propKey} + propSchema={entryProperties[propKey]} + value={value[propKey]} + onChange={next => + write({ + ...values, + [key]: { ...value, [propKey]: next }, + }) + } + /> + ))} + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Reset %s to default', key)} + icon={<Icons.CloseOutlined iconSize="s" />} + onClick={() => removeOverride(key)} + /> + </Flex> + ); + })} + {availableKeys.length > 0 && ( + // Remounted on every pick: rc-select otherwise keeps + // showing the just-picked option's label internally even + // once it's gone from `options` (a controlled `value` of + // `null`/`undefined` doesn't clear that cache on its own — + // see `ReferenceMultiList`'s identical, pre-existing gap). + <Select + key={availableKeys.length} + value={null} + placeholder={t('Add a series override…')} + ariaLabel={t('Add %s override', label)} + options={availableKeys.map(key => ({ + value: key, + label: key, + }))} + onChange={next => addOverride(next as string)} + css={{ width: '100%' }} + /> + )} + </Flex> + ), + }, + ]} + /> + </Form.Item> + ); +} + +/** + * The dataset id a column/metric-reference control resolves options + * against. A JsonForms control only sees its own field's data by default — + * the whole node's props reach it through `config.formData`, which + * `SchemaControlPanel` populates for exactly this reason (mirroring + * `SemanticLayerModal`'s own `config={{ formData }}`). + */ +function useBoundDatasetId(props: ControlProps): number | undefined { + const formData = ( + props.config as { formData?: Record<string, unknown> } | undefined + )?.formData; + const dataBinding = formData?.dataBinding as + { datasetId?: number } | undefined; + return dataBinding?.datasetId; +} + +/** The sibling `dataBinding.dimensions` list, read the same way. */ +function useBoundDimensions(props: ControlProps): string[] { + const formData = ( + props.config as { formData?: Record<string, unknown> } | undefined + )?.formData; + const dataBinding = formData?.dataBinding as + { dimensions?: string[] } | undefined; + return dataBinding?.dimensions ?? []; +} + +/** + * 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; +} + +const COLUMN_TYPE_BY_HINT: Record<string, number> = { + numeric: 0, + string: 1, + temporal: 2, + boolean: 3, +}; + +/** + * Column options for a `column`/`column-multi` control, filtered by the + * field's `x-column-types` hint (omitted means any column type). + */ +export function columnOptions( + metadata: DatasetMetadata | null, + allowedTypes: string[] | undefined, +): ReferenceOption[] { + const allowed = allowedTypes?.map(hint => COLUMN_TYPE_BY_HINT[hint]); + return (metadata?.columns ?? []) + .filter( + column => + !allowed || (column.type !== null && allowed.includes(column.type)), + ) + .map(column => ({ + value: column.name, + label: ( + <Flex align="center" gap="small"> + <ColumnTypeLabel type={column.type ?? undefined} /> + {column.name} + </Flex> + ), + })); +} + +/** A single reference value (column or metric), rendered as a Select. */ +function ReferenceSelect({ + label, + description, + value, + options, + loading, + disabled, + placeholder, + onChange, +}: { + label: string; + description: string | undefined; + value: string | undefined; + options: ReferenceOption[]; + loading: boolean; + disabled: boolean; + placeholder?: string; + onChange: (next: string | undefined) => void; +}): ReactElement { + return ( + <Form.Item label={label} tooltip={description}> + <Select + ariaLabel={label} + value={value} + onChange={next => onChange((next as string | undefined) ?? undefined)} + options={options} + loading={loading} + disabled={disabled} + placeholder={placeholder} + allowClear + css={{ width: '100%' }} + /> + </Form.Item> + ); +} + +/** + * An ordered list of reference values (columns or metrics): each entry can + * be removed or dragged to reorder, and a trailing Select adds one more from + * whatever isn't already picked. + */ +function ReferenceMultiList({ + label, + description, + values, + options, + loading, + disabled, + onChange, +}: { + label: string; + description: string | undefined; + values: string[]; + options: ReferenceOption[]; + loading: boolean; + disabled: boolean; + onChange: (next: string[]) => void; +}): ReactElement { + const dragIndexRef = useRef<number | null>(null); + const available = options.filter(option => !values.includes(option.value)); + + const move = (from: number, to: number) => { + const next = [...values]; + const [moved] = next.splice(from, 1); + next.splice(to, 0, moved); + onChange(next); + }; + + return ( + <Form.Item label={label} tooltip={description}> + <Flex vertical gap="small"> + {values.map((value, index) => { + const option = options.find(candidate => candidate.value === value); + return ( + <Flex + key={value} + align="center" + gap="small" + draggable + onDragStart={() => { + dragIndexRef.current = index; + }} + onDragOver={event => event.preventDefault()} + onDrop={() => { + if ( + dragIndexRef.current !== null && + dragIndexRef.current !== index + ) { + move(dragIndexRef.current, index); + } + dragIndexRef.current = null; + }} + > + <Icons.HolderOutlined iconSize="s" /> + <div style={{ flex: 1 }}>{option?.label ?? value}</div> + {/* Keyboard-operable equivalent of the drag handle above: that + handle isn't itself focusable, so reordering — which + decides e.g. the default color dimension — had no + non-mouse path at all. */} + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Move %s up', value)} + disabled={index === 0} + icon={<Icons.UpOutlined iconSize="s" />} + onClick={() => move(index, index - 1)} + /> + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Move %s down', value)} + disabled={index === values.length - 1} + icon={<Icons.DownOutlined iconSize="s" />} + onClick={() => move(index, index + 1)} + /> + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Remove %s', value)} + icon={<Icons.CloseOutlined iconSize="s" />} + onClick={() => onChange(values.filter((_, i) => i !== index))} + /> + </Flex> + ); + })} + {available.length > 0 && ( + // Remounted on every pick — see `SeriesOverridesControl`'s + // identical picker for why a controlled `value` of `undefined` + // alone doesn't stop rc-select echoing the just-picked label. + <Select + key={available.length} + value={null} + placeholder={t('Add field')} + ariaLabel={t('Add %s', label)} + options={available} + loading={loading} + disabled={disabled} + onChange={next => onChange([...values, next as string])} + css={{ width: '100%' }} + /> + )} + </Flex> + </Form.Item> + ); +} + +/** + * `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, 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} + description={props.description} + value={props.data as string | undefined} + options={columnOptions(metadata, allowedTypes)} + loading={loading} + disabled={!props.enabled} + onChange={value => props.handleChange(props.path, value)} + /> + ); +} + +/** + * The `colorDimension` field specifically: a column reference, but not to + * any column — the widget only colors by a dimension it already groups by + * (Balloons' `_color_dimension_must_be_grouped` validator rejects anything + * else). Offering all of a dataset's columns, most of which the backend + * will reject, taught nothing about which one was actually valid; this + * intersects the picker's options with the sibling `dataBinding.dimensions` + * instead, and disables it with an explanatory placeholder when there's + * nothing grouped yet to color by. + */ +function ColorDimensionControl(props: ControlProps): ReactElement { + const datasetId = useBoundDatasetId(props); + const dimensions = useBoundDimensions(props); + const { metadata, loading, error } = useDatasetMetadata(datasetId); + + if ( + shouldFallBackToCode(datasetId, error) || + (props.data !== undefined && typeof props.data !== 'string') + ) { + return <CodeControl {...props} />; + } + + const options = columnOptions(metadata, undefined).filter(option => + dimensions.includes(option.value), + ); + + return ( + <ReferenceSelect + label={props.label} + description={props.description} + value={props.data as string | undefined} + options={options} + loading={loading} + disabled={!props.enabled || dimensions.length === 0} + placeholder={ + dimensions.length === 0 ? t('Group a dimension first') : undefined + } + onChange={value => props.handleChange(props.path, value)} + /> ); } -/** Base Semantic-Layer renderers plus the widget-control code/color ones. */ +/** + * `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, 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 unknown[]) : []; + const hasNonStringEntry = values.some(value => typeof value !== 'string'); + + if (shouldFallBackToCode(datasetId, error) || hasNonStringEntry) { + return <CodeControl {...props} />; + } + + return ( + <ReferenceMultiList + label={props.label} + description={props.description} + values={values as string[]} + options={columnOptions(metadata, allowedTypes)} + loading={loading} + disabled={!props.enabled} + onChange={next => props.handleChange(props.path, next)} + /> + ); +} + +/** + * 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, +): ReferenceOption[] { + return (metadata?.metrics ?? []).map(metric => ({ + value: metric.name, + label: ( + <Flex align="center" gap="small"> + <ColumnTypeLabel type="metric" /> + {metric.verboseName} + </Flex> + ), + })); +} + +/** + * True for a metric entry this control has no row to draw at all: neither a + * plain saved-metric-name string nor a structurally valid ad-hoc metric + * object — e.g. malformed data hand-authored through the JSON tab. Only + * this case still drops the *whole* field to the raw JSON editor; a + * well-formed mix of saved and ad-hoc entries renders as a mixed list + * instead (see `MetricEntryList`). + */ +export function isUnrepresentableMetric(value: unknown): boolean { + return typeof value !== 'string' && !isDictionaryForAdhocMetric(value); +} + +/** Whether the bound dataset's own settings forbid ad-hoc metrics entirely + * (a dataset-level admin setting, not a per-field one) — read the same raw + * `extra` JSON the legacy metric editor reads it from. */ +export function disallowsAdhocMetrics( + metadata: DatasetMetadata | null, +): boolean { + if (!metadata?.extra) return false; + try { + return Boolean( + (JSON.parse(metadata.extra) as { disallow_adhoc_metrics?: boolean }) + .disallow_adhoc_metrics, + ); + } catch { + return false; + } +} + +type MetricEntry = string | CoreAdhocMetric; + +/** A metric entry's own display label: a saved metric's verbose name (or + * its raw name if the dataset's metric list hasn't loaded/matched yet), or + * an ad-hoc metric's own label — computed the same way the legacy editor + * derives one (`(AVG)(price)`, etc.) when the author hasn't set a custom one. */ +function metricEntryLabel( + entry: MetricEntry, + metadata: DatasetMetadata | null, +): ReactNode { + if (typeof entry === 'string') { + const known = metadata?.metrics.find(metric => metric.name === entry); + return known?.verboseName ?? entry; + } + return fromCoreAdhocMetric(entry).label; +} + +/** Sentinel option value picked from the "Add field" select to start a new + * ad-hoc metric, distinct from any real saved-metric name. */ +const ADD_CUSTOM_METRIC = '__custom_metric__'; + +/** + * An ordered list of metric references, each entry rendered by its own + * kind: a saved metric as a plain row (unchanged from `ReferenceMultiList`), + * an ad-hoc metric (SIMPLE or SQL) as a row whose label opens + * `AdhocMetricEditor` for just that entry. "Add field" offers both a saved + * metric to pick and, unless the dataset disallows it, a blank ad-hoc draft. + */ +function MetricEntryList({ + label, + description, + values, + metadata, + columns, + datasourceId, + datasourceType, + disallowAdhoc, + disabled, + onChange, +}: { + label: string; + description: string | undefined; + values: MetricEntry[]; + metadata: DatasetMetadata | null; + columns: DatasetColumnMeta[]; + datasourceId: number | undefined; + datasourceType: string | undefined; + disallowAdhoc: boolean; + disabled: boolean; + onChange: (next: MetricEntry[]) => void; +}): ReactElement { + const [editingIndex, setEditingIndex] = useState<number | null>(null); + const [addingNew, setAddingNew] = useState(false); + + const pickedSavedNames = values.filter( + (value): value is string => typeof value === 'string', + ); + const availableSaved = metricOptions(metadata).filter( + option => !pickedSavedNames.includes(option.value), + ); + const addOptions = [ + ...availableSaved, + ...(disallowAdhoc + ? [] + : [{ value: ADD_CUSTOM_METRIC, label: t('Custom metric…') }]), + ]; + + const move = (from: number, to: number) => { + const next = [...values]; + const [moved] = next.splice(from, 1); + next.splice(to, 0, moved); + onChange(next); + }; + + return ( + <Form.Item label={label} tooltip={description}> + <Flex vertical gap="small"> + {values.map((value, index) => { + const isAdhoc = typeof value !== 'string'; + const canEdit = isAdhoc && !disallowAdhoc; + const key = + typeof value === 'string' + ? value + : (value.optionName ?? `adhoc-${index}`); + // Event handlers only attached at all when `canEdit` — a static + // div with an onClick/onKeyDown regardless of role is what the + // a11y linter (rightly) objects to, not just a style choice. + const interactiveProps = canEdit + ? { + role: 'button' as const, + tabIndex: 0, + onClick: () => setEditingIndex(index), + onKeyDown: (event: KeyboardEvent<HTMLDivElement>) => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + setEditingIndex(index); + } + }, + } + : {}; + const rowLabel = ( + <div + style={{ + flex: 1, + minWidth: 0, + cursor: canEdit ? 'pointer' : 'default', + }} + {...interactiveProps} + > + <Flex align="center" gap="small"> + <ColumnTypeLabel type="metric" /> + {metricEntryLabel(value, metadata)} + </Flex> + </div> + ); + return ( + <Flex key={key} align="center" gap="small"> + <Icons.HolderOutlined iconSize="s" /> + {isAdhoc ? ( + <AdhocMetricEditor + value={value} + columns={columns} + datasourceId={datasourceId} + datasourceType={datasourceType} + open={editingIndex === index} + onOpenChange={open => { + // `Popover`'s own `trigger="click"` opens on any click to + // its children regardless of what the row's own onClick + // does — `canEdit` has to gate here too, or a disallowed + // dataset's rows would still open on click. + if (canEdit) setEditingIndex(open ? index : null); + }} + onSave={next => { + const updated = [...values]; + updated[index] = next; + onChange(updated); + }} + > + {rowLabel} + </AdhocMetricEditor> + ) : ( + rowLabel + )} + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Move metric %s up', index + 1)} + disabled={index === 0} + icon={<Icons.UpOutlined iconSize="s" />} + onClick={() => move(index, index - 1)} + /> + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Move metric %s down', index + 1)} + disabled={index === values.length - 1} + icon={<Icons.DownOutlined iconSize="s" />} + onClick={() => move(index, index + 1)} + /> + <Button + buttonSize="xsmall" + buttonStyle="link" + aria-label={t('Remove metric %s', index + 1)} + icon={<Icons.CloseOutlined iconSize="s" />} + onClick={() => onChange(values.filter((_, i) => i !== index))} + /> + </Flex> + ); + })} + {addOptions.length > 0 && ( + <Select + key={`${availableSaved.length}-${values.length}`} + value={null} + placeholder={t('Add field')} + ariaLabel={t('Add %s', label)} + options={addOptions} + disabled={disabled} + onChange={next => { + if (next === ADD_CUSTOM_METRIC) { + setAddingNew(true); + } else { + onChange([...values, next as string]); + } + }} + /> + )} + {addingNew && ( + <AdhocMetricEditor + value={undefined} + columns={columns} + datasourceId={datasourceId} + datasourceType={datasourceType} + open={addingNew} + onOpenChange={setAddingNew} + onSave={next => { + onChange([...values, next]); + setAddingNew(false); + }} + > + {/* Zero-size trigger: opening this popover is driven entirely + by picking "Custom metric…" above, not by a click here. */} + <span /> + </AdhocMetricEditor> + )} + </Flex> + </Form.Item> + ); +} + +/** + * `x-control: "metric-multi"` — an ordered list of metric references. Falls + * back to the raw JSON editor (`CodeControl`) whenever no dataset is bound + * (or its fetch failed), or an entry is genuinely unrepresentable (see + * `isUnrepresentableMetric`); a well-formed mix of saved and ad-hoc metrics + * renders as `MetricEntryList`, not raw JSON. + */ +function MetricMultiControl(props: ControlProps): ReactElement { + const datasetId = useBoundDatasetId(props); + const { metadata, loading, error } = useDatasetMetadata(datasetId); + const values = Array.isArray(props.data) ? (props.data as unknown[]) : []; + const hasUnrepresentable = values.some(isUnrepresentableMetric); + + if (shouldFallBackToCode(datasetId, error) || hasUnrepresentable) { + return <CodeControl {...props} />; + } + + return ( + <MetricEntryList + label={props.label} + description={props.description} + values={values as MetricEntry[]} + metadata={metadata} + columns={metadata?.columns ?? []} + datasourceId={datasetId} + datasourceType={metadata?.datasourceType} + disallowAdhoc={disallowsAdhocMetrics(metadata)} + disabled={!props.enabled || loading} + onChange={next => props.handleChange(props.path, next)} + /> + ); +} + +/** + * The value the dataset picker needs for an already-bound dataset. Bare — + * just the id (composite-encoded per below) — while the name is still + * resolving: `AsyncSelect` prefers a *labeled* value's own label over a + * matching loaded option's, so handing it a stale `String(datasetId)` label + * here would overwrite the real name the moment the matching option + * arrives, replaying the exact stuck-on-the-numeric-id symptom this control + * exists to avoid — instead, letting `AsyncSelect` fall back to the id on + * its own leaves it free to prefer a loaded option's label the instant one + * matches. Once `tableName` resolves, the explicit label takes over. + * + * `value` carries the composite `"ds:<id>"` encoding when the Semantic + * Layers flag is on, matching what `loadDatasetOptions` encodes its own + * options as in that mode (see `resolveDatasetPick` below) — an option and + * a bound value in two different encodings never match, which is what + * leaves a genuinely-selected dataset showing as unselected. + */ +export function toDatasetSelectValue( + datasetId: number | undefined, + tableName: string | undefined, + useSemanticLayers: boolean, +): { label: string; value: number | string } | number | string | undefined { + if (datasetId === undefined) { + return undefined; + } + const value = useSemanticLayers ? toCompositeValue(datasetId) : datasetId; + return tableName ? { label: tableName, value } : value; +} + +/** + * The numeric dataset id a picked option resolves to, or `undefined` when + * the pick should be rejected outright: a semantic view, which + * `DataBinding.datasetId` has no way to represent (SIP-182's `kind` is a + * connection-level concept, not a per-field one on this schema). Handles + * both encodings `loadDatasetOptions` can produce — a plain number when the + * Semantic Layers flag is off, or, flag on, a composite `"ds:<id>"` / + * `"sv:<id>"` string for every option (not only semantic views: with the + * flag on, ordinary datasets are composite-encoded too). + */ +export function resolveDatasetPick( + value: number | string | undefined, +): number | undefined { + if (value === undefined || typeof value === 'number') { + return value; + } + return kindFromComposite(value) === 'semantic_view' + ? undefined + : fromCompositeValue(value); +} + +/** + * `loadDatasetOptions` filtered down to plain datasets. `DataBinding` has no + * way to represent a semantic view (SIP-182's `kind` is a connection-level + * concept, not a per-field one on this schema), so rather than let one be + * picked and then reject it after the fact — leaving the closed select + * showing a value the widget never actually bound — it is never offered. + * + * Module-level, not a closure defined inside `DatasetControl`: `AsyncSelect` + * treats a change in its `options` function's identity as a reason to wipe + * its own fetched-options cache, and a fresh arrow function on every render + * would do exactly that. + * + * `totalCount` is passed through unfiltered — it counts datasets and + * semantic views together, same as the page `data` was drawn from before + * this function's own filter ran. With the Semantic Layers flag on and + * enough semantic views sorted ahead of the wanted datasets on the current + * search, a page that filters down to nothing still reports more rows + * exist, but `AsyncSelect` only requests the next page on scroll — and a + * dropdown with nothing to scroll never gets the chance. Narrow (flag off, + * the filter is a no-op and this never applies) and not addressed here; + * paging forward internally past an empty filtered page, or asking the + * backend to exclude semantic views from the query in the first place, + * would close it. + */ +async function loadDatasetOnlyOptions( + search: string, + page: number, + pageSize: number, +) { + const { data, totalCount } = await loadDatasetOptions(search, page, pageSize); + return { + data: data.filter(option => option.kind !== 'semantic_view'), + totalCount, Review Comment: Filtering semantic views after pagination can return an empty page while later pages still contain accessible datasets; because the dropdown has nothing to scroll, it never requests those pages. The comment above already describes this failure mode, but the picker is shipped with it unresolved. Filter by kind server-side or advance through empty filtered pages before returning. ########## superset-frontend/src/pages/DashboardBuilderV2/controlValueValidation.ts: ########## @@ -0,0 +1,111 @@ +/** + * 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. + */ + +/** + * The one path either half of the Properties panel (the schema-driven form + * and the JSON editor) writes a schema-controlled widget's control values + * through. + * + * Both representations edit the same `node.props`, but neither may write to + * it directly: a candidate is merged, sent to the backend's + * `Widget.validate_control_values` gate — the same gate the + * `set_widget_control_values` MCP tool commits through, reached here via its + * REST wrapper rather than a second, frontend-authored copy of its rules — + * and only committed to the store once that gate accepts it. A rejected + * candidate returns its errors and touches nothing: `node.props` is read + * here, never written to, until validation has already succeeded. + */ +import { SupersetClient } from '@superset-ui/core'; +import { provider } from 'src/core/dashboard/store'; + +export type ControlValidationError = { + loc: (string | number)[]; + message: string; +}; + +export type CommitPropsResult = + | { ok: true; values: Record<string, unknown> } + | { ok: false; errors: ControlValidationError[] }; + +/** + * SupersetClient rejects a non-2xx response with the raw, unparsed `Response` + * object rather than an `Error`, so a plain `String(e)` yields the useless + * "[object Response]". Pull the actual `{message}`/`{errors:[...]}` body + * Superset sends back (same shape `chartData.ts` handles). + */ +export async function describeError(e: unknown): Promise<string> { + if (typeof Response !== 'undefined' && e instanceof Response) { + try { + const body = await e.clone().json(); + const detail = + body?.message ?? + (Array.isArray(body?.errors) + ? body.errors + .map((err: { message?: string }) => err.message) + .join('; ') + : undefined); + return detail + ? `${e.status} ${e.statusText}: ${detail}` + : `${e.status} ${e.statusText}`; + } catch { + return `${e.status} ${e.statusText}`; + } + } + return e instanceof Error ? e.message : String(e); +} + +async function validateControlValues( + widgetType: string, + controlValues: Record<string, unknown>, +): Promise<ControlValidationError[]> { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/widgets/type/${widgetType}/validate`, + jsonPayload: { control_values: controlValues }, + }); + return (json as { result: { errors: ControlValidationError[] } }).result + .errors; +} + +/** + * Merges `delta` onto the node's current props, validates the merged + * candidate, and commits it to `node.props` only if the backend accepts it. + * + * `onBeforeCommit`, if given, runs synchronously immediately before the + * `provider.updateProps` call — not part of the merge/validate/commit + * contract itself, but the one hook a caller needs to set a flag in the + * exact tick its own commit lands (e.g. `SchemaControlPanel` telling its + * own resync effect "this `props` change was mine"), without racing the + * async validation round-trip to do it. + */ +export async function commitWidgetProps( + nodeId: string, + widgetType: string, + delta: Record<string, unknown>, + options?: { onBeforeCommit?: () => void }, +): Promise<CommitPropsResult> { + const node = provider.getNode(nodeId); + const candidate = { ...node?.props, ...delta }; + const errors = await validateControlValues(widgetType, candidate); + if (errors.length > 0) { + return { ok: false, errors }; + } + options?.onBeforeCommit?.(); + provider.updateProps(nodeId, candidate); + return { ok: true, values: candidate }; Review Comment: The backend validates a Pydantic-normalized value, but this commits the raw candidate. For example, Pydantic accepts `{"datasetId":"1"}` by coercing it to an integer, while the frontend stores the string; `DatasetControl` then treats that accepted value as unrepresentable. Return normalized values from the validation endpoint and commit those, as the MCP write path already does. ########## superset/mcp_service/widgets/tool/set_widget_control_values.py: ########## @@ -0,0 +1,107 @@ +# 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. + +"""MCP tool: set_widget_control_values""" + +from __future__ import annotations + +import logging +from typing import Any, Dict + +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.mcp_service.widgets.node_store import nodes +from superset.mcp_service.widgets.utils import ( + resolve_widget, + unknown_node_error, + unknown_widget_type_error, +) + +logger = logging.getLogger(__name__) + + +def _set_widget_control_values_impl( + node_id: str, + control_values: Dict[str, Any], +) -> Dict[str, Any]: + """Pure logic: validate-then-commit a widget node's control values. + + Builds a candidate (the node's current ``props`` shallow-merged with + ``control_values`` -- new keys override, everything else is preserved, + mirroring how the frontend's ``DashboardProvider.updateProps`` merges) + without touching the stored node. Validates the candidate through + ``Widget.validate_control_values`` -- the same commit-time gate the + ``/type/<widget_type>/validate`` REST endpoint uses. Only on success is + the node's ``props`` replaced with the candidate, a single dict + reassignment, so a validation failure leaves the stored node completely + unchanged: there is nothing to roll back because nothing was mutated in + place. + """ + node = nodes.get(node_id) + if node is None: + return unknown_node_error(node_id) Review Comment: This tool has no usable production target: the process-local `nodes` map is populated only by tests, and there is no create/seed tool or bridge from the frontend dashboard document. Consequently, every normal MCP invocation returns `unknown_node`, so the PR’s advertised widget-value write tool cannot apply any value. Wire it to an addressable document or provide a production population path. ########## superset-frontend/src/core/dashboard/widgets/echartsStructuredSeries.ts: ########## @@ -0,0 +1,97 @@ +/** + * 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. + */ + +/** + * The `echarts` widget's optional structured layer (`chartType`/`customize` + * on `EchartsControls`, see `superset/widgets/controls.py`). Precedence, + * matching the backend model's docstring: + * 1. `echartsOptions` (already `$bind`-resolved) is the base. + * 2. When `chartType` is set, `option.series` — and only `series` — is + * replaced with one generated series per `dataBinding` metric. + * Everything else the raw option authored (axes, legend, tooltip, + * title) survives unmanaged. + * 3. `chartType` unset/null ("Custom") leaves the raw option untouched, + * including mixed-series or non-Cartesian (e.g. pie) shapes. + * + * A series is keyed by its metric's `getMetricLabel` — the same label the + * `/api/v1/chart/data` result columns are named after, so `data` reads the + * right column, and the same label the backend's `_metric_key` computes, so + * a stored override matches by stable identity rather than array position. + */ +import { getMetricLabel } from '@superset-ui/core'; +import type { QueryFormMetric } from '@superset-ui/core'; +import type { dashboard as dashboardApi } from '@apache-superset/core'; + +type DataRow = dashboardApi.DataRow; + +export type EchartsChartType = 'bar' | 'line' | 'scatter'; + +export interface SeriesOverrideValue { + color?: string; + visible?: boolean; + displayName?: string; +} + +/** + * One ECharts series per metric, in `dataBinding.metrics` order. A metric + * overridden with `visible: false` is omitted entirely (there is no ECharts + * option for "present but hidden" that also frees its legend/tooltip slot). + */ +export function buildStructuredSeries( + chartType: EchartsChartType, + metrics: QueryFormMetric[], + rows: DataRow[], + overrides: Record<string, SeriesOverrideValue> | undefined, +): Record<string, unknown>[] { + return metrics + .map(metric => { + const key = getMetricLabel(metric); + const override = overrides?.[key]; + if (override?.visible === false) return null; + const series: Record<string, unknown> = { + name: override?.displayName || key, + type: chartType, + data: rows.map(row => row[key]), + }; + if (override?.color) { + series.itemStyle = { color: override.color }; + } Review Comment: The enriched schema assigns a palette default for each series, but the renderer applies a color only when an override is stored. Thus the control can display `Echarts.PALETTE[0]` as the default while the untouched chart uses ECharts’ own palette; merely adding an override without changing the swatch can change the series color. Apply the same structured palette during rendering or stop advertising these values as defaults. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
