mirror of
https://github.com/apache/superset.git
synced 2026-09-06 07:21:35 +00:00
chore(dashboard-v2): remove docs/superpowers specs/plans from this branch
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Enzo Martellucci
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
baeef73a45
commit
03d0506daa
File diff suppressed because it is too large
Load Diff
@@ -1,606 +0,0 @@
|
||||
# Dashboard V2 Data Tab Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Add a fourth tab, "Data", to the Dashboard V2 editor's left rail — a static, mock-data browser for datasets and their columns — and rename the existing "Widgets" tab to "Building Blocks", with no backend calls or widget-binding changes.
|
||||
|
||||
**Architecture:** A new self-contained `DataPanel.tsx` component (styled and structured like its siblings `Palette.tsx`/`Outline.tsx`: a search input over a hardcoded in-memory list, expandable rows) gets built and unit-tested on its own in Task 1, then wired into `EditorPanel.tsx`'s existing `Tabs` in Task 2, which also fixes the panel's selection-follow logic so it generalizes correctly to the new tab.
|
||||
|
||||
**Tech Stack:** React + TypeScript, `@apache-superset/core/theme` (styled/css), antd components via `@superset-ui/core/components`, `ColumnTypeLabel` from `@superset-ui/chart-controls`, Jest + React Testing Library.
|
||||
|
||||
**Spec:** [docs/superpowers/specs/2026-08-25-dashboard-v2-mockup-alignment-design.md](../specs/2026-08-25-dashboard-v2-mockup-alignment-design.md) (PR 1 section)
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- No network/API calls in this PR — `DataPanel` renders a hardcoded, in-memory dataset list only.
|
||||
- No changes to any widget's `dataBinding` or to `DashboardProvider` — the Data tab does not select, bind, or drag anything onto the canvas.
|
||||
- Do not touch the Assistant/chat panel (`src/core/chat/ChatHost.tsx`) or anything it depends on.
|
||||
- Column type icons must use `ColumnTypeLabel` from `@superset-ui/chart-controls` (the same component `schemaControlRenderers.tsx` already uses), not a new icon scheme.
|
||||
- Tests use flat `test()` calls, not `describe()` blocks (matches every existing test file in this directory).
|
||||
- All user-facing strings go through `t()` from `@apache-superset/core/translation`.
|
||||
|
||||
---
|
||||
|
||||
## File Structure
|
||||
|
||||
- **Create** `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.tsx` — the Data tab's content: search input + hardcoded dataset list with expandable columns. No props; no dependency on `provider`/`DashboardProvider` at all.
|
||||
- **Create** `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.test.tsx` — unit tests for the component in isolation.
|
||||
- **Modify** `superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx` — add the `data` tab (first in order), rename the `widgets` tab's label, generalize the selection-follow condition.
|
||||
- **Modify** `superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx` — update the three assertions that hardcode the old "Widgets" label, add tab-order and selection-follow-from-Data coverage.
|
||||
|
||||
---
|
||||
|
||||
## Task 1: `DataPanel` component
|
||||
|
||||
**Files:**
|
||||
- Create: `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.tsx`
|
||||
- Test: `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.test.tsx`
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `export default function DataPanel(): ReactElement` — zero props, self-contained. Task 2 renders it as `<DataPanel />` with no wiring.
|
||||
|
||||
- [ ] **Step 1: Write the failing tests**
|
||||
|
||||
Create `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.test.tsx`:
|
||||
|
||||
```tsx
|
||||
/**
|
||||
* 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 userEvent from '@testing-library/user-event';
|
||||
import { render, screen } from 'spec/helpers/testing-library';
|
||||
import DataPanel from './DataPanel';
|
||||
|
||||
const mount = () => render(<DataPanel />);
|
||||
|
||||
test('lists the placeholder datasets, collapsed by default', () => {
|
||||
mount();
|
||||
|
||||
expect(screen.getByTestId('data-panel-dataset-sales')).toBeVisible();
|
||||
expect(screen.getByTestId('data-panel-dataset-coffee_sales')).toBeVisible();
|
||||
expect(
|
||||
screen.queryByTestId('data-panel-columns-sales'),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('expanding a dataset shows its columns', async () => {
|
||||
mount();
|
||||
|
||||
await userEvent.click(screen.getByTestId('data-panel-dataset-sales'));
|
||||
|
||||
const columns = screen.getByTestId('data-panel-columns-sales');
|
||||
expect(columns).toHaveTextContent('order_id');
|
||||
expect(columns).toHaveTextContent('order_date');
|
||||
expect(columns).toHaveTextContent('sales_amount');
|
||||
expect(columns).toHaveTextContent('region');
|
||||
});
|
||||
|
||||
test('a second click collapses it again', async () => {
|
||||
mount();
|
||||
const row = screen.getByTestId('data-panel-dataset-sales');
|
||||
|
||||
await userEvent.click(row);
|
||||
await userEvent.click(row);
|
||||
|
||||
expect(
|
||||
screen.queryByTestId('data-panel-columns-sales'),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('searching narrows the list to matching dataset names', async () => {
|
||||
mount();
|
||||
|
||||
await userEvent.type(screen.getByTestId('data-panel-search'), 'coffee');
|
||||
|
||||
expect(
|
||||
screen.getByTestId('data-panel-dataset-coffee_sales'),
|
||||
).toBeVisible();
|
||||
expect(
|
||||
screen.queryByTestId('data-panel-dataset-sales'),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('a search with no matches says so', async () => {
|
||||
mount();
|
||||
|
||||
await userEvent.type(screen.getByTestId('data-panel-search'), 'nope');
|
||||
|
||||
expect(screen.getByTestId('data-panel-empty')).toBeVisible();
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the tests to verify they fail**
|
||||
|
||||
Run: `npm run test -- DataPanel.test.tsx`
|
||||
Expected: FAIL — `Cannot find module './DataPanel'` (the file doesn't exist yet).
|
||||
|
||||
- [ ] **Step 3: Implement `DataPanel.tsx`**
|
||||
|
||||
Create `superset-frontend/src/pages/DashboardBuilderV2/DataPanel.tsx`:
|
||||
|
||||
```tsx
|
||||
/**
|
||||
* 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};
|
||||
border-radius: ${theme.borderRadiusSM}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));
|
||||
|
||||
const toggle = (id: string): void =>
|
||||
setExpanded(previous => {
|
||||
const next = new Set(previous);
|
||||
if (!next.delete(id)) {
|
||||
next.add(id);
|
||||
}
|
||||
return next;
|
||||
});
|
||||
|
||||
return (
|
||||
<Column data-test="data-panel">
|
||||
<Input
|
||||
allowClear
|
||||
value={query}
|
||||
aria-label={t('Search datasets')}
|
||||
placeholder={t('Search datasets…')}
|
||||
data-test="data-panel-search"
|
||||
prefix={<Icons.SearchOutlined iconSize="s" />}
|
||||
onChange={event => setQuery(event.target.value)}
|
||||
/>
|
||||
{found.length === 0 ? (
|
||||
<div data-test="data-panel-empty">
|
||||
<EmptyState
|
||||
size="small"
|
||||
image="filter-results.svg"
|
||||
title={t('No matching datasets')}
|
||||
description={t('Nothing here is called “%s”.', query)}
|
||||
/>
|
||||
</div>
|
||||
) : (
|
||||
<List>
|
||||
{found.map(dataset => {
|
||||
const isOpen = expanded.has(dataset.id);
|
||||
return (
|
||||
<div
|
||||
key={dataset.id}
|
||||
data-test={`data-panel-dataset-${dataset.id}`}
|
||||
>
|
||||
<DatasetButton
|
||||
type="button"
|
||||
aria-expanded={isOpen}
|
||||
onClick={() => toggle(dataset.id)}
|
||||
>
|
||||
<span className="data-panel-chevron" aria-hidden>
|
||||
{isOpen ? (
|
||||
<Icons.UpOutlined iconSize="s" />
|
||||
) : (
|
||||
<Icons.DownOutlined iconSize="s" />
|
||||
)}
|
||||
</span>
|
||||
{dataset.name}
|
||||
</DatasetButton>
|
||||
{isOpen && (
|
||||
<ColumnList data-test={`data-panel-columns-${dataset.id}`}>
|
||||
{dataset.columns.map(column => (
|
||||
<ColumnRow key={column.name}>
|
||||
<ColumnTypeLabel type={column.type} />
|
||||
{column.name}
|
||||
</ColumnRow>
|
||||
))}
|
||||
</ColumnList>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
})}
|
||||
</List>
|
||||
)}
|
||||
</Column>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
Note: `data-panel-dataset-${dataset.id}` is on the wrapping `<div>`, not the
|
||||
`<button>` — the test's `toBeVisible()`/click target is the same element
|
||||
either way here since the div has no independent styling, but clicking must
|
||||
go through `userEvent.click` on that testid, which dispatches to whatever is
|
||||
at that DOM node (the div passes the click through to its child button
|
||||
because it's not `pointer-events: none`, but to keep this unambiguous the
|
||||
click handler is what matters, not which element wraps it) — see Step 4.
|
||||
|
||||
- [ ] **Step 4: Run the tests to verify they pass**
|
||||
|
||||
Run: `npm run test -- DataPanel.test.tsx`
|
||||
Expected: PASS, all 5 tests.
|
||||
|
||||
If the click tests fail because `userEvent.click` on the wrapping `<div>`
|
||||
testid doesn't reach the `<button>`'s `onClick`: change the test to click
|
||||
`screen.getByRole('button', { name: /sales/i })` scoped within
|
||||
`screen.getByTestId('data-panel-dataset-sales')` instead, e.g.:
|
||||
|
||||
```tsx
|
||||
const row = screen.getByTestId('data-panel-dataset-sales');
|
||||
await userEvent.click(within(row).getByRole('button'));
|
||||
```
|
||||
|
||||
(`within` from `spec/helpers/testing-library`, matching the pattern already
|
||||
used elsewhere in this directory's tests.)
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-frontend/src/pages/DashboardBuilderV2/DataPanel.tsx superset-frontend/src/pages/DashboardBuilderV2/DataPanel.test.tsx
|
||||
git commit -m "feat(dashboard-v2): add placeholder Data panel with mock datasets"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: Wire the Data tab into `EditorPanel`, rename Widgets → Building Blocks
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx`
|
||||
- Modify: `superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `DataPanel` (default export, zero props) from Task 1.
|
||||
- Produces: no new exports — `EditorPanel`'s own props (`{ onAdd: (type: string) => void }`) are unchanged.
|
||||
|
||||
- [ ] **Step 1: Write the failing tests**
|
||||
|
||||
In `superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx`,
|
||||
replace the existing tab-presence test (currently at the top, right after
|
||||
`mount`) with one that expects all four tabs under their new names, and add
|
||||
two new tests. The three edits:
|
||||
|
||||
Replace:
|
||||
|
||||
```tsx
|
||||
test('the panel offers widgets, properties and an outline', () => {
|
||||
mount();
|
||||
|
||||
expect(screen.getByRole('tab', { name: 'Widgets' })).toBeInTheDocument();
|
||||
expect(screen.getByRole('tab', { name: 'Properties' })).toBeInTheDocument();
|
||||
expect(screen.getByRole('tab', { name: 'Outline' })).toBeInTheDocument();
|
||||
});
|
||||
```
|
||||
|
||||
with:
|
||||
|
||||
```tsx
|
||||
test('the panel offers data, building blocks, properties and an outline', () => {
|
||||
mount();
|
||||
|
||||
expect(screen.getByRole('tab', { name: 'Data' })).toBeInTheDocument();
|
||||
expect(
|
||||
screen.getByRole('tab', { name: 'Building Blocks' }),
|
||||
).toBeInTheDocument();
|
||||
expect(screen.getByRole('tab', { name: 'Properties' })).toBeInTheDocument();
|
||||
expect(screen.getByRole('tab', { name: 'Outline' })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
test('data comes first, ahead of building blocks, properties and outline', () => {
|
||||
mount();
|
||||
|
||||
const labels = screen.getAllByRole('tab').map(tab => tab.textContent);
|
||||
expect(labels).toEqual(['Data', 'Building Blocks', 'Properties', 'Outline']);
|
||||
});
|
||||
|
||||
test('the data tab shows the placeholder dataset browser', async () => {
|
||||
mount();
|
||||
|
||||
await userEvent.click(screen.getByRole('tab', { name: 'Data' }));
|
||||
|
||||
expect(screen.getByTestId('data-panel')).toBeVisible();
|
||||
});
|
||||
|
||||
test('selecting something while browsing data brings its properties forward too', () => {
|
||||
mount();
|
||||
const id = provider.addWidget(provider.getRoot().id, 0, {
|
||||
type: 'markdown',
|
||||
});
|
||||
|
||||
act(() => provider.setSelection(id));
|
||||
|
||||
// Outline is the one tab that sets its own selection and must not be
|
||||
// ejected from; every other tab — including the new Data tab — follows a
|
||||
// selection made elsewhere the same way Widgets already does.
|
||||
expect(screen.getByTestId('inspector-identity')).toHaveTextContent(id);
|
||||
});
|
||||
```
|
||||
|
||||
Then find the two existing assertions in the panel-collapse test that name
|
||||
`'Widgets'` and change them to `'Building Blocks'`:
|
||||
|
||||
```tsx
|
||||
expect(screen.queryByRole('tab', { name: 'Widgets' })).toBeNull();
|
||||
expect(screen.getByTestId('panel-expand')).toBeInTheDocument();
|
||||
|
||||
await userEvent.click(screen.getByTestId('panel-expand'));
|
||||
|
||||
expect(screen.getByRole('tab', { name: 'Widgets' })).toBeInTheDocument();
|
||||
```
|
||||
|
||||
becomes:
|
||||
|
||||
```tsx
|
||||
expect(screen.queryByRole('tab', { name: 'Building Blocks' })).toBeNull();
|
||||
expect(screen.getByTestId('panel-expand')).toBeInTheDocument();
|
||||
|
||||
await userEvent.click(screen.getByTestId('panel-expand'));
|
||||
|
||||
expect(
|
||||
screen.getByRole('tab', { name: 'Building Blocks' }),
|
||||
).toBeInTheDocument();
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the tests to verify they fail**
|
||||
|
||||
Run: `npm run test -- EditorPanel.test.tsx`
|
||||
Expected: FAIL — the renamed/new assertions don't match anything yet (`EditorPanel.tsx` hasn't changed), and the two `'Widgets'` assertions in the collapse test now fail too since they were changed to expect `'Building Blocks'`.
|
||||
|
||||
- [ ] **Step 3: Implement the `EditorPanel.tsx` changes**
|
||||
|
||||
Add the import (alongside the existing `Inspector`/`Outline`/`Palette` imports):
|
||||
|
||||
```tsx
|
||||
import DataPanel from './DataPanel';
|
||||
```
|
||||
|
||||
Change the tab type:
|
||||
|
||||
```tsx
|
||||
type PanelTab = 'data' | 'widgets' | 'properties' | 'outline';
|
||||
```
|
||||
|
||||
Generalize the selection-follow condition — this file's own doc comment
|
||||
already states the intended rule ("A selection made in the Outline is the
|
||||
exception... every other route brings Properties forward"); today's
|
||||
`tab === 'widgets'` check only implements that rule for one of the two
|
||||
non-Outline tabs, which was invisible with three tabs but becomes a real gap
|
||||
once Data is a second tab someone can be browsing when a selection changes
|
||||
elsewhere:
|
||||
|
||||
```tsx
|
||||
const selection = provider.getSelection();
|
||||
const [shown, setShown] = useState(selection);
|
||||
if (selection !== shown) {
|
||||
setShown(selection);
|
||||
if (selection !== undefined && tab !== 'outline') {
|
||||
setTab('properties');
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
(Only the inner condition changes, from `tab === 'widgets'` to
|
||||
`tab !== 'outline'`.)
|
||||
|
||||
Add the `data` tab first, and rename `widgets`'s label, in the `items` array:
|
||||
|
||||
```tsx
|
||||
items={[
|
||||
{
|
||||
key: 'data',
|
||||
label: t('Data'),
|
||||
children: <DataPanel />,
|
||||
},
|
||||
{
|
||||
key: 'widgets',
|
||||
label: t('Building Blocks'),
|
||||
children: <Palette onAdd={onAdd} />,
|
||||
},
|
||||
{
|
||||
key: 'properties',
|
||||
label: t('Properties'),
|
||||
children: <Inspector />,
|
||||
},
|
||||
{
|
||||
key: 'outline',
|
||||
label: t('Outline'),
|
||||
children: <Outline />,
|
||||
},
|
||||
]}
|
||||
```
|
||||
|
||||
Leave `useState<PanelTab>('widgets')` (the default active tab on mount)
|
||||
unchanged — this PR adds Data to the tab bar without deciding it should also
|
||||
become the tab an author lands on first; that's a product decision for a
|
||||
later change, not an implementation default to slip in unannounced here.
|
||||
|
||||
- [ ] **Step 4: Run the tests to verify they pass**
|
||||
|
||||
Run: `npm run test -- EditorPanel.test.tsx`
|
||||
Expected: PASS, all tests including the four touched/added above.
|
||||
|
||||
- [ ] **Step 5: Run the full DashboardBuilderV2 test directory to check for collateral damage**
|
||||
|
||||
Run: `npm run test -- superset-frontend/src/pages/DashboardBuilderV2`
|
||||
Expected: PASS. This directory's tests are the only place `'Widgets'` as a
|
||||
tab name or `PanelTab` are referenced outside the two files just changed —
|
||||
confirm nothing else (e.g. an `index.test.tsx` mounting the full page) also
|
||||
hardcodes the old label.
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx
|
||||
git commit -m "feat(dashboard-v2): add Data tab to editor panel, rename Widgets to Building Blocks"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Plan self-review
|
||||
|
||||
- **Spec coverage:** tab rename (Task 2) ✓, new Data tab first in order (Task 2) ✓, mock search + expandable mock columns with `ColumnTypeLabel` icons (Task 1) ✓, no API calls/no dataBinding wiring/no drag-drop (both tasks — never introduced) ✓, tests following existing patterns (both tasks) ✓. No spec item lacks a task.
|
||||
- **Placeholder scan:** no TBD/TODO; both components are fully written out above, not summarized.
|
||||
- **Type consistency:** `DataPanel` is a zero-prop `() => ReactElement` in both Task 1's implementation and Task 2's usage (`<DataPanel />`, no props passed). `PanelTab` gains `'data'` in Task 2 and every `items` entry's `key` matches one of the four `PanelTab` union members.
|
||||
@@ -1,750 +0,0 @@
|
||||
# Composite Control Registry Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Extract `DataBinding.metrics` into a standalone, reusable `MetricControl` Pydantic mixin registered in a new `superset_core` composite-control registry, so it can be composed into other widgets' control models via plain inheritance — with zero change to the schema `DataBinding` currently serves.
|
||||
|
||||
**Architecture:** A new `superset_core/widgets/composites.py` module holds `MetricControl` (a `BaseModel` mixin carrying exactly today's `metrics` field) plus a `@composite_control` decorator that registers it — and any future composite — into a discoverable registry, mirroring the existing `@widget`/`superset/widgets/registry.py` pattern already in this codebase. `DataBinding` in `superset/widgets/controls.py` becomes `class DataBinding(MetricControl)`. Because Pydantic always orders inherited fields ahead of a subclass's own fields regardless of redeclaration position, `build_configuration_schema` (`superset_core/semantic_layers/config.py`) gains an additive, opt-in `field_order` override so `DataBinding` can pin its rendered field order back to today's exact sequence.
|
||||
|
||||
**Tech Stack:** Python/Pydantic (backend control models), pytest.
|
||||
|
||||
**Spec:** [docs/superpowers/specs/2026-08-26-composite-control-registry-design.md](../specs/2026-08-26-composite-control-registry-design.md)
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- New Python code needs full type hints and must be mypy-clean (per CLAUDE.md).
|
||||
- New files need the standard ASF license header (`.rat-excludes` doesn't cover these).
|
||||
- Run `pre-commit run --all-files` before pushing (non-negotiable per CLAUDE.md).
|
||||
- No new metric capability — `MetricControl.metrics` is moved verbatim from `DataBinding`, not extended (per spec Scope).
|
||||
- No frontend changes — the served schema for every existing widget is provably unchanged (per spec Scope).
|
||||
- `build_configuration_schema`'s `field_order` override must be additive: every existing caller that doesn't set `field_order` must behave exactly as before.
|
||||
|
||||
---
|
||||
|
||||
## File Structure
|
||||
|
||||
**Backend — new:**
|
||||
- `superset-core/src/superset_core/widgets/composites.py` — `CompositeControlInfo`, `composite_control` decorator, `list_composite_controls()`, `MetricControl`.
|
||||
- `tests/unit_tests/widgets/test_composites.py` — registry unit tests.
|
||||
- `tests/unit_tests/semantic_layers/config_test.py` — `field_order` override unit tests (naming matches this directory's existing `*_test.py` convention, e.g. `schemas_test.py`).
|
||||
|
||||
**Backend — modified:**
|
||||
- `superset-core/src/superset_core/semantic_layers/config.py` — add the `field_order` override to `build_configuration_schema`.
|
||||
- `superset-core/src/superset_core/widgets/__init__.py` — re-export `MetricControl`, `composite_control`, `list_composite_controls`.
|
||||
- `superset/widgets/controls.py` — `DataBinding` drops its inline `metrics` field, inherits `MetricControl`, declares `field_order`.
|
||||
- `tests/unit_tests/widgets/test_registry.py` — add the schema-identity golden-fixture test (this file already tests `DataBinding`'s served schema via `_block("metric-tile")`/`_block("balloons")`, so the new test belongs alongside them).
|
||||
|
||||
---
|
||||
|
||||
## Task 1: `field_order` override on `build_configuration_schema`
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset-core/src/superset_core/semantic_layers/config.py:25-59` (the `build_configuration_schema` function)
|
||||
- Test: `tests/unit_tests/semantic_layers/config_test.py` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: nothing new — pure extension of the existing `build_configuration_schema(config_class: type[BaseModel], configuration: BaseModel | None = None) -> dict[str, Any]` signature.
|
||||
- Produces: `build_configuration_schema` now honors an optional `field_order: ClassVar[list[str]]` attribute on `config_class`, used by Task 3.
|
||||
|
||||
- [ ] **Step 1: Write the failing tests**
|
||||
|
||||
Create `tests/unit_tests/semantic_layers/config_test.py`:
|
||||
|
||||
```python
|
||||
# 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.
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import ClassVar
|
||||
|
||||
import pytest
|
||||
from pydantic import BaseModel
|
||||
|
||||
from superset_core.semantic_layers.config import build_configuration_schema
|
||||
|
||||
|
||||
class _NoOverride(BaseModel):
|
||||
b: int = 0
|
||||
a: int = 0
|
||||
|
||||
|
||||
class _WithOverride(BaseModel):
|
||||
field_order: ClassVar[list[str]] = ["a", "b"]
|
||||
|
||||
b: int = 0
|
||||
a: int = 0
|
||||
|
||||
|
||||
class _WithBadOverride(BaseModel):
|
||||
field_order: ClassVar[list[str]] = ["a", "c"]
|
||||
|
||||
b: int = 0
|
||||
a: int = 0
|
||||
|
||||
|
||||
class _Nested(BaseModel):
|
||||
field_order: ClassVar[list[str]] = ["y", "x"]
|
||||
|
||||
x: int = 0
|
||||
y: int = 0
|
||||
|
||||
|
||||
class _NestedBase(BaseModel):
|
||||
y: int
|
||||
|
||||
|
||||
class _NestedComposed(_NestedBase):
|
||||
field_order: ClassVar[list[str]] = ["y", "x"]
|
||||
|
||||
x: int
|
||||
|
||||
|
||||
class _Outer(BaseModel):
|
||||
nested: _Nested
|
||||
|
||||
|
||||
class _OuterComposed(BaseModel):
|
||||
nested: _NestedComposed
|
||||
|
||||
|
||||
def test_no_field_order_behaves_as_today() -> None:
|
||||
# Unchanged behavior: model-field declaration order (b, a), not alphabetical.
|
||||
schema = build_configuration_schema(_NoOverride)
|
||||
assert list(schema["properties"]) == ["b", "a"]
|
||||
|
||||
|
||||
def test_field_order_override_reorders_properties() -> None:
|
||||
schema = build_configuration_schema(_WithOverride)
|
||||
assert list(schema["properties"]) == ["a", "b"]
|
||||
|
||||
|
||||
def test_field_order_override_must_be_exact_permutation() -> None:
|
||||
with pytest.raises(ValueError, match="field_order"):
|
||||
build_configuration_schema(_WithBadOverride)
|
||||
|
||||
|
||||
def test_field_order_applies_to_nested_defs_models() -> None:
|
||||
# `_Nested` only ever appears inside `$defs`, never as the top-level
|
||||
# `config_class` -- this is the DataBinding-inside-MetricTileControls shape.
|
||||
schema = build_configuration_schema(_Outer)
|
||||
assert list(schema["$defs"]["_Nested"]["properties"]) == ["y", "x"]
|
||||
|
||||
|
||||
def test_field_order_on_nested_model_with_inherited_field() -> None:
|
||||
# `x` is inherited from `_NestedBase` (like DataBinding inheriting
|
||||
# `metrics` from MetricControl), so Pydantic's natural field order would
|
||||
# put `x` first; the override pins it back.
|
||||
schema = build_configuration_schema(_OuterComposed)
|
||||
assert list(schema["$defs"]["_NestedComposed"]["properties"]) == ["y", "x"]
|
||||
assert schema["$defs"]["_NestedComposed"]["required"] == ["y", "x"]
|
||||
```
|
||||
|
||||
- [x] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/semantic_layers/config_test.py -v`
|
||||
Expected: everything except `test_no_field_order_behaves_as_today` FAILS
|
||||
before implementation. Confirmed directly: the first version of this step
|
||||
(before the nested cases were added) showed exactly
|
||||
`test_field_order_override_reorders_properties` and
|
||||
`test_field_order_override_must_be_exact_permutation` failing, matching this
|
||||
prediction.
|
||||
|
||||
- [x] **Step 3: Implement the override**
|
||||
|
||||
Replace the body of `build_configuration_schema` in
|
||||
`superset-core/src/superset_core/semantic_layers/config.py`. **Important — a
|
||||
top-level-only reorder is not enough.** `DataBinding` (Task 3) is never
|
||||
itself the `config_class` passed to `build_configuration_schema`; it only
|
||||
ever appears nested inside another model (e.g.
|
||||
`MetricTileControls.data_binding: DataBinding`), and Pydantic generates each
|
||||
nested model's `$defs` entry using that model's own `model_fields` order,
|
||||
independent of any reordering done to the outer schema. This was verified
|
||||
directly by running a top-level-only version against the real widget
|
||||
registry: `$defs.DataBinding` still came back with `metrics` before
|
||||
`datasetId`. The override below walks every `BaseModel` reachable from
|
||||
`config_class` and applies the same declared-or-derived ordering to each one
|
||||
that has a `$defs` entry:
|
||||
|
||||
```python
|
||||
from __future__ import annotations
|
||||
|
||||
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", {}))):
|
||||
raise ValueError(
|
||||
f"{model_cls.__name__}.field_order must be a permutation of its "
|
||||
f"schema properties; declared={sorted(declared)} actual={sorted(actual)}"
|
||||
)
|
||||
return declared_order
|
||||
|
||||
|
||||
def _reorder(schema_node: dict[str, Any], field_order: list[str]) -> None:
|
||||
"""Reorder ``schema_node``'s ``properties`` (and, for determinism,
|
||||
``required``) to match ``field_order``. Mutates in place."""
|
||||
if (properties := schema_node.get("properties")) is not None:
|
||||
schema_node["properties"] = {
|
||||
key: properties[key] for key in field_order if key in properties
|
||||
}
|
||||
if (required := schema_node.get("required")) is not None:
|
||||
index = {key: position for position, key in enumerate(field_order)}
|
||||
schema_node["required"] = sorted(
|
||||
required, key=lambda key: index.get(key, len(field_order))
|
||||
)
|
||||
|
||||
|
||||
def build_configuration_schema(
|
||||
config_class: type[BaseModel],
|
||||
configuration: BaseModel | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Build a JSON schema from a Pydantic configuration class.
|
||||
|
||||
Handles generic boilerplate that any semantic layer with dynamic fields needs:
|
||||
|
||||
- Reorders properties to match model field order (Pydantic sorts alphabetically),
|
||||
or an explicit ``field_order: ClassVar[list[str]]`` on a model when declared —
|
||||
needed because Pydantic always places an inherited field ahead of a
|
||||
subclass's own fields in ``model_fields``, regardless of where the subclass
|
||||
redeclares it, so composed models can't rely on declaration order alone.
|
||||
Applied to ``config_class`` itself *and* to every nested model that lands in
|
||||
the schema's ``$defs`` — a model's declared/inherited field order isn't only
|
||||
relevant when it's the top-level schema, and Pydantic emits ``$defs`` entries
|
||||
in each nested model's own (potentially inheritance-skewed) field order too.
|
||||
- When ``configuration`` is None, sets ``enum: []`` on all ``x-dynamic`` properties
|
||||
so the frontend renders them as empty dropdowns
|
||||
|
||||
Semantic layer implementations call this instead of
|
||||
``model_json_schema()`` directly,
|
||||
then only need to add their own dynamic population logic.
|
||||
"""
|
||||
schema = config_class.model_json_schema()
|
||||
|
||||
_reorder(schema, _resolve_field_order(config_class, schema))
|
||||
|
||||
defs = schema.get("$defs", {})
|
||||
for nested_cls in _iter_nested_models(config_class, seen=set()):
|
||||
if nested_cls is config_class:
|
||||
continue
|
||||
def_entry = defs.get(nested_cls.__name__)
|
||||
if def_entry is None:
|
||||
continue
|
||||
_reorder(def_entry, _resolve_field_order(nested_cls, def_entry))
|
||||
|
||||
if configuration is None:
|
||||
for prop_schema in schema["properties"].values():
|
||||
if prop_schema.get("x-dynamic"):
|
||||
prop_schema["enum"] = []
|
||||
|
||||
return schema
|
||||
```
|
||||
|
||||
Note: `Any`/`get_args`/`get_origin`/`Iterator` replace the file's original
|
||||
`from typing import Any` import.
|
||||
|
||||
- [x] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/semantic_layers/config_test.py -v`
|
||||
Expected: all 5 PASS. Confirmed.
|
||||
|
||||
- [x] **Step 5: Run the full existing widgets suite to confirm no regression**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ -v`
|
||||
Expected: all PASS unchanged (no model in this codebase sets `field_order` yet, so every existing call falls into the `else` branch, byte-identical to before). Confirmed: 29 passed.
|
||||
|
||||
- [x] **Step 5b: mypy**
|
||||
|
||||
`isinstance(annotation, type) and issubclass(annotation, BaseModel)` alone
|
||||
did not narrow `annotation`'s type enough for mypy to allow
|
||||
`annotation.model_fields` (`error: "type" has no attribute "model_fields"
|
||||
[attr-defined]`) — fixed by binding to an explicitly-annotated local,
|
||||
`model_cls: type[BaseModel] = annotation`, before using it. Confirmed
|
||||
`pre-commit run mypy` passes after this fix.
|
||||
|
||||
- [x] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-core/src/superset_core/semantic_layers/config.py tests/unit_tests/semantic_layers/config_test.py
|
||||
git commit -m "feat(dashboard-v2): add field_order override to build_configuration_schema"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: `MetricControl` composite-control registry
|
||||
|
||||
**Files:**
|
||||
- Create: `superset-core/src/superset_core/widgets/composites.py`
|
||||
- Modify: `superset-core/src/superset_core/widgets/__init__.py`
|
||||
- Test: `tests/unit_tests/widgets/test_composites.py` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: nothing from Task 1.
|
||||
- Produces: `superset_core.widgets.MetricControl` (a `BaseModel` subclass with a single `metrics: list[Any]` field, `x-control: "metric-multi"`), `superset_core.widgets.composite_control` (decorator), `superset_core.widgets.list_composite_controls() -> Mapping[str, CompositeControlInfo]`. Task 3 imports `MetricControl` from `superset_core.widgets`.
|
||||
|
||||
- [x] **Step 1: Write the failing tests**
|
||||
|
||||
Create `tests/unit_tests/widgets/test_composites.py`:
|
||||
|
||||
```python
|
||||
# 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.
|
||||
from __future__ import annotations
|
||||
|
||||
from types import MappingProxyType
|
||||
|
||||
import pytest
|
||||
from pydantic import BaseModel
|
||||
from superset_core.widgets import (
|
||||
composite_control,
|
||||
list_composite_controls,
|
||||
MetricControl,
|
||||
)
|
||||
from superset_core.widgets.composites import _registry
|
||||
|
||||
|
||||
def test_metric_control_is_registered() -> None:
|
||||
info = list_composite_controls()["metric"]
|
||||
assert info.name == "metric"
|
||||
assert info.model is MetricControl
|
||||
|
||||
|
||||
def test_metric_control_declares_metric_multi_field() -> None:
|
||||
schema = MetricControl.model_json_schema()
|
||||
assert schema["properties"]["metrics"]["x-control"] == "metric-multi"
|
||||
|
||||
|
||||
def test_list_composite_controls_is_read_only() -> None:
|
||||
result = list_composite_controls()
|
||||
assert isinstance(result, MappingProxyType)
|
||||
with pytest.raises(TypeError):
|
||||
result["metric"] = None # type: ignore[index]
|
||||
|
||||
|
||||
def test_composite_control_registers_new_entry() -> None:
|
||||
@composite_control(name="test-only", title="Test Only", description="...")
|
||||
class _TestOnly(BaseModel):
|
||||
value: int = 0
|
||||
|
||||
try:
|
||||
info = list_composite_controls()["test-only"]
|
||||
assert info.title == "Test Only"
|
||||
assert info.model is _TestOnly
|
||||
finally:
|
||||
_registry.pop("test-only", None)
|
||||
|
||||
|
||||
def test_composite_control_duplicate_name_raises() -> None:
|
||||
@composite_control(name="dup-test", title="Dup", description="...")
|
||||
class _First(BaseModel):
|
||||
value: int = 0
|
||||
|
||||
try:
|
||||
with pytest.raises(ValueError, match="already registered"):
|
||||
|
||||
@composite_control(name="dup-test", title="Dup2", description="...")
|
||||
class _Second(BaseModel):
|
||||
value: int = 0
|
||||
|
||||
finally:
|
||||
_registry.pop("dup-test", None)
|
||||
```
|
||||
|
||||
- [x] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_composites.py -v`
|
||||
Expected: FAIL with `ModuleNotFoundError` / `ImportError` (`superset_core.widgets.composites` doesn't exist yet).
|
||||
|
||||
- [x] **Step 3: Create `superset-core/src/superset_core/widgets/composites.py`**
|
||||
|
||||
```python
|
||||
# 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.
|
||||
|
||||
"""
|
||||
Reusable, discoverable building blocks for Dashboard V2 widget control models.
|
||||
|
||||
A composite control is a ``BaseModel`` mixin carrying one or more fields
|
||||
(with their ``x-control`` schema extras already set) that a widget's
|
||||
``controls_class`` composes in via plain single inheritance — no nesting, so
|
||||
the composed field renders exactly where a directly-declared field would.
|
||||
The ``@composite_control`` decorator only registers the class for discovery
|
||||
(docs generation, MCP tooling); composing one into a widget never touches the
|
||||
registry.
|
||||
|
||||
Composing more than one composite control into the same model (multiple
|
||||
inheritance across two or more registered mixins) is unsupported for now —
|
||||
see the design spec's Scope section.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from dataclasses import dataclass
|
||||
from types import MappingProxyType
|
||||
from typing import Any, Callable, Mapping
|
||||
|
||||
from pydantic import BaseModel, ConfigDict, Field
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class CompositeControlInfo:
|
||||
name: str
|
||||
title: str
|
||||
description: str
|
||||
model: type[BaseModel]
|
||||
|
||||
|
||||
_registry: dict[str, CompositeControlInfo] = {}
|
||||
|
||||
|
||||
def composite_control(
|
||||
name: str, title: str, description: str
|
||||
) -> Callable[[type[BaseModel]], type[BaseModel]]:
|
||||
"""Register a reusable Pydantic mixin as a discoverable composite control.
|
||||
|
||||
Composing one into a widget's ``controls_class`` is plain inheritance —
|
||||
this decorator only makes the class discoverable via
|
||||
``list_composite_controls()``.
|
||||
"""
|
||||
|
||||
def decorator(cls: type[BaseModel]) -> type[BaseModel]:
|
||||
if name in _registry:
|
||||
raise ValueError(f"composite control {name!r} already registered")
|
||||
_registry[name] = CompositeControlInfo(name, title, description, cls)
|
||||
return cls
|
||||
|
||||
return decorator
|
||||
|
||||
|
||||
def list_composite_controls() -> Mapping[str, CompositeControlInfo]:
|
||||
"""Read-only view of registered composite controls, for docs/MCP
|
||||
discovery. An extension-defined composite appears only once its defining
|
||||
module has been imported (decorator side effect, same as ``@widget``)."""
|
||||
return MappingProxyType(_registry)
|
||||
|
||||
|
||||
@composite_control(
|
||||
name="metric",
|
||||
title="Metrics",
|
||||
description=(
|
||||
"Reusable metric-list field (saved-metric names or ad-hoc SIMPLE "
|
||||
"aggregates)."
|
||||
),
|
||||
)
|
||||
class MetricControl(BaseModel):
|
||||
"""Mixin providing a ``metrics`` field, extracted verbatim from
|
||||
``DataBinding`` for reuse outside it."""
|
||||
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
metrics: list[Any] = Field(
|
||||
title="Metrics",
|
||||
description=(
|
||||
"Metrics to fetch. Each entry is EITHER a string naming a saved "
|
||||
'metric on the dataset (e.g. "count"), OR an ad-hoc aggregate '
|
||||
"object of the shape "
|
||||
'{"expressionType": "SIMPLE", "column": {"column_name": "<col>"}, '
|
||||
'"aggregate": "SUM"|"AVG"|"COUNT"|"COUNT_DISTINCT"|"MIN"|"MAX", '
|
||||
'"label": "<optional display label>"}. Do not pass a raw SQL string '
|
||||
'like "SUM(sales)" — a plain string is looked up as a saved-metric '
|
||||
"name, not evaluated as an expression."
|
||||
),
|
||||
json_schema_extra={"x-control": "metric-multi", "x-language": "json"},
|
||||
)
|
||||
```
|
||||
|
||||
- [x] **Step 4: Re-export from `superset-core/src/superset_core/widgets/__init__.py`**
|
||||
|
||||
```python
|
||||
from superset_core.widgets.base import Widget as Widget
|
||||
from superset_core.widgets.composites import (
|
||||
composite_control as composite_control,
|
||||
list_composite_controls as list_composite_controls,
|
||||
MetricControl as MetricControl,
|
||||
)
|
||||
from superset_core.widgets.decorators import widget as widget
|
||||
```
|
||||
|
||||
- [x] **Step 5: Run tests to verify they pass**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_composites.py -v`
|
||||
Expected: all 5 PASS. Confirmed.
|
||||
|
||||
- [x] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-core/src/superset_core/widgets/composites.py superset-core/src/superset_core/widgets/__init__.py tests/unit_tests/widgets/test_composites.py
|
||||
git commit -m "feat(dashboard-v2): add composite control registry with MetricControl"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: `DataBinding` composes `MetricControl`
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset/widgets/controls.py:33-77` (the `DataBinding` class)
|
||||
- Modify: `tests/unit_tests/widgets/test_registry.py` (add the golden-fixture test)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `superset_core.widgets.MetricControl` (Task 2), `build_configuration_schema`'s `field_order` support (Task 1).
|
||||
- Produces: nothing new — `DataBinding`'s public shape (the served schema) is unchanged by definition; this task's entire purpose is proving that.
|
||||
|
||||
- [x] **Step 1: Write the failing golden-fixture test**
|
||||
|
||||
Append to `tests/unit_tests/widgets/test_registry.py` (uses the `_block` helper already defined at the top of that file):
|
||||
|
||||
```python
|
||||
def test_data_binding_schema_is_unchanged_after_metric_control_extraction() -> None:
|
||||
# Golden fixture captured from `main` before DataBinding composed
|
||||
# MetricControl, via `registry.get("metric-tile").get_control_schema(None, None)`.
|
||||
# Guards against both the extraction itself and the field_order fix
|
||||
# regressing the served schema — checked at the same boundary the
|
||||
# Inspector and MCP tools consume, not just raw model_json_schema().
|
||||
schema = _block("metric-tile").get_control_schema(None, None)
|
||||
data_binding = schema["$defs"]["DataBinding"]
|
||||
|
||||
assert list(data_binding["properties"]) == [
|
||||
"datasetId",
|
||||
"metrics",
|
||||
"dimensions",
|
||||
"rowLimit",
|
||||
]
|
||||
assert data_binding["required"] == ["datasetId", "metrics"]
|
||||
assert data_binding["properties"]["metrics"] == {
|
||||
"description": (
|
||||
"Metrics to fetch. Each entry is EITHER a string naming a saved "
|
||||
'metric on the dataset (e.g. "count"), OR an ad-hoc aggregate '
|
||||
"object of the shape "
|
||||
'{"expressionType": "SIMPLE", "column": {"column_name": "<col>"}, '
|
||||
'"aggregate": "SUM"|"AVG"|"COUNT"|"COUNT_DISTINCT"|"MIN"|"MAX", '
|
||||
'"label": "<optional display label>"}. Do not pass a raw SQL string '
|
||||
'like "SUM(sales)" — a plain string is looked up as a saved-metric '
|
||||
"name, not evaluated as an expression."
|
||||
),
|
||||
"items": {},
|
||||
"title": "Metrics",
|
||||
"type": "array",
|
||||
"x-control": "metric-multi",
|
||||
"x-language": "json",
|
||||
}
|
||||
assert data_binding["properties"]["datasetId"] == {
|
||||
"description": "Numeric id of the dataset to query.",
|
||||
"title": "Dataset ID",
|
||||
"type": "integer",
|
||||
}
|
||||
assert data_binding["properties"]["dimensions"] == {
|
||||
"description": "Columns to group by (the categories / series).",
|
||||
"items": {"type": "string"},
|
||||
"title": "Dimensions",
|
||||
"type": "array",
|
||||
"x-control": "column-multi",
|
||||
}
|
||||
assert data_binding["properties"]["rowLimit"] == {
|
||||
"default": 1000,
|
||||
"description": "Maximum number of rows to fetch.",
|
||||
"minimum": 1,
|
||||
"title": "Row limit",
|
||||
"type": "integer",
|
||||
}
|
||||
```
|
||||
|
||||
Also add the MCP-boundary variant, exercising the other consumer of the same
|
||||
schema — **note this is not a `$defs` lookup**: verified directly that the
|
||||
MCP tool's minimal-viable pruning (`schema_tools.py`) inlines a *mandatory*
|
||||
nested object (like `dataBinding`) recursively rather than leaving a `$ref`
|
||||
into `$defs`, so all four properties land under
|
||||
`result["properties"]["dataBinding"]["properties"]` instead:
|
||||
|
||||
```python
|
||||
def test_data_binding_schema_unchanged_via_mcp_boundary() -> None:
|
||||
from superset.mcp_service.widgets.tool.get_widget_control_schema import (
|
||||
_get_widget_control_schema_impl,
|
||||
)
|
||||
|
||||
# dataBinding is mandatory, so the minimal-viable pruning inlines it
|
||||
# (recursing into its own mandatory leaves) rather than leaving a $ref
|
||||
# into $defs -- a different code path through schema_tools.py than the
|
||||
# REST/get_control_schema boundary above, so this exercises the field
|
||||
# order fix against progressive disclosure too.
|
||||
result = _get_widget_control_schema_impl("metric-tile")
|
||||
data_binding = result["properties"]["dataBinding"]
|
||||
assert list(data_binding["properties"]) == [
|
||||
"datasetId",
|
||||
"metrics",
|
||||
"dimensions",
|
||||
"rowLimit",
|
||||
]
|
||||
```
|
||||
|
||||
- [x] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_registry.py -v -k data_binding_schema`
|
||||
Expected: PASS actually — `DataBinding` hasn't changed yet, so this documents
|
||||
current behavior. This is expected; the point of Step 1 here is to capture
|
||||
the golden fixture *before* touching `DataBinding`, not to see it fail. Confirm
|
||||
both new tests pass before proceeding to Step 3.
|
||||
|
||||
- [x] **Step 3: Refactor `DataBinding` in `superset/widgets/controls.py`**
|
||||
|
||||
Change the imports at the top of the file to add:
|
||||
|
||||
```python
|
||||
from typing import Any, ClassVar
|
||||
|
||||
from superset_core.widgets import MetricControl
|
||||
```
|
||||
|
||||
Replace the `DataBinding` class body:
|
||||
|
||||
```python
|
||||
class DataBinding(MetricControl):
|
||||
"""Query binding for a data-backed widget (mirrors the frontend
|
||||
``DataBindingSpec``). ``datasetId`` and ``metrics`` are mandatory; the rest
|
||||
are optional."""
|
||||
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
# Pydantic places an inherited field (``metrics``, from ``MetricControl``)
|
||||
# ahead of this class's own fields in ``model_fields`` regardless of
|
||||
# declaration order, so the rendered field order needs to be pinned
|
||||
# explicitly to match what this class served before the extraction.
|
||||
field_order: ClassVar[list[str]] = [
|
||||
"datasetId",
|
||||
"metrics",
|
||||
"dimensions",
|
||||
"rowLimit",
|
||||
]
|
||||
|
||||
dataset_id: int = Field(
|
||||
alias="datasetId",
|
||||
title="Dataset ID",
|
||||
description="Numeric id of the dataset to query.",
|
||||
)
|
||||
dimensions: list[str] = Field(
|
||||
default_factory=list,
|
||||
title="Dimensions",
|
||||
description="Columns to group by (the categories / series).",
|
||||
json_schema_extra={"x-control": "column-multi"},
|
||||
)
|
||||
row_limit: int = Field(
|
||||
default=1000,
|
||||
ge=1,
|
||||
alias="rowLimit",
|
||||
title="Row limit",
|
||||
description="Maximum number of rows to fetch.",
|
||||
)
|
||||
```
|
||||
|
||||
Remove the now-unused `metrics` field declaration that previously lived on
|
||||
`DataBinding` (it's inherited from `MetricControl` instead).
|
||||
|
||||
- [x] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_registry.py -v -k data_binding_schema`
|
||||
Expected: both tests still PASS — now proving the *post-refactor* schema
|
||||
matches the golden fixture, not just documenting the pre-refactor one.
|
||||
Confirmed — including after fixing the MCP-boundary test's assertion path
|
||||
(see the note above the test).
|
||||
|
||||
- [x] **Step 5: Run the full widgets + MCP widget-tool test suites**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ tests/unit_tests/semantic_layers/ -v`
|
||||
Expected: all PASS, no fixture or assertion needed updating anywhere else
|
||||
(per spec, this is the non-breaking proof). Confirmed: 431 passed.
|
||||
|
||||
- [x] **Step 6: Run mypy on the touched files**
|
||||
|
||||
Run: `pre-commit run mypy --files superset/widgets/controls.py superset-core/src/superset_core/widgets/composites.py superset-core/src/superset_core/widgets/__init__.py superset-core/src/superset_core/semantic_layers/config.py`
|
||||
(No standalone `mypy` module is installed in any local venv — `pre-commit run
|
||||
mypy` is the only working invocation; it runs mypy in its own managed
|
||||
environment.)
|
||||
Expected: no errors. Confirmed, after the Task 1 narrowing fix.
|
||||
|
||||
- [x] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add superset/widgets/controls.py tests/unit_tests/widgets/test_registry.py
|
||||
git commit -m "refactor(dashboard-v2): compose DataBinding.metrics from MetricControl"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Final Verification
|
||||
|
||||
- [x] Run `pre-commit run --files <all touched files>` and fix anything it flags. Confirmed clean (auto-walrus, mypy, ruff-format, ruff, pylint all pass; ruff/ruff-format auto-fixed import order and a walrus-operator simplification in `config.py` along the way — re-verify tests after any auto-fix, since files change on disk).
|
||||
- [x] Run the full backend unit suite once more: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ tests/unit_tests/semantic_layers/ -v` — 431 passed.
|
||||
- [x] Confirm no frontend files were touched: `git status --porcelain -- superset-frontend/` — empty. Confirmed.
|
||||
@@ -1,801 +0,0 @@
|
||||
# Control Dependency Graph Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Let a widget register more than one dynamic-field enricher, derive their execution order and a dependency graph from the fields' existing `x-dependsOn` declarations, detect a cyclic graph once at widget-registration time, and retire the single `Widget.enrich_schema` hook in favor of this mechanism — retrofitting `Balloons` (the only current dynamic-field widget) as the proof case with zero behavior change.
|
||||
|
||||
**Architecture:** A new `superset_core/widgets/enrichment.py` walks a built (pre-enrichment) control schema to find every `x-dynamic` field, builds a dependency graph from each field's `x-dependsOn` list (an entry naming another dynamic field's path is an ordering edge; anything else is a truthiness gate against the parsed control values), topologically sorts it (raising on a cycle), and runs each field's registered enricher in that order, threading prior enrichers' return values forward. `Widget.get_control_schema` (superset_core) drives this pipeline instead of calling a single `enrich_schema` override; the `@widget` decorator (`inject_widget_implementations`) calls `get_control_schema(None, None)` once at registration so a cyclic graph fails the import instead of surfacing at request time.
|
||||
|
||||
**Tech Stack:** Python/Pydantic, pytest.
|
||||
|
||||
**Spec:** [docs/superpowers/specs/2026-08-26-control-dependency-graph-design.md](../specs/2026-08-26-control-dependency-graph-design.md)
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- New Python code needs full type hints and must be mypy-clean (per CLAUDE.md); `pre-commit run mypy` is the only working invocation in this environment (no venv here has a standalone `mypy` module).
|
||||
- New files need the standard ASF license header.
|
||||
- Run `pre-commit run --files <touched files>` before considering any task done; re-run tests after any auto-fix since files change on disk.
|
||||
- No new dynamic-field capability, no frontend changes — Balloons' served schema must remain byte-identical before/after (golden-fixture discipline, same as the composite-control-registry slice).
|
||||
- Corrections found while writing this plan, not yet reflected in the spec (fold into the spec-reconciliation step at the end, same as last time): (1) `EnricherFn` must receive the **whole schema**, not just its own field's fragment — Balloons' real enricher reads `schema["$defs"]["SeriesStyle"]`, a sibling `$defs` entry, not something reachable from its own node alone. (2) `check_dependencies` (`superset_core/semantic_layers/config.py`) resolves a dependency name via `getattr(configuration, dep, None)` using the *raw* `x-dependsOn` string (e.g. `"dataBinding"`) directly as a Python attribute name — but Pydantic attribute access always uses the Python field name (`data_binding`), never the alias, even under `populate_by_name=True`. Verified directly: `getattr(parsed, "dataBinding", "MISSING")` returns `"MISSING"` on a real parsed `BalloonsControls` instance. `check_dependencies` has zero callers today, so this has never been exercised — it must resolve alias → Python name before `getattr`, since this plan is its first real caller. (3) The `x-dependsOn: ["dataBinding"]` gate is coarser than what Balloons actually needs: `dataBinding` is a required field, so it's truthy whenever `parsed` exists at all — it does not capture "`dimensions` is non-empty" or "the `series` parameter is non-empty" (the latter isn't even a field on `parsed`, so no `x-dependsOn` entry could ever express it). Balloons' enricher body must keep its existing fine-grained `if not dimensions or not series: return` guard; the schema-level gate is an additional coarse pre-filter for the *new* multi-hop-ordering feature, not a replacement for per-field logic.
|
||||
|
||||
---
|
||||
|
||||
## File Structure
|
||||
|
||||
**Backend — new:**
|
||||
- `superset-core/src/superset_core/widgets/enrichment.py` — `EnricherFn`, `dynamic_field_paths`, `build_dependency_graph`, `toposort_or_raise`, `run_enrichers`.
|
||||
- `tests/unit_tests/widgets/test_enrichment.py` — unit tests for the module above, using synthetic schemas (no real widget needed).
|
||||
|
||||
**Backend — modified:**
|
||||
- `superset-core/src/superset_core/semantic_layers/config.py` — fix `check_dependencies`'s alias resolution.
|
||||
- `superset-core/src/superset_core/widgets/base.py` — `Widget` gains `enrichers: ClassVar[dict[str, EnricherFn]]`; `get_control_schema` drives the new pipeline; `enrich_schema` is removed.
|
||||
- `superset-core/src/superset_core/widgets/__init__.py` — re-export `EnricherFn` (widget authors need the type to annotate their enrichers).
|
||||
- `superset/core/api/core_api_injection.py` — `widget_impl`'s `decorator` calls `cls.get_control_schema(None, None)` once after registering, so a cyclic graph fails import.
|
||||
- `superset/widgets/builtin.py` — `Balloons.enrich_schema` becomes `Balloons._populate_series` registered via `enrichers`.
|
||||
- `tests/unit_tests/widgets/test_builtin.py` — unchanged assertions, but now exercised through the new pipeline (regression proof).
|
||||
- `tests/unit_tests/widgets/test_registry.py` — add a registration-time cycle-detection test.
|
||||
|
||||
---
|
||||
|
||||
## Task 1: `enrichment.py` — graph, toposort, and the enricher runner
|
||||
|
||||
**Files:**
|
||||
- Create: `superset-core/src/superset_core/widgets/enrichment.py`
|
||||
- Test: `tests/unit_tests/widgets/test_enrichment.py` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: nothing new.
|
||||
- Produces: `EnricherFn = Callable[[dict[str, Any], dict[str, Any], BaseModel | None, list[str], dict[str, Any]], Any]` — `(schema, node, parsed, series, upstream_results) -> Any`; `dynamic_field_paths(schema) -> dict[str, dict[str, Any]]`; `build_dependency_graph(fields: dict[str, dict[str, Any]]) -> dict[str, list[str]]`; `toposort_or_raise(graph: dict[str, list[str]], widget_type: str) -> list[str]`; `run_enrichers(schema, fields, order, enrichers, parsed, series) -> None`. Task 2 wires these into `Widget.get_control_schema`.
|
||||
|
||||
- [x] **Step 1: Write the failing tests**
|
||||
|
||||
Create `tests/unit_tests/widgets/test_enrichment.py`:
|
||||
|
||||
```python
|
||||
# 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.
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from superset_core.widgets.enrichment import (
|
||||
build_dependency_graph,
|
||||
dynamic_field_paths,
|
||||
run_enrichers,
|
||||
toposort_or_raise,
|
||||
)
|
||||
|
||||
|
||||
def test_dynamic_field_paths_finds_top_level_field() -> None:
|
||||
schema = {
|
||||
"properties": {
|
||||
"a": {"x-dynamic": True, "x-dependsOn": ["b"]},
|
||||
"b": {"type": "string"},
|
||||
}
|
||||
}
|
||||
fields = dynamic_field_paths(schema)
|
||||
assert list(fields) == ["a"]
|
||||
assert fields["a"] is schema["properties"]["a"]
|
||||
|
||||
|
||||
def test_dynamic_field_paths_finds_nested_defs_field() -> None:
|
||||
# Mirrors Customization.series's actual shape: the dynamic field lives
|
||||
# inside a $defs entry referenced by $ref, not directly in properties.
|
||||
schema = {
|
||||
"properties": {"customize": {"$ref": "#/$defs/Customization"}},
|
||||
"$defs": {
|
||||
"Customization": {
|
||||
"properties": {
|
||||
"series": {"x-dynamic": True, "x-dependsOn": ["dataBinding"]}
|
||||
}
|
||||
}
|
||||
},
|
||||
}
|
||||
fields = dynamic_field_paths(schema)
|
||||
assert list(fields) == ["customize/series"]
|
||||
|
||||
|
||||
def test_build_dependency_graph_edge_vs_gate() -> None:
|
||||
fields = {
|
||||
"a": {"x-dynamic": True, "x-dependsOn": ["b", "staticField"]},
|
||||
"b": {"x-dynamic": True},
|
||||
}
|
||||
graph = build_dependency_graph(fields)
|
||||
# "b" is a known dynamic path -> ordering edge; "staticField" isn't -> not an edge.
|
||||
assert graph == {"a": ["b"], "b": []}
|
||||
|
||||
|
||||
def test_toposort_orders_a_chain() -> None:
|
||||
graph = {"c": ["b"], "b": ["a"], "a": []}
|
||||
order = toposort_or_raise(graph, "test-widget")
|
||||
assert order.index("a") < order.index("b") < order.index("c")
|
||||
|
||||
|
||||
def test_toposort_raises_on_cycle() -> None:
|
||||
graph = {"a": ["b"], "b": ["a"]}
|
||||
with pytest.raises(ValueError, match="a") as exc_info:
|
||||
toposort_or_raise(graph, "test-widget")
|
||||
assert "b" in str(exc_info.value)
|
||||
|
||||
|
||||
def test_run_enrichers_threads_upstream_result_forward() -> None:
|
||||
schema = {
|
||||
"properties": {
|
||||
"a": {"x-dynamic": True},
|
||||
"b": {"x-dynamic": True, "x-dependsOn": ["a"]},
|
||||
}
|
||||
}
|
||||
fields = dynamic_field_paths(schema)
|
||||
order = toposort_or_raise(build_dependency_graph(fields), "test-widget")
|
||||
|
||||
seen_upstream = {}
|
||||
|
||||
def enrich_a(schema_arg, node, parsed, series, upstream):
|
||||
node["computed"] = "from-a"
|
||||
return "a-result"
|
||||
|
||||
def enrich_b(schema_arg, node, parsed, series, upstream):
|
||||
seen_upstream.update(upstream)
|
||||
|
||||
run_enrichers(schema, fields, order, {"a": enrich_a, "b": enrich_b}, None, [])
|
||||
|
||||
assert schema["properties"]["a"]["computed"] == "from-a"
|
||||
assert seen_upstream == {"a": "a-result"}
|
||||
|
||||
|
||||
def test_run_enrichers_skips_ungated_field_without_error() -> None:
|
||||
# No enricher registered for a discovered dynamic field: no-op, not an error.
|
||||
schema = {"properties": {"a": {"x-dynamic": True}}}
|
||||
fields = dynamic_field_paths(schema)
|
||||
order = toposort_or_raise(build_dependency_graph(fields), "test-widget")
|
||||
run_enrichers(schema, fields, order, {}, None, []) # must not raise
|
||||
```
|
||||
|
||||
- [x] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_enrichment.py -v`
|
||||
Expected: all FAIL with `ModuleNotFoundError` (`superset_core.widgets.enrichment` doesn't exist yet).
|
||||
|
||||
- [x] **Step 3: Implement `superset-core/src/superset_core/widgets/enrichment.py`**
|
||||
|
||||
```python
|
||||
# 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)
|
||||
upstream_results[path] = result
|
||||
```
|
||||
|
||||
- [x] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_enrichment.py -v`
|
||||
Expected: all 7 PASS.
|
||||
|
||||
- [x] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-core/src/superset_core/widgets/enrichment.py tests/unit_tests/widgets/test_enrichment.py
|
||||
git commit -m "feat(dashboard-v2): add dependency graph + enricher runner for dynamic control fields"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: Fix `check_dependencies`'s alias resolution
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset-core/src/superset_core/semantic_layers/config.py`
|
||||
- Test: `tests/unit_tests/semantic_layers/config_test.py` (append)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: nothing new.
|
||||
- Produces: `check_dependencies` now resolves an `x-dependsOn` entry against the parsed model's *alias* (matching what's actually written in the schema) before falling back to the raw name, instead of only ever trying the raw name directly.
|
||||
|
||||
- [x] **Step 1: Write the failing test**
|
||||
|
||||
Append to `tests/unit_tests/semantic_layers/config_test.py`:
|
||||
|
||||
```python
|
||||
from pydantic import ConfigDict, Field
|
||||
|
||||
from superset_core.semantic_layers.config import check_dependencies
|
||||
|
||||
|
||||
class _Aliased(BaseModel):
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
data_binding: int = Field(alias="dataBinding")
|
||||
|
||||
|
||||
def test_check_dependencies_resolves_alias() -> None:
|
||||
# x-dependsOn is written using the schema-facing alias ("dataBinding"),
|
||||
# but Pydantic attribute access always uses the Python field name
|
||||
# ("data_binding") -- confirmed directly that getattr(parsed, "dataBinding")
|
||||
# misses even under populate_by_name=True.
|
||||
configuration = _Aliased(dataBinding=1)
|
||||
assert getattr(configuration, "dataBinding", "MISSING") == "MISSING"
|
||||
assert check_dependencies({"x-dependsOn": ["dataBinding"]}, configuration)
|
||||
|
||||
|
||||
def test_check_dependencies_false_when_dependency_falsy() -> None:
|
||||
configuration = _Aliased(dataBinding=0)
|
||||
assert not check_dependencies({"x-dependsOn": ["dataBinding"]}, configuration)
|
||||
|
||||
|
||||
def test_check_dependencies_true_when_no_dependencies_declared() -> None:
|
||||
configuration = _Aliased(dataBinding=0)
|
||||
assert check_dependencies({}, configuration)
|
||||
```
|
||||
|
||||
- [x] **Step 2: Run tests to verify they fail**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/semantic_layers/config_test.py -v -k check_dependencies`
|
||||
Expected: `test_check_dependencies_resolves_alias` FAILS (`check_dependencies` returns falsy — `getattr(configuration, "dataBinding", None)` is `None`); the other two already pass (they don't exercise the alias bug).
|
||||
|
||||
- [x] **Step 3: Fix `check_dependencies`**
|
||||
|
||||
In `superset-core/src/superset_core/semantic_layers/config.py`, replace:
|
||||
|
||||
```python
|
||||
def check_dependencies(
|
||||
prop_schema: dict[str, Any],
|
||||
configuration: BaseModel,
|
||||
) -> bool:
|
||||
"""
|
||||
Check whether a dynamic property's dependencies are satisfied.
|
||||
|
||||
Reads the ``x-dependsOn`` list from the property schema and returns ``True``
|
||||
when every referenced attribute on ``configuration`` is truthy.
|
||||
"""
|
||||
dependencies = prop_schema.get("x-dependsOn", [])
|
||||
return all(getattr(configuration, dep, None) for dep in dependencies)
|
||||
```
|
||||
|
||||
with:
|
||||
|
||||
```python
|
||||
def check_dependencies(
|
||||
prop_schema: dict[str, Any],
|
||||
configuration: BaseModel,
|
||||
) -> bool:
|
||||
"""
|
||||
Check whether a dynamic property's dependencies are satisfied.
|
||||
|
||||
Reads the ``x-dependsOn`` list from the property schema and returns ``True``
|
||||
when every referenced attribute on ``configuration`` is truthy. Entries are
|
||||
written using the schema-facing alias (e.g. ``"dataBinding"``), so each is
|
||||
resolved to its Pydantic field name before ``getattr`` -- Pydantic attribute
|
||||
access always uses the field name, never the alias, even under
|
||||
``populate_by_name=True``.
|
||||
"""
|
||||
dependencies = prop_schema.get("x-dependsOn", [])
|
||||
alias_to_name = {
|
||||
(field.alias or name): name
|
||||
for name, field in type(configuration).model_fields.items()
|
||||
}
|
||||
return all(
|
||||
getattr(configuration, alias_to_name.get(dep, dep), None)
|
||||
for dep in dependencies
|
||||
)
|
||||
```
|
||||
|
||||
- [x] **Step 4: Run tests to verify they pass**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/semantic_layers/config_test.py -v`
|
||||
Expected: all PASS (8 total: the 5 from the field-order slice plus these 3).
|
||||
|
||||
- [x] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-core/src/superset_core/semantic_layers/config.py tests/unit_tests/semantic_layers/config_test.py
|
||||
git commit -m "fix(dashboard-v2): resolve x-dependsOn aliases in check_dependencies"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: Wire the pipeline into `Widget.get_control_schema`, retire `enrich_schema`
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset-core/src/superset_core/widgets/base.py`
|
||||
- Modify: `superset-core/src/superset_core/widgets/__init__.py`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `enrichment.py`'s four functions (Task 1).
|
||||
- Produces: `Widget.enrichers: ClassVar[dict[str, EnricherFn]] = {}`; `get_control_schema` runs the full pipeline. `enrich_schema` no longer exists on `Widget`. Task 5 (Balloons retrofit) depends on this.
|
||||
|
||||
- [x] **Step 1: Modify `superset-core/src/superset_core/widgets/base.py`**
|
||||
|
||||
Replace the imports and the `get_control_schema`/`enrich_schema` methods:
|
||||
|
||||
```python
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from typing import Any, ClassVar
|
||||
|
||||
from pydantic import BaseModel, ValidationError
|
||||
|
||||
from superset_core.semantic_layers.config import build_configuration_schema
|
||||
from superset_core.widgets.enrichment import (
|
||||
build_dependency_graph,
|
||||
dynamic_field_paths,
|
||||
EnricherFn,
|
||||
run_enrichers,
|
||||
toposort_or_raise,
|
||||
)
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
class Widget:
|
||||
widget_type: str
|
||||
name: str
|
||||
description: str = ""
|
||||
controls_class: type[BaseModel]
|
||||
enrichers: ClassVar[dict[str, EnricherFn]] = {}
|
||||
|
||||
@classmethod
|
||||
def get_control_schema(
|
||||
cls,
|
||||
control_values: dict[str, Any] | None = None,
|
||||
series: list[str] | None = None,
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Return the JSON Schema for this widget's controls.
|
||||
|
||||
``control_values`` (the full current ``node.props``) is accepted so
|
||||
dynamic fields can be enriched from it, mirroring the Semantic Layer.
|
||||
``series`` carries the distinct dimension values the frontend
|
||||
discovered from the query results (they cannot come from
|
||||
``control_values`` alone, which hold the dimension *name*, not its
|
||||
values). Partial or invalid values during editing are tolerated and
|
||||
fall back to the base schema; enrichment errors propagate so the caller
|
||||
can degrade gracefully.
|
||||
|
||||
Every ``x-dynamic`` field found in the built schema is enriched (if
|
||||
this widget has a registered enricher for its path) in dependency
|
||||
order -- derived from each field's own ``x-dependsOn``, see
|
||||
``superset_core.widgets.enrichment``. A cyclic dependency raises
|
||||
``ValueError``; for a built-in or registered widget this is caught
|
||||
once at registration time (``inject_widget_implementations``), not
|
||||
on every request.
|
||||
"""
|
||||
parsed: BaseModel | None = None
|
||||
if control_values:
|
||||
try:
|
||||
parsed = cls.controls_class.model_validate(control_values)
|
||||
except Exception: # pylint: disable=broad-except
|
||||
# Partial control values during editing are expected; fall back
|
||||
# to the base schema.
|
||||
logger.debug(
|
||||
"Could not validate control values for %s; using base schema",
|
||||
cls.widget_type,
|
||||
exc_info=True,
|
||||
)
|
||||
schema = build_configuration_schema(cls.controls_class, parsed)
|
||||
fields = dynamic_field_paths(schema)
|
||||
order = toposort_or_raise(build_dependency_graph(fields), cls.widget_type)
|
||||
run_enrichers(schema, fields, order, cls.enrichers, parsed, series or [])
|
||||
return schema
|
||||
|
||||
@classmethod
|
||||
def validate_control_values(
|
||||
cls,
|
||||
control_values: dict[str, Any] | None,
|
||||
) -> list[dict[str, Any]]:
|
||||
"""
|
||||
Strictly validate ``control_values`` against ``controls_class``,
|
||||
returning a list of ``{"loc", "message"}`` errors (empty when valid).
|
||||
|
||||
This is the **commit-time** gate, distinct from ``get_control_schema``
|
||||
(which tolerates partial/invalid values so a form can be edited without
|
||||
erroring). It runs the model's full validation — including cross-field
|
||||
rules declared as Pydantic ``@model_validator`` / ``@field_validator``
|
||||
on ``controls_class`` — so a caller (or an AI) gets an actionable
|
||||
message instead of a silently-broken widget. It is widget-agnostic:
|
||||
every rule lives declaratively on the model; this method just surfaces
|
||||
whatever the model enforces.
|
||||
"""
|
||||
if not control_values:
|
||||
return []
|
||||
try:
|
||||
cls.controls_class.model_validate(control_values)
|
||||
except ValidationError as ex:
|
||||
return [
|
||||
{
|
||||
"loc": [str(part) for part in error.get("loc", ())],
|
||||
"message": error.get("msg", ""),
|
||||
}
|
||||
for error in ex.errors()
|
||||
]
|
||||
return []
|
||||
```
|
||||
|
||||
(Keep the module and class docstrings as they are today — only the imports
|
||||
and the two methods change.)
|
||||
|
||||
- [x] **Step 2: Re-export `EnricherFn` from `superset-core/src/superset_core/widgets/__init__.py`**
|
||||
|
||||
```python
|
||||
from superset_core.widgets.base import Widget as Widget
|
||||
from superset_core.widgets.composites import (
|
||||
composite_control as composite_control,
|
||||
list_composite_controls as list_composite_controls,
|
||||
MetricControl as MetricControl,
|
||||
)
|
||||
from superset_core.widgets.decorators import widget as widget
|
||||
from superset_core.widgets.enrichment import EnricherFn as EnricherFn
|
||||
```
|
||||
|
||||
- [x] **Step 3: Run the existing widgets suite — expect Balloons failures**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ -v`
|
||||
Expected: `test_builtin.py`'s Balloons tests now FAIL, because `Balloons` still
|
||||
overrides the now-nonexistent `enrich_schema` (dead code — no longer called)
|
||||
and has no `enrichers` registered, so `series` stays open-ended in every
|
||||
case. Every non-Balloons test should still PASS. This is expected — Task 5
|
||||
fixes it; do not treat this as a regression to chase down within this task.
|
||||
|
||||
- [x] **Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add superset-core/src/superset_core/widgets/base.py superset-core/src/superset_core/widgets/__init__.py
|
||||
git commit -m "feat(dashboard-v2): drive get_control_schema through the enricher pipeline"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 4: Registration-time cycle detection
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset/core/api/core_api_injection.py`
|
||||
- Test: `tests/unit_tests/widgets/test_registry.py` (append)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: Task 3's `get_control_schema` (which now raises `ValueError` internally on a cyclic graph, via `toposort_or_raise`).
|
||||
- Produces: nothing new — this task only changes *when* that `ValueError` can surface (at `@widget` application, not first request).
|
||||
|
||||
- [x] **Step 1: Write the failing test**
|
||||
|
||||
Append to `tests/unit_tests/widgets/test_registry.py`:
|
||||
|
||||
```python
|
||||
def test_widget_registration_raises_on_cyclic_dependency() -> None:
|
||||
from typing import Any, ClassVar
|
||||
|
||||
from pydantic import BaseModel, Field
|
||||
from superset_core.widgets import widget, Widget
|
||||
|
||||
class _CyclicControls(BaseModel):
|
||||
a: dict[str, Any] = Field(
|
||||
default_factory=dict,
|
||||
json_schema_extra={"x-dynamic": True, "x-dependsOn": ["b"]},
|
||||
)
|
||||
b: dict[str, Any] = Field(
|
||||
default_factory=dict,
|
||||
json_schema_extra={"x-dynamic": True, "x-dependsOn": ["a"]},
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match="Cyclic control dependency"):
|
||||
|
||||
@widget(widget_type="cyclic-test-widget", name="Cyclic")
|
||||
class _Cyclic(Widget):
|
||||
controls_class = _CyclicControls
|
||||
|
||||
# The widget must not remain half-registered after the failure.
|
||||
assert registry.get("cyclic-test-widget") is None
|
||||
```
|
||||
|
||||
(Add `import pytest` at the top of the file if not already present — check
|
||||
first, since `test_registry.py` may already import it via other tests in
|
||||
this suite.)
|
||||
|
||||
- [x] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_registry.py -v -k cyclic`
|
||||
Expected: FAIL — the widget currently registers successfully (no eager
|
||||
schema build happens at registration), so no `ValueError` is raised, and the
|
||||
final `assert registry.get(...) is None` also fails since it *did* register.
|
||||
|
||||
- [x] **Step 3: Make registration eager, and roll back on failure**
|
||||
|
||||
In `superset/core/api/core_api_injection.py`, inside `inject_widget_implementations`'s
|
||||
`widget_impl`'s `decorator`, after `registry[key] = cls` and before `return cls`:
|
||||
|
||||
```python
|
||||
cls.widget_type = key
|
||||
cls.name = name
|
||||
cls.description = description or ""
|
||||
registry[key] = cls
|
||||
try:
|
||||
# Eagerly build the base (control_values=None) schema once, so
|
||||
# a cyclic x-dependsOn graph among this widget's dynamic
|
||||
# fields fails at import time -- get_control_schema's
|
||||
# toposort_or_raise call is what actually detects the cycle;
|
||||
# this just forces that check to run now instead of on the
|
||||
# widget's first real request.
|
||||
cls.get_control_schema(None, None)
|
||||
except Exception:
|
||||
registry.pop(key, None)
|
||||
raise
|
||||
return cls
|
||||
```
|
||||
|
||||
- [x] **Step 4: Run test to verify it passes**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_registry.py -v -k cyclic`
|
||||
Expected: PASS.
|
||||
|
||||
- [x] **Step 5: Run the full widgets + MCP suite**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ -v`
|
||||
Expected: same state as Task 3 Step 3 — Balloons tests still failing (Task 5
|
||||
fixes them), everything else passing, plus the new cyclic test passing.
|
||||
|
||||
- [x] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add superset/core/api/core_api_injection.py tests/unit_tests/widgets/test_registry.py
|
||||
git commit -m "feat(dashboard-v2): detect cyclic control dependencies at widget registration"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 5: Retrofit `Balloons` onto the enricher registry
|
||||
|
||||
**Files:**
|
||||
- Modify: `superset/widgets/builtin.py`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `Widget.enrichers` (Task 3).
|
||||
- Produces: nothing new — `Balloons`'s served schema must be byte-identical to before this whole plan started (golden-fixture proof, `test_builtin.py`/`test_registry.py` already assert this exactly).
|
||||
|
||||
- [x] **Step 1: Replace `Balloons.enrich_schema` with a registered enricher**
|
||||
|
||||
In `superset/widgets/builtin.py`, replace the `enrich_schema` classmethod with:
|
||||
|
||||
```python
|
||||
@staticmethod
|
||||
def _populate_series(
|
||||
schema: dict[str, Any],
|
||||
node: dict[str, Any],
|
||||
parsed: BaseModel | None,
|
||||
series: list[str],
|
||||
upstream: dict[str, Any],
|
||||
) -> None:
|
||||
# `node` is Customization.series's own fragment; `SeriesStyle` is a
|
||||
# sibling $defs entry, only reachable via the full `schema`.
|
||||
style_def = schema.get("$defs", {}).get("SeriesStyle")
|
||||
if style_def is None:
|
||||
return
|
||||
# The x-dependsOn: ["dataBinding"] gate (run_enrichers) only confirms
|
||||
# a dataBinding was parsed at all -- it can't express "dimensions is
|
||||
# non-empty" (a nested attribute) or "series is non-empty" (a runtime
|
||||
# parameter, not a field on parsed), so both stay checked here.
|
||||
dimensions = None
|
||||
if parsed is not None:
|
||||
data_binding = getattr(parsed, "data_binding", None)
|
||||
dimensions = getattr(data_binding, "dimensions", None)
|
||||
if not dimensions or not series:
|
||||
return
|
||||
# Dedupe (preserving order) and cap before doing per-series work, so an
|
||||
# oversized/duplicate list can't blow up CPU, memory, or response size.
|
||||
unique_series = list(dict.fromkeys(series))[: Balloons.MAX_SERIES]
|
||||
# Replace the open-ended map with one inlined, pre-colored style per series.
|
||||
node.pop("additionalProperties", None)
|
||||
properties: dict[str, Any] = {}
|
||||
for index, value in enumerate(unique_series):
|
||||
style = deepcopy(style_def)
|
||||
style["properties"]["color"]["default"] = Balloons.PALETTE[
|
||||
index % len(Balloons.PALETTE)
|
||||
]
|
||||
# Title each group with the series value so the control panel labels
|
||||
# it by series rather than by the shared model name ("SeriesStyle").
|
||||
style["title"] = value
|
||||
properties[value] = style
|
||||
node["properties"] = properties
|
||||
|
||||
enrichers: ClassVar[dict[str, EnricherFn]] = {"customize/series": _populate_series}
|
||||
```
|
||||
|
||||
Add the necessary imports at the top of `superset/widgets/builtin.py`:
|
||||
```python
|
||||
from typing import Any, ClassVar
|
||||
|
||||
from superset_core.widgets import EnricherFn, Widget, widget
|
||||
```
|
||||
(replacing the existing `from typing import Any` and `from superset_core.widgets import Widget, widget` lines).
|
||||
|
||||
- [x] **Step 2: Run the Balloons tests to verify they pass again**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/test_builtin.py -v`
|
||||
Expected: all PASS, unchanged assertions — proves the retrofit reproduces
|
||||
`enrich_schema`'s exact prior behavior.
|
||||
|
||||
- [x] **Step 3: Run the full widgets + MCP + semantic_layers suite**
|
||||
|
||||
Run: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ tests/unit_tests/semantic_layers/ -v`
|
||||
Expected: all PASS, including the composite-control-registry slice's golden-fixture
|
||||
tests for `metric-tile`/`balloons` (this plan must not regress those either).
|
||||
|
||||
- [x] **Step 4: mypy**
|
||||
|
||||
Run: `pre-commit run mypy --files superset-core/src/superset_core/widgets/enrichment.py superset-core/src/superset_core/widgets/base.py superset-core/src/superset_core/widgets/__init__.py superset-core/src/superset_core/semantic_layers/config.py superset/core/api/core_api_injection.py superset/widgets/builtin.py`
|
||||
Expected: no errors. Fix inline if any surface (the composite-control-registry
|
||||
slice hit one narrowing issue mypy flagged that `isinstance`+`issubclass` alone
|
||||
didn't resolve — bind to an explicitly-annotated local if something similar
|
||||
comes up here).
|
||||
|
||||
- [x] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add superset/widgets/builtin.py
|
||||
git commit -m "refactor(dashboard-v2): retrofit Balloons onto the enricher registry"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Final Verification
|
||||
|
||||
- [x] Run `pre-commit run --files <every file touched across all 5 tasks>` and fix anything it flags; re-run the affected tests after any auto-fix. Confirmed clean per-task (no auto-fixes needed on any of the 5 commits' files).
|
||||
- [x] Run the full backend unit suite once more: `venv/bin/python3 -m pytest tests/unit_tests/widgets/ tests/unit_tests/mcp_service/widgets/ tests/unit_tests/semantic_layers/ -v` — 442 passed.
|
||||
- [x] Confirm no frontend files were touched: `git status --porcelain -- superset-frontend/` should be empty. Confirmed empty.
|
||||
- [x] Reconcile the spec (`docs/superpowers/specs/2026-08-26-control-dependency-graph-design.md`) with anything discovered during implementation that diverged from the plan (`EnricherFn`'s signature, `check_dependencies`'s alias fix, the gate-vs-fine-grained-guard nuance) — same discipline as the composite-control-registry slice's post-implementation doc reconciliation. Done: this plan's design matched what shipped exactly (all three corrections were caught while writing the plan, before implementation, rather than during it), so the spec only needed its Design section rewritten to match, not a behavior change.
|
||||
@@ -1,170 +0,0 @@
|
||||
# Dataset-Aware Widget Controls — Design
|
||||
|
||||
**Status:** Approved for planning
|
||||
**Branch:** `enxdev/poc/dashboard-v2-editing-ui-flow`
|
||||
**Date:** 2026-08-20
|
||||
|
||||
## Problem
|
||||
|
||||
Dashboard V2's widget control panel is schema-driven end to end: a backend
|
||||
`Widget.controls_class` (a Pydantic model) becomes a JSON Schema served at
|
||||
`/api/v1/widgets/type/<widget_type>/control-schema`, rendered generically by
|
||||
JsonForms in the Inspector's Form tab
|
||||
([SchemaControlPanel.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx)),
|
||||
and written back through `provider.updateProps` — the same call an
|
||||
AI/MCP-driven edit would use.
|
||||
|
||||
That machinery renders every field as a generic input keyed only to its JSON
|
||||
type (string, number, array). It has no concept that a field's value is a
|
||||
*reference into the widget's bound dataset* — a column name, a metric — so
|
||||
fields like `BalloonsControls.dimensions` or `color_dimension`
|
||||
([superset/widgets/controls.py](../../../superset/widgets/controls.py)) render
|
||||
as bare text inputs, and `metrics` is forced into a raw-JSON editor
|
||||
(`x-control: "code"`) purely because its entries are heterogeneous, not
|
||||
because a real picker is impossible.
|
||||
|
||||
Reference UIs (Looker Studio / Power BI-style panels) tie each such field to
|
||||
the dataset's column metadata: a type icon (ABC for string, 1.2 for numeric),
|
||||
an ordered add/remove/drag list for multi-value fields (X axis, Y axis), and
|
||||
per-category swatches for a chosen dimension's distinct values. The last of
|
||||
these already exists in spirit — `BalloonsControls.Customization.series` uses
|
||||
`x-dynamic` + `x-key-source` to enrich per-series color controls once a
|
||||
grouping dimension is set — but the column/metric-reference fields themselves
|
||||
have no equivalent.
|
||||
|
||||
SIP-182 (Semantic Layer support, apache/superset#35003) establishes the same
|
||||
principle one layer up — an `Explorable` protocol with typed columns driving
|
||||
a "reactive metric/dimension compatibility matrix" for semantic-layer
|
||||
*connections*. It does not specify per-field UI rendering and this design
|
||||
does not depend on it; the vocabulary (column type gates which control
|
||||
renders) is the only thing borrowed.
|
||||
|
||||
## Scope
|
||||
|
||||
In scope: making fields *within* an existing widget's control schema
|
||||
type-aware (column pickers, metric pickers, ordered multi-value lists).
|
||||
|
||||
Out of scope (deferred): swapping an already-placed widget's type in place
|
||||
(the "Visualization" dropdown in the reference screenshots, e.g. Bar → Table).
|
||||
That is a separate feature — changing `node.type` on a live node — and is not
|
||||
addressed here.
|
||||
|
||||
## Approach
|
||||
|
||||
Two options were considered:
|
||||
|
||||
- **Extend the existing `x-control` vocabulary** (chosen) — add new
|
||||
`x-control` values alongside the existing `code`/`color` entries in
|
||||
[schemaControlRenderers.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx),
|
||||
following the exact pattern already established there: a JsonForms tester
|
||||
matches the `x-control` value, the renderer fetches whatever data it needs
|
||||
and calls `handleChange`. The backend control model stays the single source
|
||||
of truth for which fields are column/metric references and which types
|
||||
they accept, declared declaratively via `json_schema_extra` — the same way
|
||||
`x-control: code` and `x-dynamic` are declared today.
|
||||
- **Embed Superset's classic Explore controls** (`MetricsControl`,
|
||||
`DndColumnSelect`) directly — rejected. Those components are coupled to the
|
||||
Explore Redux slice; using them in Dashboard V2 would mean faking
|
||||
Explore-shaped state to borrow a widget, which works against the reason the
|
||||
schema-driven JsonForms system exists in the first place, and reintroduces
|
||||
exactly the legacy-pattern coupling the frontend modernization effort is
|
||||
moving away from.
|
||||
|
||||
## Design
|
||||
|
||||
### New `x-control` values
|
||||
|
||||
Declared via `json_schema_extra` on a Pydantic field, identically to how
|
||||
`x-control: "code"` is declared today:
|
||||
|
||||
- `x-control: "column"` — single column reference. Optional
|
||||
`x-column-types: ["numeric" | "temporal" | "string"]` restricts which
|
||||
dataset columns are selectable; omitted means any column.
|
||||
- `x-control: "column-multi"` — ordered list of column references (for
|
||||
`dimensions`-shaped fields): add, remove, and reorder (drag).
|
||||
- `x-control: "metric"` — single metric reference (a saved metric name, or an
|
||||
ad-hoc aggregate expressible as one).
|
||||
- `x-control: "metric-multi"` — ordered list of metric references.
|
||||
|
||||
`metrics` fields keep their existing raw-JSON (`x-control: "code"`) path as a
|
||||
fallback for anything not expressible through the picker (this design does
|
||||
not change or remove that path — see Error handling).
|
||||
|
||||
### Dataset metadata plumbing
|
||||
|
||||
A new `fetchDatasetColumns(datasetId)` helper is added to
|
||||
`superset-frontend/src/core/dashboard/`, parallel to the existing
|
||||
`fetchQueryData` in
|
||||
[chartData.ts](../../../superset-frontend/src/core/dashboard/chartData.ts).
|
||||
It calls the existing `/api/v1/dataset/<id>` REST endpoint (already used by
|
||||
V1 Explore; no backend endpoint work is needed for this) and returns
|
||||
`{ columns: [{ name, type, isTemporal, isNumeric }], metrics: [{ metricName,
|
||||
verboseName }] }`. Results are cached per `datasetId` for the lifetime of the
|
||||
Inspector session, mirroring the fetch-once-and-cache pattern
|
||||
`schemaControlledWidgets.ts` already uses for widget types.
|
||||
|
||||
### New renderers
|
||||
|
||||
Added to `schemaControlRenderers.tsx` beside `CodeControl`/`ColorControl`:
|
||||
|
||||
- `ColumnControl` / `ColumnMultiControl` — fetch via
|
||||
`fetchDatasetColumns`, filter by the field's `x-column-types`, render an
|
||||
antd `Select` (single) or an ordered add/remove/drag list (multi). Each
|
||||
option is labeled with `ColumnTypeLabel` from `@superset-ui/chart-controls`
|
||||
(confirmed presentational, no Redux coupling — safe to reuse as-is).
|
||||
- `MetricControl` / `MetricMultiControl` — same fetch, options are the
|
||||
dataset's saved metrics (`MetricOption` from the same package for the
|
||||
icon/label treatment), plus an escape hatch that drops to the existing
|
||||
`CodeControl` for a value not expressible as a saved metric.
|
||||
- The two multi-controls share one small drag-list primitive implemented
|
||||
locally (plain `onDragStart`/`onDrop`) — no new dependency, since nothing
|
||||
in this layer currently pulls in a drag library.
|
||||
|
||||
### Data flow
|
||||
|
||||
Unchanged at the edges: every control still reads and writes `node.props`
|
||||
through `provider.updateProps`, and the schema itself still comes from the
|
||||
existing `/api/v1/widgets/type/<t>/control-schema` endpoint. The only new
|
||||
network call is the dataset-columns/metrics fetch, and it is triggered
|
||||
exactly the way `x-dynamic` already triggers `SchemaControlPanel`'s
|
||||
`loadSeries` call today — off the presence of a field carrying the new
|
||||
`x-control` value in the already-fetched schema, not off a new schema
|
||||
capability flag.
|
||||
|
||||
Backend changes are additive and narrow, and land on the shared `DataBinding`
|
||||
model so every widget that embeds it (`MetricTileControls`,
|
||||
`AgGridTableControls`, `BalloonsControls`, `EchartsControls`) picks them up
|
||||
uniformly rather than needing per-widget duplication:
|
||||
`DataBinding.dimensions` gets `x-control: "column-multi"`, and
|
||||
`DataBinding.metrics` gets `x-control: "metric-multi"` in place of its
|
||||
current `x-control: "code"`. `BalloonsControls.color_dimension` separately
|
||||
gets `x-control: "column"`. `EchartsControls.echarts_options` (a distinct
|
||||
field, not `metrics`) is unaffected and stays free-form JSON, since its
|
||||
`$bind` markers are not a fixed set of fields.
|
||||
|
||||
### Error handling
|
||||
|
||||
- A failed `fetchDatasetColumns` call fails open to a plain text/code input
|
||||
for the affected field, rather than blocking the panel — the same fail-open
|
||||
behavior `useSchemaControlledWidgetTypes` already applies to the widget
|
||||
registry fetch.
|
||||
- A previously-picked column or metric that no longer exists on the dataset
|
||||
is shown selected but visually flagged (not silently cleared), consistent
|
||||
with `BalloonsControls`'s existing philosophy of surfacing invalid state
|
||||
through `validate_control_values` rather than swallowing it.
|
||||
|
||||
### Testing
|
||||
|
||||
- Backend: schema-shape assertions in the style of
|
||||
`tests/unit_tests/widgets/test_registry.py`, asserting each newly annotated
|
||||
field serves the expected `x-control`/`x-column-types` extras.
|
||||
- Frontend: one Jest + React Testing Library test per new renderer (mirroring
|
||||
`SchemaControlPanel.test.tsx`), mocking `fetchDatasetColumns`; one
|
||||
integration-style test per control confirming a pick round-trips through
|
||||
`provider.updateProps` into `node.props`.
|
||||
|
||||
## Open questions
|
||||
|
||||
None outstanding — visualization-type swapping is explicitly deferred (see
|
||||
Scope), and the reuse-vs-rebuild fork was resolved in favor of extending
|
||||
`x-control` during design review.
|
||||
@@ -1,110 +0,0 @@
|
||||
# Dashboard V2 Mockup Alignment — Design
|
||||
|
||||
**Status:** Approved for planning (PR 1 only; PRs 2-4 scoped, not yet detailed)
|
||||
**Branch:** `enxdev/poc/dashboard-v2-editing-ui-flow`
|
||||
**Date:** 2026-08-25
|
||||
|
||||
## Problem
|
||||
|
||||
A reference mockup ("Dashboard V2 – Dynamic Panels") shows a target look for
|
||||
the Dashboard V2 prototype: a left rail with four contextual tabs (Data,
|
||||
Outline, Building Blocks, Properties), a card-styled canvas, and a docked
|
||||
Assistant panel. Comparing it against the current implementation in
|
||||
[DashboardBuilderV2](../../../superset-frontend/src/pages/DashboardBuilderV2)
|
||||
surfaces four independent, real gaps:
|
||||
|
||||
1. The left rail ([EditorPanel.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx))
|
||||
has three tabs (Widgets, Properties, Outline) — no standalone tab for
|
||||
browsing datasets independent of a selected widget.
|
||||
2. Setting a widget's dataset today means typing a raw numeric
|
||||
`dataBinding.datasetId` — [SchemaControlPanel.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx)
|
||||
and [schemaControlRenderers.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx)
|
||||
have picker renderers for columns/metrics *within* an already-bound
|
||||
dataset, but nothing for choosing the dataset itself. The mockup's
|
||||
Properties tab also shows a "Show filters" link next to the Dataset field
|
||||
that has no equivalent today.
|
||||
3. Canvas widgets render plainly; the mockup gives each a card with a title
|
||||
and a "⋮" overflow menu.
|
||||
4. There is no per-widget filter configuration concept in Dashboard V2 at
|
||||
all (confirmed by grep — the only "filter" hits in this code are unrelated
|
||||
`.filter()` array calls).
|
||||
|
||||
Explicitly **not** in scope: the Assistant/chat panel
|
||||
([ChatHost.tsx](../../../superset-frontend/src/core/chat/ChatHost.tsx)) stays
|
||||
exactly as it is. The mockup's docked-assistant styling is not being pursued —
|
||||
the user asked directly not to touch the chatbot.
|
||||
|
||||
## Decomposition
|
||||
|
||||
Four independent PRs, in build order:
|
||||
|
||||
1. **Data tab (this design)** — a new left-rail tab for browsing datasets,
|
||||
shipped first as a static/mock UI shell (see Scope below).
|
||||
2. **Properties panel polish** — a real dataset picker for
|
||||
`dataBinding.datasetId` (replacing the raw numeric input) plus the
|
||||
mockup's "Show filters" link. Will revisit reusing
|
||||
[DatasetSelect.tsx](../../../superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect.tsx)'s
|
||||
`loadDatasetOptions` (or extracting it) once this PR needs a real dataset
|
||||
search — deferred, not decided, by PR 1's mock-only scope.
|
||||
3. **Canvas card styling** — visual-only pass: title + "⋮" menu chrome on
|
||||
canvas widgets, no new state.
|
||||
4. **Per-widget filters** — the functionality behind PR 2's "Show filters"
|
||||
link.
|
||||
|
||||
Each gets its own brainstorm/design/plan when its turn comes; only PR 1 is
|
||||
detailed below.
|
||||
|
||||
## PR 1: Data tab — Scope
|
||||
|
||||
Ship the left-rail shape and the tab's visual shell now; wire it to real
|
||||
dataset data in a later PR. Concretely:
|
||||
|
||||
- Rename the existing "Widgets" tab to "Building Blocks" (label change only —
|
||||
it already holds the Structure/Content shelves the mockup's Building
|
||||
Blocks tab shows).
|
||||
- Add a new "Data" tab, first in tab order, ahead of Building Blocks.
|
||||
- The Data tab renders a new `DataPanel.tsx` component:
|
||||
- A search input, filtering a **hardcoded** in-memory list of mock
|
||||
datasets client-side (no network calls this PR).
|
||||
- Each mock dataset row is expandable (chevron, local component state) to
|
||||
reveal a **hardcoded** list of mock columns, each with a `ColumnTypeLabel`
|
||||
icon (`@superset-ui/chart-controls`, already used the same way in
|
||||
`schemaControlRenderers.tsx`) so the visual language matches the
|
||||
Properties tab's existing column pickers.
|
||||
- Rows are not clickable/draggable — no `dataBinding` wiring, no
|
||||
interaction with canvas or selection state. Purely a browsing shell.
|
||||
- No new API calls, no new backend work, no changes to `DashboardProvider`
|
||||
or any widget's props.
|
||||
|
||||
### Out of scope for PR 1 (explicit, not silent)
|
||||
|
||||
- Real dataset search/listing (deferred to PR 2, alongside the Properties
|
||||
dataset picker — same underlying need, one decision).
|
||||
- Real column/metric metadata per dataset (deferred with the above).
|
||||
- Clicking a dataset to bind it to a selected widget.
|
||||
- Dragging a column onto the canvas to create a chart.
|
||||
|
||||
### Components touched
|
||||
|
||||
- `EditorPanel.tsx`: tab list — rename `widgets` tab label, add `data` tab,
|
||||
reorder so Data is first.
|
||||
- New `DataPanel.tsx` (sibling to `Palette.tsx`/`Outline.tsx`): the mock
|
||||
search + expandable list.
|
||||
- New `DataPanel.test.tsx`, following the existing `Palette.test.tsx` /
|
||||
`Outline.test.tsx` pattern.
|
||||
|
||||
### Testing
|
||||
|
||||
Jest + RTL: tab renders in the right position with the right label; search
|
||||
filters the mock list; a row expands/collapses to show its mock columns with
|
||||
type icons. No integration/E2E needed — nothing here talks to the backend.
|
||||
|
||||
## Spec self-review
|
||||
|
||||
- No placeholders or TBDs remain — PR 1's scope is fully concrete; PRs 2-4
|
||||
are intentionally one-line pointers, not commitments to a specific design.
|
||||
- Consistent: the "mock only" boundary is stated once and every "out of
|
||||
scope" item traces back to it.
|
||||
- Focused: this document covers one implementable PR in full plus a map of
|
||||
what comes after it, which is what was asked (split the work into
|
||||
several PRs, spec the first one).
|
||||
@@ -1,410 +0,0 @@
|
||||
# Composite Control Registry — Design
|
||||
|
||||
**Status:** Approved for planning
|
||||
**Branch:** `enxdev/poc/dashboard-v2-editing-ui-flow`
|
||||
**Date:** 2026-08-26
|
||||
|
||||
## Problem
|
||||
|
||||
Dashboard V2's widget control panels are schema-driven end to end: a backend
|
||||
`Widget.controls_class` (a Pydantic model) becomes a JSON Schema served at
|
||||
`/api/v1/widgets/type/<widget_type>/control-schema`, rendered generically by
|
||||
JsonForms in the Inspector's Form tab
|
||||
([SchemaControlPanel.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx))
|
||||
and consumed identically by MCP tools
|
||||
([get_widget_control_schema.py](../../../superset/mcp_service/widgets/tool/get_widget_control_schema.py)).
|
||||
|
||||
Today, reuse across widget control models is achieved only by importing a
|
||||
`BaseModel` subclass and nesting it as a field — e.g.
|
||||
[`DataBinding`](../../../superset/widgets/controls.py) is imported and nested
|
||||
under `data_binding` in `MetricTileControls`, `AgGridTableControls`,
|
||||
`BalloonsControls`, and `EchartsControls`. This works, but:
|
||||
|
||||
- There's no way to reuse a *field-level* building block (like the `metrics`
|
||||
picker) without pulling in everything else nested alongside it in whatever
|
||||
model first defined it.
|
||||
- There's no registry of what reusable building blocks exist, so an extension
|
||||
author (or an agent) has no way to discover "what composable controls does
|
||||
Superset ship" short of reading `controls.py` source.
|
||||
|
||||
The immediate driver is `DataBinding.metrics` — a `list[Any]` field tagged
|
||||
`x-control: "metric-multi"` — which preserves today's existing behavior
|
||||
(saved-metric names, ad-hoc SIMPLE aggregates, JSON fallback for anything
|
||||
richer) but is locked inside `DataBinding` with no way to reuse just the
|
||||
metric-picking piece in a widget that doesn't want the rest of `DataBinding`'s
|
||||
shape. It does not give a headless consumer a structured description of what
|
||||
a valid ad-hoc metric object looks like (that's still `Any` under the hood);
|
||||
typed ad-hoc metric schemas are a possible future improvement, not part of
|
||||
this extraction.
|
||||
|
||||
## Scope
|
||||
|
||||
In scope: a general mechanism for defining and registering a **composite
|
||||
control** (a reusable Pydantic field-bearing mixin), and using it to extract
|
||||
`metrics` into a standalone `MetricControl` as the first (and for this slice,
|
||||
only) instance.
|
||||
|
||||
Explicitly out of scope, deferred to follow-up specs:
|
||||
- A dependency graph between controls with circular-dependency detection
|
||||
(today's `x-dependsOn` / `check_dependencies` only checks truthiness of
|
||||
named attributes — no graph, no ordering, no cycle detection).
|
||||
- A formal contract for how provided values, defaults, and control-generated
|
||||
values (like `BalloonsControls.enrich_schema`'s per-series population) are
|
||||
propagated and validated.
|
||||
- Any new metric capability (SQL-expression ad-hoc metrics, etc.) — this is a
|
||||
refactor for reusability, not a capability change. `MetricControl`'s
|
||||
`metrics` field is moved verbatim from `DataBinding`, not extended.
|
||||
- Any frontend change. The rendering path
|
||||
(`MetricMultiControl` / `ReferenceMultiList` in
|
||||
[schemaControlRenderers.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/schemaControlRenderers.tsx))
|
||||
keys off the `x-control` value and field name, both unchanged by this
|
||||
refactor.
|
||||
- Composing *multiple* composite controls into one model (multiple
|
||||
inheritance across two or more registered mixins). This slice only
|
||||
exercises one composite (`MetricControl`) composed into one widget model
|
||||
(`DataBinding`) via single inheritance. Field-name collision detection,
|
||||
`model_config` merge rules, and validator-ordering across composites are
|
||||
real open questions once a second composite exists to motivate them —
|
||||
solving them speculatively now, with only one composite in existence,
|
||||
would be guessing at a contract with no real case to validate it against.
|
||||
Composing two registered composites together is unsupported until a
|
||||
follow-up spec defines that contract.
|
||||
|
||||
This slice establishes reusable schema composition and discovery. It does
|
||||
not, on its own, solve dynamic headless control behavior (dependent-control
|
||||
propagation, cycle detection) — that remains fully deferred, below.
|
||||
|
||||
## Approach
|
||||
|
||||
**Mixin composition, not nested-object composition.** `DataBinding.metrics` is
|
||||
read as a *flat* field in several places — `SchemaControlPanel.tsx:122`
|
||||
(`binding.metrics?.length`), `chartData.ts:67` (`binding.metrics`), and the
|
||||
`DataBindingSpec` TypeScript type — so wrapping `metrics` in a new nested
|
||||
object (e.g. `dataBinding.metricControl.metrics`) would be a breaking
|
||||
schema-shape change requiring frontend updates. Pydantic supports composing a
|
||||
model by inheriting from a `BaseModel` subclass and flattens the parent's
|
||||
fields into the subclass's schema with no nesting, which avoids this
|
||||
entirely: `DataBinding` becomes
|
||||
```python
|
||||
class DataBinding(MetricControl):
|
||||
... # dataset_id, dimensions, row_limit — metrics no longer declared here
|
||||
```
|
||||
(single inheritance from `MetricControl`, which itself already subclasses
|
||||
`BaseModel` — inheriting from both `BaseModel` and `MetricControl` at once
|
||||
raises `TypeError: Cannot create a consistent method resolution order`,
|
||||
confirmed against pydantic directly; `MetricControl` alone is both necessary
|
||||
and sufficient as the base) and `model_json_schema()`'s property *set* and
|
||||
`x-control` extras for `DataBinding` are unchanged.
|
||||
|
||||
**Field order needs an explicit override — inheritance order is not enough.**
|
||||
Pydantic always collects a base class's fields into `model_fields` ahead of
|
||||
the subclass's own fields, regardless of where the subclass redeclares them —
|
||||
confirmed directly: a subclass inheriting `metrics` from `MetricControl` gets
|
||||
field order `metrics, dataset_id, dimensions, row_limit`, not today's
|
||||
`dataset_id, metrics, dimensions, row_limit`, even when `metrics` is
|
||||
redeclared at its original position in the subclass body. This isn't
|
||||
cosmetic: `build_configuration_schema` restores model-field order specifically
|
||||
so JsonForms renders fields in the author's intended order, so an
|
||||
uncorrected reorder would visibly move the Metrics control ahead of the
|
||||
Dataset picker in the Inspector form.
|
||||
|
||||
So `build_configuration_schema` gains an explicit, opt-in override: a model
|
||||
may declare `field_order: ClassVar[list[str]]` naming its properties (by
|
||||
alias) in the order they should render; when present,
|
||||
`build_configuration_schema` uses that list instead of deriving order from
|
||||
`model_fields`, and raises `ValueError` if it isn't an exact permutation of
|
||||
the schema's property names (a declared order that's missing or misnames a
|
||||
property is a bug worth failing loudly on, not silently dropping fields from
|
||||
the rendered form). When absent, behavior is unchanged from today
|
||||
(derive order from `model_fields`) — every other consumer of
|
||||
`build_configuration_schema` (there are none today outside
|
||||
`Widget.get_control_schema`, confirmed by repo-wide search, so this is
|
||||
low-risk) keeps working exactly as before. `DataBinding` declares
|
||||
```python
|
||||
field_order: ClassVar[list[str]] = ["datasetId", "metrics", "dimensions", "rowLimit"]
|
||||
```
|
||||
— deliberately preserving today's exact order, so this remains a
|
||||
strictly non-breaking refactor and not an incidental UX change.
|
||||
|
||||
**Registry for discovery, not for composition.** Composing a composite
|
||||
control into a widget is plain Python inheritance/import — no runtime lookup
|
||||
is involved. A `@composite_control(name, title, description)` decorator
|
||||
(mirroring the existing `@widget` → `superset/widgets/registry.py` pattern)
|
||||
registers each composite class, *with* its `name`/`title`/`description`, into
|
||||
a module-level store purely so tooling (docs generation, an MCP "list
|
||||
composite controls" tool, future extension-author documentation) can
|
||||
enumerate what's available without grepping source — discovery needs the
|
||||
metadata, not just the class object. This mirrors how `@widget` registration
|
||||
and widget discovery already work in this codebase, so it's a familiar idiom
|
||||
rather than a new one.
|
||||
|
||||
Registration is a decorator side effect, exactly like `@widget`: a composite
|
||||
control is only in the registry once its defining module has been imported
|
||||
(see `superset/widgets/builtin.py`'s "Importing this registers them"). This
|
||||
applies equally to extension-defined composites — an extension's composite
|
||||
won't appear in MCP/docs discovery until something imports that extension's
|
||||
module, the same constraint `inject_widget_implementations` already handles
|
||||
for widgets.
|
||||
|
||||
Two alternatives were considered and rejected:
|
||||
- **Reusable `Annotated` field type** (e.g. `MetricList = Annotated[list[Any],
|
||||
Field(...)]`) — avoids nesting equally well, but isn't a `BaseModel`
|
||||
subclass, so it can't hold validators or be registered/discovered the same
|
||||
way `Widget` subclasses are; inconsistent with the codebase's established
|
||||
class+registry idiom.
|
||||
- **Registry of field-factories** (a decorator on a function returning
|
||||
`(type, FieldInfo)`, looked up by name at model-definition time) — more
|
||||
machinery than this slice needs; the "simple import" requirement is better
|
||||
served by importing a class and inheriting from it directly.
|
||||
|
||||
## Design
|
||||
|
||||
### `superset_core/semantic_layers/config.py` (changed)
|
||||
|
||||
`build_configuration_schema` gains an explicit field-order override —
|
||||
**applied recursively to every nested model that lands in `$defs`, not just
|
||||
`config_class` itself.** This turned out to be required, not optional:
|
||||
`DataBinding` is never the top-level `config_class` passed to
|
||||
`build_configuration_schema` — it only ever appears nested (e.g.
|
||||
`MetricTileControls.data_binding: DataBinding`) — and Pydantic generates each
|
||||
nested model's own `$defs` entry using that model's own `model_fields` order,
|
||||
independent of anything done to the outer schema. A version of this override
|
||||
that only reordered the top-level `schema["properties"]` was verified (by
|
||||
actually running it against `metric-tile`'s served schema) to leave
|
||||
`$defs.DataBinding` unreordered — `metrics` still rendered ahead of
|
||||
`datasetId`. The fix walks every `BaseModel` reachable from `config_class`
|
||||
(through its fields, including through generics like `list[...]`) and applies
|
||||
the same declared-or-derived field-order logic to each one that has a
|
||||
corresponding `$defs` entry:
|
||||
|
||||
```python
|
||||
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", {}))):
|
||||
raise ValueError(
|
||||
f"{model_cls.__name__}.field_order must be a permutation of its "
|
||||
f"schema properties; declared={sorted(declared)} actual={sorted(actual)}"
|
||||
)
|
||||
return declared_order
|
||||
|
||||
|
||||
def _reorder(schema_node: dict[str, Any], field_order: list[str]) -> None:
|
||||
"""Reorder ``schema_node``'s ``properties`` (and, for determinism,
|
||||
``required``) to match ``field_order``. Mutates in place."""
|
||||
if (properties := schema_node.get("properties")) is not None:
|
||||
schema_node["properties"] = {
|
||||
key: properties[key] for key in field_order if key in properties
|
||||
}
|
||||
if (required := schema_node.get("required")) is not None:
|
||||
index = {key: position for position, key in enumerate(field_order)}
|
||||
schema_node["required"] = sorted(
|
||||
required, key=lambda key: index.get(key, len(field_order))
|
||||
)
|
||||
|
||||
|
||||
def build_configuration_schema(
|
||||
config_class: type[BaseModel],
|
||||
configuration: BaseModel | None = None,
|
||||
) -> dict[str, Any]:
|
||||
schema = config_class.model_json_schema()
|
||||
|
||||
_reorder(schema, _resolve_field_order(config_class, schema))
|
||||
|
||||
defs = schema.get("$defs", {})
|
||||
for nested_cls in _iter_nested_models(config_class, seen=set()):
|
||||
if nested_cls is config_class:
|
||||
continue
|
||||
def_entry = defs.get(nested_cls.__name__)
|
||||
if def_entry is None:
|
||||
continue
|
||||
_reorder(def_entry, _resolve_field_order(nested_cls, def_entry))
|
||||
|
||||
if configuration is None:
|
||||
for prop_schema in schema["properties"].values():
|
||||
if prop_schema.get("x-dynamic"):
|
||||
prop_schema["enum"] = []
|
||||
|
||||
return schema
|
||||
```
|
||||
|
||||
Reordering `required` alongside `properties` is not strictly necessary for
|
||||
JsonForms rendering (`required`'s array order carries no rendering meaning),
|
||||
but it's included for determinism and so the golden-fixture test can assert
|
||||
an exact, stable value rather than a set comparison.
|
||||
|
||||
Verified end to end against the real widget registry (not just the isolated
|
||||
helper): `registry.get("metric-tile").get_control_schema(None, None)` and the
|
||||
MCP `get_widget_control_schema` path (which prunes mandatory nested objects
|
||||
inline rather than leaving a `$ref`, a different code path through
|
||||
`schema_tools.py`) both now serve `$defs.DataBinding`/the inlined
|
||||
`dataBinding` in exactly `datasetId, metrics, dimensions, rowLimit` — byte-
|
||||
identical to the pre-refactor capture.
|
||||
|
||||
`field_order` is looked up with `getattr`, not a required base-class field, so
|
||||
every existing model without it is unaffected — this is additive, not a
|
||||
signature change.
|
||||
|
||||
### `superset_core/widgets/composites.py` (new)
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class CompositeControlInfo:
|
||||
name: str
|
||||
title: str
|
||||
description: str
|
||||
model: type[BaseModel]
|
||||
|
||||
|
||||
_registry: dict[str, CompositeControlInfo] = {}
|
||||
|
||||
|
||||
def composite_control(
|
||||
name: str, title: str, description: str
|
||||
) -> Callable[[type[BaseModel]], type[BaseModel]]:
|
||||
"""Register a reusable Pydantic mixin as a discoverable composite control.
|
||||
|
||||
Composing one into a widget's controls_class is plain inheritance —
|
||||
this decorator only makes the class discoverable via
|
||||
`list_composite_controls()`."""
|
||||
def decorator(cls: type[BaseModel]) -> type[BaseModel]:
|
||||
if name in _registry:
|
||||
raise ValueError(f"composite control {name!r} already registered")
|
||||
_registry[name] = CompositeControlInfo(name, title, description, cls)
|
||||
return cls
|
||||
return decorator
|
||||
|
||||
|
||||
def list_composite_controls() -> Mapping[str, CompositeControlInfo]:
|
||||
"""Read-only view of registered composite controls, for docs/MCP
|
||||
discovery. Extension-defined composites appear only once their
|
||||
defining module has been imported (decorator side effect, same as
|
||||
`@widget`)."""
|
||||
return MappingProxyType(_registry)
|
||||
|
||||
|
||||
@composite_control(
|
||||
name="metric",
|
||||
title="Metrics",
|
||||
description="Reusable metric-list field (saved-metric names or ad-hoc "
|
||||
"SIMPLE aggregates).",
|
||||
)
|
||||
class MetricControl(BaseModel):
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
metrics: list[Any] = Field(
|
||||
title="Metrics",
|
||||
description="...", # moved verbatim from DataBinding
|
||||
json_schema_extra={"x-control": "metric-multi", "x-language": "json"},
|
||||
)
|
||||
```
|
||||
|
||||
This lives in `superset_core` (not `superset/widgets/`) because it's meant for
|
||||
extension authors composing their own widget control models, the same
|
||||
audience `superset_core.widgets.Widget`/`@widget` already serve — not just
|
||||
Superset's own built-ins. `superset_core/widgets/__init__.py` re-exports
|
||||
`MetricControl`, `composite_control`, and `list_composite_controls` using the
|
||||
codebase's existing redundant-alias re-export idiom (`X as X`, e.g.
|
||||
`from superset_core.widgets.composites import MetricControl as MetricControl`),
|
||||
matching how `Widget`/`@widget` are already exposed, so consumers write
|
||||
`from superset_core.widgets import MetricControl` rather than reaching into
|
||||
the `composites` submodule directly. The underlying `_registry` dict is not
|
||||
exported; `list_composite_controls()` is the public discovery surface, so
|
||||
extension authors can't accidentally mutate the shared store.
|
||||
|
||||
### `superset/widgets/controls.py` (changed)
|
||||
|
||||
`DataBinding` drops its inline `metrics: list[Any] = Field(...)` declaration
|
||||
and instead:
|
||||
```python
|
||||
from typing import ClassVar
|
||||
|
||||
from superset_core.widgets import MetricControl
|
||||
|
||||
class DataBinding(MetricControl):
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
field_order: ClassVar[list[str]] = ["datasetId", "metrics", "dimensions", "rowLimit"]
|
||||
|
||||
dataset_id: int = Field(alias="datasetId", ...)
|
||||
dimensions: list[str] = Field(default_factory=list, ...)
|
||||
row_limit: int = Field(default=1000, alias="rowLimit", ...)
|
||||
```
|
||||
|
||||
No other file in `superset/widgets/` or `superset-frontend/` changes.
|
||||
|
||||
## Testing
|
||||
|
||||
- **Schema-identity test**: a golden fixture — the literal JSON Schema dict
|
||||
for `DataBinding`, captured from `main` before this refactor lands — checked
|
||||
into the test file and asserted equal (properties, order, `required`,
|
||||
defaults, aliases, and `x-control`/`x-language` extras on `metrics`)
|
||||
against the post-refactor output. "Before vs. after in the same PR" isn't
|
||||
enough on its own since both sides would be written by the same change;
|
||||
the frozen fixture is what makes the comparison meaningful. Assert this at
|
||||
the actual API/MCP boundary — the served
|
||||
`/api/v1/widgets/type/metric-tile/control-schema` response and
|
||||
`get_widget_control_schema`'s output — not just the raw
|
||||
`model_json_schema()` call, so a regression introduced by
|
||||
`schema_tools.py`'s progressive-disclosure layer would also be caught, not
|
||||
just one in the raw Pydantic schema.
|
||||
- **Registry unit tests**: registering a composite control, duplicate-name
|
||||
registration raises, `MetricControl` is present in
|
||||
`list_composite_controls()["metric"]` with the expected `title`/
|
||||
`description`, and the returned mapping is not mutable by callers.
|
||||
- **`build_configuration_schema` field-order tests**: a model with no
|
||||
`field_order` behaves exactly as today (regression coverage for the
|
||||
existing behavior this change extends); a model with `field_order` set
|
||||
gets properties in exactly that order; a model with `field_order` missing
|
||||
or misnaming a property raises `ValueError` naming the mismatch; a model
|
||||
with `field_order` that only ever appears nested inside another model (not
|
||||
as the top-level `config_class`) still gets reordered in `$defs` — this
|
||||
last case is what actually exercises the recursive walk, and is the case
|
||||
that would have caught the original top-level-only version as wrong.
|
||||
Directly exercises the mechanism that makes the `DataBinding` order fix
|
||||
correct, independent of `DataBinding` itself.
|
||||
- **Existing widget tests** (`MetricTile`, `AgGridTable`, `Balloons`,
|
||||
`Echarts`, and their control-schema API/MCP tests) run unchanged — no
|
||||
fixture or assertion should need updating, since the served schema doesn't
|
||||
move.
|
||||
- No frontend test changes expected.
|
||||
|
||||
## Follow-ups (separate specs)
|
||||
|
||||
- Dependency graph between controls: propagation ordering when one control's
|
||||
change should update another, plus circular-dependency detection. Builds on
|
||||
`x-dependsOn` but needs an actual graph, not per-field truthiness checks.
|
||||
- A propagation/validation contract: how a provided value, a schema default,
|
||||
and a control-generated value (`enrich_schema`-style) interact and validate
|
||||
against each other.
|
||||
- Whether other existing nested models (`Customization`, or future ones)
|
||||
should be retrofitted as registered composite controls, once there's a
|
||||
second real consumer beyond `MetricControl`.
|
||||
@@ -1,288 +0,0 @@
|
||||
# Control Dependency Graph — Design
|
||||
|
||||
**Status:** Approved for planning
|
||||
**Branch:** `enxdev/poc/dashboard-v2-editing-ui-flow`
|
||||
**Date:** 2026-08-26
|
||||
|
||||
## Problem
|
||||
|
||||
Dashboard V2's control schemas support a single dynamic field today:
|
||||
`BalloonsControls.Customization.series`
|
||||
([superset/widgets/controls.py](../../../superset/widgets/controls.py)),
|
||||
tagged `x-dynamic: true` and `x-dependsOn: ["dataBinding"]`. It's populated by
|
||||
`Balloons.enrich_schema`
|
||||
([superset/widgets/builtin.py](../../../superset/widgets/builtin.py)), a
|
||||
single classmethod hook every `Widget` subclass may override
|
||||
([superset_core/widgets/base.py](../../../superset-core/src/superset_core/widgets/base.py)).
|
||||
That hook hand-checks its own preconditions (`if not dimensions or not
|
||||
series: return`) rather than using the schema's declared `x-dependsOn`.
|
||||
|
||||
`check_dependencies`
|
||||
([superset_core/semantic_layers/config.py](../../../superset-core/src/superset_core/semantic_layers/config.py))
|
||||
already exists to read an `x-dependsOn` list and check truthiness against the
|
||||
parsed control values — but it is dead code, never called anywhere in the
|
||||
codebase (confirmed by repo-wide search). There is exactly one dynamic field
|
||||
in the entire codebase, so nothing today exercises multiple interdependent
|
||||
dynamic fields, an evaluation order between them, or a cycle.
|
||||
|
||||
There is no concrete second use case driving this yet — this is groundwork
|
||||
for the next widget that has multiple interdependent dynamic fields, so a
|
||||
future widget author has a declarative mechanism to reach for instead of
|
||||
reinventing `enrich_schema`'s ad hoc pattern, and so a cyclic mistake is
|
||||
caught at import time rather than shipped.
|
||||
|
||||
## Scope
|
||||
|
||||
In scope: a mechanism for a widget to register more than one dynamic-field
|
||||
enricher, each keyed to the schema field path it populates; deriving an
|
||||
execution order and a dependency graph from the fields' existing
|
||||
`x-dependsOn` declarations (no new declaration syntax); cycle detection at
|
||||
widget-registration time; retiring `Widget.enrich_schema` in favor of this
|
||||
mechanism, with `Balloons` retrofitted as the (only) proof case.
|
||||
|
||||
Out of scope:
|
||||
- Any new dynamic field or widget capability — this is infrastructure,
|
||||
validated against Balloons' existing single dynamic field with no behavior
|
||||
change.
|
||||
- Frontend changes. The served schema for `balloons` is unchanged by this
|
||||
work (same discipline as the composite-control-registry slice: golden
|
||||
fixture proves it).
|
||||
- Incremental/partial recomputation. Every control-value change already
|
||||
triggers a full `get_control_schema(control_values, series)` recompute on
|
||||
the frontend
|
||||
([SchemaControlPanel.tsx](../../../superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx)) —
|
||||
"changing one control updates its dependents" already happens for free via
|
||||
full statelessness recompute. What this slice adds is strictly about
|
||||
*multiple dynamic fields within one recompute* needing to run in the right
|
||||
order and see each other's computed output, and about catching a
|
||||
self-referential mistake in that graph.
|
||||
- A value-propagation/validation contract (how a provided value, a default,
|
||||
and a control-generated value interact) — still deferred to its own spec,
|
||||
per the composite-control-registry spec's Follow-ups section.
|
||||
|
||||
## Approach
|
||||
|
||||
**Derive the graph from the schema itself — one declaration, not two.**
|
||||
`x-dependsOn` already exists on every dynamic field. The alternative —
|
||||
a separate Python-level `enrichment_steps: ClassVar[list[EnrichmentStep]]`
|
||||
declaration with its own `depends_on` list — was considered and rejected: it
|
||||
would require a widget author to declare the same dependency twice (once in
|
||||
the field's `x-dependsOn` for frontend/MCP visibility, once again in Python
|
||||
for backend ordering), a duplication that drifts the moment one is updated
|
||||
without the other. Deriving the graph from the schema means there is exactly
|
||||
one place a dependency is ever written down.
|
||||
|
||||
This does mean `x-dependsOn` entries carry two possible meanings, disambiguated
|
||||
by what they name:
|
||||
- Names another **dynamic field's path** → an **ordering edge**: that field's
|
||||
enricher must run first, and this field's enricher receives its result.
|
||||
- Names anything else (a plain field on the parsed control model) → a
|
||||
**gate**, exactly `check_dependencies`'s existing (currently unused)
|
||||
semantics: this field's enricher only runs once that named attribute is
|
||||
truthy on `parsed`.
|
||||
|
||||
Field paths use the `a/b` dot-path convention `schema_tools.py` already
|
||||
established for drill-in paths
|
||||
([get_subtree](../../../superset/widgets/schema_tools.py)), so there is one
|
||||
path vocabulary across the widget-schema codebase, not two.
|
||||
|
||||
**Cycle detection at registration time, not request time.** The dependency
|
||||
graph for a widget type is fully static — it's derived from the Pydantic
|
||||
model's schema, which doesn't change between requests — so it can and should
|
||||
be checked once, when the widget registers, exactly like `@widget`'s existing
|
||||
duplicate-`widget_type` check
|
||||
([superset/core/api/core_api_injection.py](../../../superset/core/api/core_api_injection.py)).
|
||||
A cyclic dependency is an authoring mistake; it should fail the import, not
|
||||
surface lazily on some future request with a stack overflow or silent
|
||||
non-termination.
|
||||
|
||||
**Retire `Widget.enrich_schema` outright.** It has exactly one override in
|
||||
the entire codebase (`Balloons`). Keeping it as a parallel escape hatch
|
||||
alongside the new mechanism would mean documenting and maintaining two
|
||||
patterns for one real use case; retiring it keeps the contract to one
|
||||
pattern. (This framework is explicitly labeled experimental throughout its
|
||||
own docstrings, so there is no external-extension compatibility promise being
|
||||
broken here.)
|
||||
|
||||
## Design
|
||||
|
||||
Three corrections surfaced during implementation, verified directly rather
|
||||
than assumed — the code below reflects what was actually built and shipped,
|
||||
not the original draft:
|
||||
|
||||
1. **`EnricherFn` needs the whole schema, not just its own field's fragment.**
|
||||
Balloons' real enricher reads `schema["$defs"]["SeriesStyle"]`, a *sibling*
|
||||
`$defs` entry — unreachable from `Customization.series`'s own node alone.
|
||||
The signature is `(schema, node, parsed, series, upstream_results) -> Any`,
|
||||
not `(node, parsed, series, upstream_results) -> Any`.
|
||||
2. **`check_dependencies` never actually worked.** It resolved an
|
||||
`x-dependsOn` entry (e.g. `"dataBinding"`, the schema-facing alias)
|
||||
directly as a `getattr` name against the parsed model — but Pydantic
|
||||
attribute access always uses the Python field name (`data_binding`), never
|
||||
the alias, even under `populate_by_name=True`. Confirmed directly:
|
||||
`getattr(parsed, "dataBinding", "MISSING")` returns `"MISSING"` on a real
|
||||
parsed instance. It had zero callers before this slice, so this was never
|
||||
exercised. Fixed to resolve alias → field name via `model_fields` first.
|
||||
3. **The `x-dependsOn` gate is coarser than Balloons' actual guard, and stays
|
||||
that way.** `dataBinding` is a required field, so it's truthy whenever
|
||||
`parsed` exists at all — the gate can't express "`dimensions` is
|
||||
non-empty" (a nested attribute) or "`series` is non-empty" (a runtime
|
||||
parameter, not a field on `parsed`, so no `x-dependsOn` entry could ever
|
||||
name it). Balloons' enricher keeps its original fine-grained
|
||||
`if not dimensions or not series: return` guard unchanged; the schema-level
|
||||
gate is an additional coarse pre-filter that matters for the *new*
|
||||
multi-hop-ordering feature, not a replacement for a field's own logic.
|
||||
|
||||
### `superset_core/widgets/enrichment.py` (new)
|
||||
|
||||
```python
|
||||
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 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 (``schema_tools.py``'s drill-in
|
||||
convention). Descends into ``properties`` and resolves ``$ref`` against
|
||||
``$defs`` along the way."""
|
||||
|
||||
|
||||
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."""
|
||||
|
||||
|
||||
def toposort_or_raise(graph: dict[str, list[str]], widget_type: str) -> list[str]:
|
||||
"""Kahn's algorithm over ``graph``. Raises ``ValueError`` naming every
|
||||
field on the cycle when the graph isn't a DAG."""
|
||||
|
||||
|
||||
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
|
||||
(``check_dependencies``), and threading each enricher's return value
|
||||
forward as ``upstream_results[path]`` for anything ordered after it."""
|
||||
```
|
||||
|
||||
### `superset_core/semantic_layers/config.py` (changed)
|
||||
|
||||
`check_dependencies` resolves each `x-dependsOn` entry to its Pydantic field
|
||||
name (via `model_fields`'s alias mapping) before `getattr`, instead of trying
|
||||
the raw alias string directly — see correction 2 above. This is the first
|
||||
real caller `check_dependencies` has ever had.
|
||||
|
||||
### `superset_core/widgets/base.py` (changed)
|
||||
|
||||
`Widget` gains:
|
||||
```python
|
||||
enrichers: ClassVar[dict[str, EnricherFn]] = {}
|
||||
```
|
||||
and `get_control_schema` replaces its `cls.enrich_schema(schema, parsed,
|
||||
series or [])` call with the `dynamic_field_paths` → `build_dependency_graph`
|
||||
→ `toposort_or_raise` → `run_enrichers` pipeline above. The `enrich_schema`
|
||||
classmethod is removed from the base class entirely (not deprecated —
|
||||
retired, per the Approach section).
|
||||
|
||||
### `superset/core/api/core_api_injection.py` (changed)
|
||||
|
||||
`inject_widget_implementations`'s `widget_impl` decorator, right after
|
||||
`registry[key] = cls`, eagerly calls `cls.get_control_schema(None, None)` —
|
||||
that call's own internal `toposort_or_raise` is what detects a cycle, so no
|
||||
separate graph-building code is needed here; a `ValueError` propagates and
|
||||
the widget is popped back out of the registry rather than left
|
||||
half-registered. The static (`control_values=None`) schema is sufficient:
|
||||
`x-dynamic`/`x-dependsOn` are schema-level declarations, not value-dependent,
|
||||
so the graph is identical regardless of what control values a real request
|
||||
would carry.
|
||||
|
||||
### `superset/widgets/builtin.py` (changed)
|
||||
|
||||
`Balloons.enrich_schema` becomes:
|
||||
```python
|
||||
class Balloons(Widget):
|
||||
controls_class = BalloonsControls
|
||||
PALETTE = [...] # unchanged
|
||||
MAX_SERIES = 100 # unchanged
|
||||
|
||||
@staticmethod
|
||||
def _populate_series(schema, node, parsed, series, upstream):
|
||||
style_def = schema.get("$defs", {}).get("SeriesStyle")
|
||||
if style_def is None:
|
||||
return
|
||||
# Gate (x-dependsOn: ["dataBinding"]) only confirms dataBinding was
|
||||
# parsed at all -- dimensions/series non-emptiness stay checked here,
|
||||
# unchanged from the original enrich_schema body (see correction 3).
|
||||
dimensions = None
|
||||
if parsed is not None:
|
||||
data_binding = getattr(parsed, "data_binding", None)
|
||||
dimensions = getattr(data_binding, "dimensions", None)
|
||||
if not dimensions or not series:
|
||||
return
|
||||
... # dedupe/cap/palette body, otherwise identical to before
|
||||
|
||||
enrichers: ClassVar[dict[str, EnricherFn]] = {"customize/series": _populate_series}
|
||||
```
|
||||
`Customization.series`'s existing `x-dependsOn: ["dataBinding"]`
|
||||
(`superset/widgets/controls.py`) is unchanged — `dataBinding` doesn't name
|
||||
another dynamic-field path, so it resolves to a gate, now actually enforced
|
||||
by the fixed `check_dependencies` rather than being dead code.
|
||||
|
||||
## Testing
|
||||
|
||||
- **`enrichment.py` unit tests** (new `tests/unit_tests/widgets/test_enrichment.py`):
|
||||
`dynamic_field_paths` against a schema with nested `x-dynamic` fields (including
|
||||
one inside `$defs`, mirroring `Customization.series`'s actual shape);
|
||||
`build_dependency_graph`'s path-vs-gate disambiguation (an entry naming a
|
||||
known dynamic path becomes an edge, an entry naming anything else doesn't);
|
||||
`toposort_or_raise` on a synthetic 3-node chain (correct order) and a
|
||||
synthetic 2-node cycle (raises, names both members); `run_enrichers`
|
||||
threading a synthetic upstream enricher's return value into a downstream
|
||||
one's `upstream_results` argument, proving the "sees already-computed
|
||||
values" claim isn't just asserted but actually exercised.
|
||||
- **Registration-time cycle test**: two dynamic fields on a throwaway test
|
||||
widget whose `x-dependsOn` name each other raises at `@widget`-application
|
||||
time (import time), not on a later `get_control_schema` call — asserted by
|
||||
catching the exception around the class definition itself, not around a
|
||||
later method call.
|
||||
- **Balloons regression, golden-fixture discipline**: existing
|
||||
`test_builtin.py`/`test_registry.py` Balloons tests (series population
|
||||
across the empty/dimension-only/series-only/both states, `x-control`
|
||||
extras, base schema shape) pass unchanged after the retrofit — proving the
|
||||
new mechanism reproduces `enrich_schema`'s exact prior behavior for the one
|
||||
real case, not just that it runs without erroring.
|
||||
|
||||
## Follow-ups (separate specs)
|
||||
|
||||
- A value-propagation/validation contract (provided value vs. default vs.
|
||||
control-generated value) — unchanged from the composite-control-registry
|
||||
spec's Follow-ups, still not addressed by this slice.
|
||||
- Whether a second real widget with genuinely interdependent dynamic fields
|
||||
ever arrives to validate this mechanism beyond the single-field Balloons
|
||||
retrofit — this spec is deliberately built and tested against synthetic
|
||||
multi-field cases in `test_enrichment.py` precisely because no real
|
||||
second case exists yet.
|
||||
@@ -1,142 +0,0 @@
|
||||
# Widget Value Write Path (`set_widget_control_values`) — Design
|
||||
|
||||
**Status:** Approved for planning
|
||||
**Branch:** `enxdev/poc/dashboard-v2-editing-ui-flow`
|
||||
**Date:** 2026-08-27
|
||||
|
||||
## Problem
|
||||
|
||||
The original ask ("define how provided values, defaults, and control-generated
|
||||
values are propagated and validated") assumed infrastructure that doesn't
|
||||
exist. Verified directly: Dashboard V2's node tree (`node.props`) lives only
|
||||
in the frontend's in-memory `DashboardProvider` singleton, whose own docstring
|
||||
says "No persistence." `Widget.validate_control_values` — the commit-time
|
||||
strict-validation gate — has exactly one caller in the whole codebase (the
|
||||
`/type/<widget_type>/validate` REST endpoint's own handler), and nothing in
|
||||
the traced human edit flow (Inspector → `provider.updateProps`) calls it.
|
||||
Nothing writes a computed value into `node.props` today — enrichers (e.g.
|
||||
Balloons') only shape the served *schema*. And `WIDGET_FRAMEWORK.md`, cited as
|
||||
"the broader proposal" this POC implements half of, doesn't exist in the repo.
|
||||
|
||||
Given that, this slice is scoped narrowly to the one concrete, motivating
|
||||
case: an agent (MCP) writing control values needs its edit validated before
|
||||
it's accepted, and today no MCP tool writes control values at all —
|
||||
`mcp_service/widgets/tool/` only had two read-only tools.
|
||||
|
||||
## Scope
|
||||
|
||||
In scope: one new MCP tool, `set_widget_control_values`, that locates a
|
||||
widget node, builds a validated candidate without touching the stored node,
|
||||
and commits only on success — giving `validate_control_values` its first
|
||||
real caller.
|
||||
|
||||
Explicitly out of scope (per direct instruction, not rediscovered mid-build):
|
||||
- **Persistence.** The node store this tool operates on is new,
|
||||
MCP-process-local, in-memory state — not a bridge to the frontend's real
|
||||
document (which has no backend-addressable form) and not a database table.
|
||||
It does not survive a process restart.
|
||||
- **Default seeding.** A node with no `dataBinding` set still has no
|
||||
`dataBinding` set after this tool runs unless the caller provides one;
|
||||
schema defaults are not proactively written into a node's stored values.
|
||||
- **Generated-value propagation.** Nothing here writes an enricher-computed
|
||||
value into a node's stored `props` — enrichment still only shapes the
|
||||
served schema, unchanged from the control-dependency-graph slice.
|
||||
- **Incremental enrichment.** Each call revalidates the full candidate; there
|
||||
is no partial/incremental recomputation.
|
||||
- **A broader value-precedence contract.** This does not define a general
|
||||
"provided vs. default vs. generated" policy — it defines exactly one
|
||||
operation's behavior (merge, validate, commit-or-reject).
|
||||
|
||||
## Design
|
||||
|
||||
### `superset/mcp_service/widgets/node_store.py` (new)
|
||||
|
||||
A minimal registry — `WidgetNode` (`widget_type: str`, `props: dict[str,
|
||||
Any]`) and a module-level `nodes: dict[str, WidgetNode]` — mirroring
|
||||
`superset/widgets/registry.py`'s plain-dict idiom. Tests seed it directly
|
||||
(`nodes["n1"] = WidgetNode(...)`); this tool does not create nodes, only
|
||||
writes to ones that already exist (an unknown `node_id` is a structured
|
||||
error, not an implicit create).
|
||||
|
||||
### `superset/mcp_service/widgets/tool/set_widget_control_values.py` (new)
|
||||
|
||||
```python
|
||||
def _set_widget_control_values_impl(node_id, control_values):
|
||||
node = nodes.get(node_id)
|
||||
if node is None:
|
||||
return unknown_node_error(node_id)
|
||||
widget = resolve_widget(node.widget_type)
|
||||
if widget is None:
|
||||
return unknown_widget_type_error(node.widget_type)
|
||||
|
||||
candidate = {**node.props, **control_values} # shallow merge
|
||||
errors = widget.validate_control_values(candidate)
|
||||
if errors:
|
||||
return {"errors": errors} # node.props untouched
|
||||
|
||||
normalized = widget.controls_class.model_validate(candidate).model_dump(by_alias=True)
|
||||
node.props = normalized # single reassignment: the only mutation, and only on success
|
||||
return {"errors": [], "values": normalized}
|
||||
```
|
||||
|
||||
The merge is shallow — new top-level keys override, everything else is kept
|
||||
— deliberately matching `DashboardProvider.updateProps`'s existing merge
|
||||
semantics on the frontend, so this tool's behavior isn't a new, different
|
||||
merge policy from the one humans already experience.
|
||||
|
||||
"Atomic rollback on failure" falls out of the design rather than needing
|
||||
explicit rollback code: `node.props` is never mutated in place (no `.update()`
|
||||
on it) — a candidate dict is built separately and only assigned to
|
||||
`node.props` after validation succeeds. A failed call touches nothing.
|
||||
|
||||
`validate_control_values` discards the validated model (it returns only an
|
||||
error list), so getting back *normalized* values (coerced types, alias keys)
|
||||
requires a second `model_validate` call on the same already-known-valid
|
||||
input — cheap and deterministic, not a design compromise, just how the
|
||||
existing method's return shape works.
|
||||
|
||||
Registered via `@tool(tags=["mutate"], class_permission_name="Chart",
|
||||
annotations=ToolAnnotations(title=..., readOnlyHint=False,
|
||||
destructiveHint=False))`, matching the sibling read-only widget tools'
|
||||
`class_permission_name` (reusing the `Chart` permission, no new permission
|
||||
introduced) and `mcp_service/dashboard/tool/`'s `tags=["mutate"]` convention
|
||||
for a write tool. Imported in `superset/mcp_service/app.py` alongside the
|
||||
other two widget tools — a tool exported from `tool/__init__.py` but missing
|
||||
from `app.py`'s import list is invisible to a real MCP client despite every
|
||||
direct-call unit test passing; verified this distinction matters by adding a
|
||||
`Client(mcp)`-based registration test, not just direct `_impl` calls.
|
||||
|
||||
### `superset/mcp_service/widgets/utils.py` (changed)
|
||||
|
||||
Adds `unknown_node_error(node_id)`, mirroring the existing
|
||||
`unknown_widget_type_error`'s structured-error shape.
|
||||
|
||||
## Testing
|
||||
|
||||
- Successful write: merge preserves untouched keys, normalizes to schema
|
||||
defaults for fields nobody set, actually updates the stored node (not just
|
||||
the return value).
|
||||
- Invalid values: structured errors returned, stored node byte-identical to
|
||||
before the call (the rollback-by-construction proof).
|
||||
- Missing required field: same failure path, `dataBinding` named in the
|
||||
error location.
|
||||
- Unknown `node_id`: structured `unknown_node` error.
|
||||
- Node referencing an unregistered `widget_type` (an orphaned-node defensive
|
||||
case): structured `invalid_widget_type` error.
|
||||
- End-to-end registration: a `Client(mcp)`-based test proving the tool is
|
||||
reachable through the real MCP surface, not just importable — this is the
|
||||
check that would have caught the tool being exported from `tool/__init__.py`
|
||||
but never added to `app.py`'s import list, a mistake every direct-call test
|
||||
above is blind to.
|
||||
|
||||
## Follow-ups (not this slice)
|
||||
|
||||
- Whether Dashboard V2 ever gets a real backend-addressable document (a
|
||||
genuine bridge between the frontend's `DashboardProvider` and any backend
|
||||
process) is an open, larger architectural question this slice deliberately
|
||||
does not answer — the node store here is scoped to prove out validation
|
||||
wiring, not to be that bridge.
|
||||
- Default-seeding a freshly placed widget's `props` (currently `undefined`
|
||||
until a human or agent touches every field) remains unaddressed.
|
||||
- The general provided/default/generated value-precedence contract from the
|
||||
original ask remains undesigned.
|
||||
Reference in New Issue
Block a user