From 18465bad51fae607d29b30f2c73d87fac14a920e Mon Sep 17 00:00:00 2001 From: Jyun-An Chen Date: Thu, 3 Sep 2026 17:07:47 +0800 Subject: [PATCH 1/2] perf(dashboard): batch dataset lookups during dashboard export (#43017) Co-authored-by: Enzo Martellucci <52219496+EnxDev@users.noreply.github.com> --- superset/commands/dashboard/export.py | 98 +++--- .../commands/dashboard/export_test.py | 287 +++++++++++++++++- 2 files changed, 340 insertions(+), 45 deletions(-) diff --git a/superset/commands/dashboard/export.py b/superset/commands/dashboard/export.py index 940adb691e9c..3d831dc20fee 100644 --- a/superset/commands/dashboard/export.py +++ b/superset/commands/dashboard/export.py @@ -51,6 +51,19 @@ DEFAULT_CHART_WIDTH = 4 +def _coerce_dataset_id(raw_dataset_id: Any) -> Optional[int]: + if isinstance(raw_dataset_id, bool): + return None + if isinstance(raw_dataset_id, int): + return raw_dataset_id + if isinstance(raw_dataset_id, str) and raw_dataset_id.isdigit(): + try: + return int(raw_dataset_id) + except ValueError: + return None + return None + + def get_default_position(title: str) -> dict[str, Any]: return { "DASHBOARD_VERSION_KEY": "v2", @@ -317,37 +330,53 @@ def _file_content(model: Dashboard) -> str: logger.info("Unable to decode `%s` field: %s", key, value) payload[new_name] = {} + metadata = payload.get("metadata") or {} + + referenced_dataset_ids = { + dataset_id + for native_filter in metadata.get("native_filter_configuration", []) + for target in native_filter.get("targets", []) + if (dataset_id := _coerce_dataset_id(target.get("datasetId"))) is not None + } | { + dataset_id + for customization in metadata.get("chart_customization_config") or [] + for target in customization.get("targets") or [] + if (dataset_id := _coerce_dataset_id(target.get("datasetId"))) is not None + } + datasets_by_id = { + dataset.id: dataset + for dataset in DatasetDAO.find_by_ids(list(referenced_dataset_ids)) + } + # Extract all native filter datasets and replace native # filter dataset references with uuid - for native_filter in payload.get("metadata", {}).get( - "native_filter_configuration", [] - ): + for native_filter in metadata.get("native_filter_configuration", []): for target in native_filter.get("targets", []): - dataset_id = target.pop("datasetId", None) - if dataset_id is not None: - dataset = DatasetDAO.find_by_id(dataset_id) - if dataset: - target["datasetUuid"] = str(dataset.uuid) + dataset_id = _coerce_dataset_id(target.pop("datasetId", None)) + if dataset_id is not None and ( + dataset := datasets_by_id.get(dataset_id) + ): + target["datasetUuid"] = str(dataset.uuid) # Replace display control dataset references with uuid. # datasetId is intentionally preserved alongside datasetUuid so that # bundles remain importable by older versions that do not yet understand # datasetUuid for display-control targets. - for customization in ( - payload.get("metadata", {}).get("chart_customization_config") or [] - ): + for customization in metadata.get("chart_customization_config") or []: for target in customization.get("targets") or []: - dataset_id = target.get("datasetId") - if dataset_id is not None: - dataset = DatasetDAO.find_by_id(dataset_id) - if dataset: + raw_dataset_id = target.get("datasetId") + if raw_dataset_id is not None: + dataset_id = _coerce_dataset_id(raw_dataset_id) + if dataset_id is not None and ( + dataset := datasets_by_id.get(dataset_id) + ): target["datasetUuid"] = str(dataset.uuid) else: logger.warning( "Dashboard '%s': display control target references " "missing dataset %s; datasetUuid will not be set", model.dashboard_title, - dataset_id, + raw_dataset_id, ) # the mapping between dashboard -> charts is inferred from the position @@ -440,30 +469,27 @@ def _export( payload[new_name] = {} if export_related: + metadata = payload.get("metadata") or {} + # Extract all native filter datasets and export referenced datasets - for native_filter in payload.get("metadata", {}).get( - "native_filter_configuration", [] - ): + referenced_dataset_ids: set[int] = set() + for native_filter in metadata.get("native_filter_configuration", []): for target in native_filter.get("targets", []): - dataset_id = target.pop("datasetId", None) + dataset_id = _coerce_dataset_id(target.pop("datasetId", None)) if dataset_id is not None: - dataset = DatasetDAO.find_by_id(dataset_id) - if dataset: - # Pass the shared seen set to the dataset export command - yield from ExportDatasetsCommand([dataset_id]).run( - seen=seen - ) + referenced_dataset_ids.add(dataset_id) # Export datasets referenced by display controls - for customization in ( - payload.get("metadata", {}).get("chart_customization_config") or [] - ): + for customization in metadata.get("chart_customization_config") or []: for target in customization.get("targets") or []: - dataset_id = target.get("datasetId") + dataset_id = _coerce_dataset_id(target.get("datasetId")) if dataset_id is not None: - dataset = DatasetDAO.find_by_id(dataset_id) - if dataset: - # Pass the shared seen set to the dataset export command - yield from ExportDatasetsCommand([dataset_id]).run( - seen=seen - ) + referenced_dataset_ids.add(dataset_id) + + found_dataset_ids = [ + dataset.id + for dataset in DatasetDAO.find_by_ids(list(referenced_dataset_ids)) + ] + if found_dataset_ids: + # Pass the shared seen set to the dataset export command + yield from ExportDatasetsCommand(found_dataset_ids).run(seen=seen) diff --git a/tests/unit_tests/commands/dashboard/export_test.py b/tests/unit_tests/commands/dashboard/export_test.py index 53fba8e0d7f1..0a80b980041a 100644 --- a/tests/unit_tests/commands/dashboard/export_test.py +++ b/tests/unit_tests/commands/dashboard/export_test.py @@ -82,12 +82,13 @@ def test_file_content_replaces_dataset_id_with_uuid_in_display_controls(): ) mock_dataset = MagicMock() + mock_dataset.id = 99 mock_dataset.uuid = dataset_uuid with ( patch( - "superset.commands.dashboard.export.DatasetDAO.find_by_id", - return_value=mock_dataset, + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset], ), patch( "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", @@ -108,6 +109,272 @@ def test_file_content_replaces_dataset_id_with_uuid_in_display_controls(): assert customizations[1]["targets"] == [] +def test_file_content_batches_dataset_lookup_across_targets(): + """ + Regression test: dataset lookups must go through a single batched + DatasetDAO.find_by_ids call, not one DatasetDAO.find_by_id call per + target. Multiple filters/customizations referencing the same dataset + must not trigger redundant DB round-trips. + """ + from superset.commands.dashboard.export import ExportDashboardsCommand + + dataset_uuid_1 = str(uuid.uuid4()) + dataset_uuid_2 = str(uuid.uuid4()) + + mock_dashboard = _make_mock_dashboard( + { + "native_filter_configuration": [ + { + "id": "FILTER-1", + "targets": [{"datasetId": 1}, {"datasetId": 2}], + }, + { + "id": "FILTER-2", + "targets": [{"datasetId": 1}], + }, + ], + "chart_customization_config": [ + { + "id": "CUSTOMIZATION-1", + "type": "CHART_CUSTOMIZATION", + "targets": [{"datasetId": 1}], + }, + ], + } + ) + + mock_dataset_1 = MagicMock() + mock_dataset_1.id = 1 + mock_dataset_1.uuid = dataset_uuid_1 + mock_dataset_2 = MagicMock() + mock_dataset_2.id = 2 + mock_dataset_2.uuid = dataset_uuid_2 + + with ( + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset_1, mock_dataset_2], + ) as mock_find_by_ids, + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_id" + ) as mock_find_by_id, + patch( + "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", + return_value=False, + ), + ): + content = ExportDashboardsCommand._file_content(mock_dashboard) + + mock_find_by_id.assert_not_called() + mock_find_by_ids.assert_called_once() + (called_ids,), _ = mock_find_by_ids.call_args + assert set(called_ids) == {1, 2} + + result = yaml.safe_load(content) + native_filters = result["metadata"]["native_filter_configuration"] + assert native_filters[0]["targets"][0]["datasetUuid"] == dataset_uuid_1 + assert native_filters[0]["targets"][1]["datasetUuid"] == dataset_uuid_2 + assert native_filters[1]["targets"][0]["datasetUuid"] == dataset_uuid_1 + + customization_target = result["metadata"]["chart_customization_config"][0][ + "targets" + ][0] + assert customization_target["datasetUuid"] == dataset_uuid_1 + + +def test_export_batches_dataset_export_across_targets(): + """ + Regression test: _export must batch dataset exports into a single + ExportDatasetsCommand call, not one call per target. Multiple + filters/customizations referencing the same dataset must only trigger + a single find_by_ids lookup and a single export command. + """ + from superset.commands.dashboard.export import ExportDashboardsCommand + + mock_dashboard = _make_mock_dashboard( + { + "native_filter_configuration": [ + { + "id": "FILTER-1", + "targets": [{"datasetId": 1}, {"datasetId": 2}], + }, + ], + "chart_customization_config": [ + { + "id": "CUSTOMIZATION-1", + "type": "CHART_CUSTOMIZATION", + "targets": [{"datasetId": 1}], + }, + ], + } + ) + + mock_dataset_1 = MagicMock() + mock_dataset_1.id = 1 + mock_dataset_2 = MagicMock() + mock_dataset_2.id = 2 + mock_datasets_cmd = MagicMock() + mock_datasets_cmd.run.return_value = iter([]) + + with ( + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset_1, mock_dataset_2], + ) as mock_find_by_ids, + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_id" + ) as mock_find_by_id, + patch( + "superset.commands.dashboard.export.ExportDatasetsCommand", + return_value=mock_datasets_cmd, + ) as mock_datasets_cls, + patch( + "superset.commands.dashboard.export.ExportChartsCommand" + ) as mock_charts_cls, + patch( + "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", + return_value=False, + ), + ): + mock_charts_cls.return_value.run.return_value = iter([]) + list(ExportDashboardsCommand._export(mock_dashboard)) + + mock_find_by_id.assert_not_called() + mock_find_by_ids.assert_called_once() + mock_datasets_cls.assert_called_once() + mock_datasets_cmd.run.assert_called_once() + (called_ids,), _ = mock_datasets_cls.call_args + assert set(called_ids) == {1, 2} + + +def test_file_content_resolves_string_and_int_dataset_ids_to_same_dataset(): + """ + Regression test: datasetId may be stored as either an int or a numeric + string (native_filter_cache.py types it int | str). A target with a + string datasetId must still resolve against the (int-keyed) dataset + lookup instead of silently missing datasetUuid. + """ + from superset.commands.dashboard.export import ExportDashboardsCommand + + dataset_uuid = str(uuid.uuid4()) + + mock_dashboard = _make_mock_dashboard( + { + "native_filter_configuration": [ + { + "id": "FILTER-1", + "targets": [{"datasetId": "5"}, {"datasetId": 5}], + }, + ], + "chart_customization_config": [], + } + ) + + mock_dataset = MagicMock() + mock_dataset.id = 5 + mock_dataset.uuid = dataset_uuid + + with ( + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset], + ) as mock_find_by_ids, + patch( + "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", + return_value=False, + ), + ): + content = ExportDashboardsCommand._file_content(mock_dashboard) + + # both the string and int forms of the same id are batched together + (called_ids,), _ = mock_find_by_ids.call_args + assert set(called_ids) == {5} + + native_filters = yaml.safe_load(content)["metadata"]["native_filter_configuration"] + assert native_filters[0]["targets"][0]["datasetUuid"] == dataset_uuid + assert native_filters[0]["targets"][1]["datasetUuid"] == dataset_uuid + + +def test_coerce_dataset_id_rejects_non_integral_values(): + """Regression test: bare int() silently truncates 1.9 to 1, parses "1_0" as 10.""" + from superset.commands.dashboard.export import _coerce_dataset_id + + assert _coerce_dataset_id(5) == 5 + assert _coerce_dataset_id("5") == 5 + assert _coerce_dataset_id(1.9) is None + assert _coerce_dataset_id("1.9") is None + assert _coerce_dataset_id("1_0") is None + assert _coerce_dataset_id("abc") is None + assert _coerce_dataset_id(None) is None + assert _coerce_dataset_id(True) is None + assert _coerce_dataset_id(-5) == -5 + assert _coerce_dataset_id("-5") is None + + +def test_export_skips_dangling_dataset_references_without_raising(): + """ + Regression test: the find_by_ids pre-filter in _export must only pass + ids that actually resolved to ExportDatasetsCommand. Passing every + referenced id straight through — including one for a dataset that no + longer exists — would make ExportModelsCommand.validate() raise + DatasetNotFoundError and abort the entire dashboard export over a + single dangling filter/customization reference. + """ + from superset.commands.dashboard.export import ExportDashboardsCommand + + mock_dashboard = _make_mock_dashboard( + { + "native_filter_configuration": [ + { + "id": "FILTER-1", + "targets": [{"datasetId": 1}, {"datasetId": 2}], + }, + ], + "chart_customization_config": [ + { + "id": "CUSTOMIZATION-1", + "type": "CHART_CUSTOMIZATION", + # dataset 3 no longer exists (deleted dataset) + "targets": [{"datasetId": 3}], + }, + ], + } + ) + + mock_dataset_1 = MagicMock() + mock_dataset_1.id = 1 + mock_dataset_2 = MagicMock() + mock_dataset_2.id = 2 + mock_datasets_cmd = MagicMock() + mock_datasets_cmd.run.return_value = iter([]) + + with ( + # dataset 3 is deliberately absent from the resolved list + patch( + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset_1, mock_dataset_2], + ), + patch( + "superset.commands.dashboard.export.ExportDatasetsCommand", + return_value=mock_datasets_cmd, + ) as mock_datasets_cls, + patch( + "superset.commands.dashboard.export.ExportChartsCommand" + ) as mock_charts_cls, + patch( + "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", + return_value=False, + ), + ): + mock_charts_cls.return_value.run.return_value = iter([]) + # must not raise DatasetNotFoundError + list(ExportDashboardsCommand._export(mock_dashboard)) + + mock_datasets_cls.assert_called_once() + (called_ids,), _ = mock_datasets_cls.call_args + assert set(called_ids) == {1, 2} + + def test_export_yields_dataset_files_for_display_controls(): """ _export must yield dataset files for datasets referenced by display controls. @@ -134,14 +401,15 @@ def test_export_yields_dataset_files_for_display_controls(): ) mock_dataset = MagicMock() + mock_dataset.id = dataset_id sentinel_file = ("datasets/my_dataset.yaml", lambda: "dataset_content") mock_datasets_cmd = MagicMock() mock_datasets_cmd.run.return_value = iter([sentinel_file]) with ( patch( - "superset.commands.dashboard.export.DatasetDAO.find_by_id", - return_value=mock_dataset, + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[mock_dataset], ), patch( "superset.commands.dashboard.export.ExportDatasetsCommand", @@ -686,9 +954,10 @@ def test_stabilize_chart_ids_remaps_expanded_slices() -> None: def test_file_content_missing_dataset_preserves_dataset_id() -> None: """ - When DatasetDAO.find_by_id returns None for a display control target, - datasetId is preserved (dual-write: it was never popped) and no - datasetUuid is added — the target is not silently emptied. + When DatasetDAO.find_by_ids does not return a match for a display + control target's dataset, datasetId is preserved (dual-write: it was + never popped) and no datasetUuid is added — the target is not silently + emptied. """ from superset.commands.dashboard.export import ExportDashboardsCommand @@ -706,8 +975,8 @@ def test_file_content_missing_dataset_preserves_dataset_id() -> None: with ( patch( - "superset.commands.dashboard.export.DatasetDAO.find_by_id", - return_value=None, + "superset.commands.dashboard.export.DatasetDAO.find_by_ids", + return_value=[], ), patch( "superset.commands.dashboard.export.feature_flag_manager.is_feature_enabled", From bf12a21367ca10f6f29e553b3245f7d888d543f5 Mon Sep 17 00:00:00 2001 From: Mafi Date: Thu, 3 Sep 2026 20:32:01 +1000 Subject: [PATCH 2/2] fix(charts): sort chart types by display name (#43634) Co-authored-by: Matt Fitzgerald Co-authored-by: Amin Ghadersohi --- .../src/components/ListView/types.ts | 2 + .../ChartList/ChartList.listview.test.tsx | 76 ++++++++- .../src/pages/ChartList/index.tsx | 28 +++- .../src/views/CRUD/hooks.test.tsx | 102 ++++++++++++ superset-frontend/src/views/CRUD/hooks.ts | 17 ++ superset/charts/api.py | 146 +++++++++++++++++- superset/charts/schemas.py | 21 +++ tests/integration_tests/charts/api_tests.py | 114 ++++++++++++++ tests/unit_tests/charts/test_schemas.py | 42 +++++ 9 files changed, 544 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/components/ListView/types.ts b/superset-frontend/src/components/ListView/types.ts index 6a07082ed1e5..571b02306f34 100644 --- a/superset-frontend/src/components/ListView/types.ts +++ b/superset-frontend/src/components/ListView/types.ts @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { type JsonObject } from '@superset-ui/core'; import { type ReactNode } from 'react'; export interface SortColumn { @@ -102,6 +103,7 @@ export interface ListViewFetchDataConfig { pageSize: number; sortBy: SortColumn[]; filters: ListViewFilterValue[]; + extraQueryParams?: JsonObject; } export interface InternalFilter extends ListViewFilterValue { diff --git a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx index 69ff7e21f7b3..d04d9a56ba8c 100644 --- a/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx +++ b/superset-frontend/src/pages/ChartList/ChartList.listview.test.tsx @@ -20,7 +20,12 @@ import fetchMock from 'fetch-mock'; import { mockUserSubjectsBootstrapData } from 'spec/helpers/mockBootstrapData'; import { screen, waitFor, within } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; -import { isFeatureEnabled } from '@superset-ui/core'; +import rison from 'rison'; +import { + ChartMetadata, + getChartMetadataRegistry, + isFeatureEnabled, +} from '@superset-ui/core'; import { mockCharts, mockHandleResourceExport, @@ -279,6 +284,75 @@ test('sorts table when clicking column headers', async () => { }); }); +test('sends display-ordered chart type slugs only for Type sorting', async () => { + const registry = getChartMetadataRegistry(); + const customVizTypes = Array.from( + { length: 260 }, + (_, index) => `custom_display_order_${index}`, + ); + registry + .registerValue( + 'slug_a', + new ChartMetadata({ name: '001 Zulu', thumbnail: '', behaviors: [] }), + ) + .registerValue( + 'slug_z', + new ChartMetadata({ name: '000 Alpha', thumbnail: '', behaviors: [] }), + ); + customVizTypes.forEach((vizType, index) => + registry.registerValue( + vizType, + new ChartMetadata({ + name: `Plugin ${index}`, + thumbnail: '', + behaviors: [], + }), + ), + ); + + try { + renderChartList(mockUser); + + const table = await screen.findByTestId('listview-table'); + const initialCall = fetchMock.callHistory + .calls(/chart\/\?q/) + .find(call => !call.url.includes('order_column:viz_type')); + expect(initialCall).toBeDefined(); + const initialQuery = new URL( + initialCall!.url, + 'http://localhost', + ).searchParams.get('q'); + expect(rison.decode(initialQuery!)).not.toHaveProperty('viz_type_order'); + + await userEvent.click(within(table).getByTitle('Type')); + + await waitFor(() => { + const typeSortCall = fetchMock.callHistory + .calls(/chart\/\?q/) + .find(call => call.url.includes('order_column:viz_type')); + expect(typeSortCall).toBeDefined(); + + const query = new URL( + typeSortCall!.url, + 'http://localhost', + ).searchParams.get('q'); + const decoded = rison.decode(query!) as { + order_column: string; + viz_type_order: string[]; + }; + expect(decoded.order_column).toBe('viz_type'); + expect(decoded.viz_type_order).toHaveLength(256); + expect(decoded.viz_type_order.indexOf('slug_z')).toBeLessThan( + decoded.viz_type_order.indexOf('slug_a'), + ); + }); + } finally { + registry.remove('slug_a'); + registry.remove('slug_z'); + customVizTypes.forEach(vizType => registry.remove(vizType)); + } +}); + test('displays chart data correctly in table rows', async () => { /** * @todo Implement test logic for tagging. diff --git a/superset-frontend/src/pages/ChartList/index.tsx b/superset-frontend/src/pages/ChartList/index.tsx index 16c4d40c62cf..7d2f90f4bff9 100644 --- a/superset-frontend/src/pages/ChartList/index.tsx +++ b/superset-frontend/src/pages/ChartList/index.tsx @@ -71,6 +71,7 @@ import { ListViewFilterOperator as FilterOperator, DashboardCrossLinks, type ListViewProps, + type ListViewFetchDataConfig, type ListViewFilters, type ListViewFilter, } from 'src/components'; @@ -198,6 +199,7 @@ const CONFIRM_OVERWRITE_MESSAGE = t( ); const registry = getChartMetadataRegistry(); +const MAX_VIZ_TYPE_ORDER_LENGTH = 256; const createFetchDatasets = async ( filterValue = '', @@ -260,11 +262,35 @@ function ChartList(props: ChartListProps) { }, setResourceCollection: setCharts, hasPerm, - fetchData, + fetchData: fetchChartData, toggleBulkSelect, refreshData, } = useListViewResource('chart', t('chart'), addDangerToast); + const fetchData = useCallback( + (config: ListViewFetchDataConfig) => + fetchChartData({ + ...config, + ...(config.sortBy[0]?.id === 'viz_type' + ? { + extraQueryParams: { + ...config.extraQueryParams, + viz_type_order: registry + .keys() + .sort((left, right) => { + const nameComparison = ( + registry.get(left)?.name || left + ).localeCompare(registry.get(right)?.name || right); + return nameComparison || left.localeCompare(right); + }) + .slice(0, MAX_VIZ_TYPE_ORDER_LENGTH), + }, + } + : {}), + }), + [fetchChartData], + ); + const chartIds = useMemo(() => charts.map(c => c.id), [charts]); const { roles } = useSelector( state => state.user, diff --git a/superset-frontend/src/views/CRUD/hooks.test.tsx b/superset-frontend/src/views/CRUD/hooks.test.tsx index 179970fefb41..dbe44e83b814 100644 --- a/superset-frontend/src/views/CRUD/hooks.test.tsx +++ b/superset-frontend/src/views/CRUD/hooks.test.tsx @@ -17,6 +17,7 @@ * under the License. */ import { renderHook, act } from '@testing-library/react'; +import rison from 'rison'; import { waitFor } from 'spec/helpers/testing-library'; import { JsonResponse, SupersetClient } from '@superset-ui/core'; @@ -551,6 +552,107 @@ test('useListViewResource: uses desc sort direction when desc is true', async () expect(endpoint).toContain('order_direction:desc'); }); +test('useListViewResource: includes extra list query parameters', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'viz_type' }], + filters: [], + extraQueryParams: { + viz_type_order: ['slug_z', 'slug_a'], + }, + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + const query = new URL(endpoint, 'http://localhost').searchParams.get('q'); + expect(rison.decode(query!)).toMatchObject({ + order_column: 'viz_type', + viz_type_order: ['slug_z', 'slug_a'], + }); +}); + +test('useListViewResource: refresh reuses extra list query parameters', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'viz_type' }], + filters: [], + extraQueryParams: { viz_type_order: ['slug_z', 'slug_a'] }, + }); + await result.current.refreshData(); + }); + + const listQueries = getSpy.mock.calls + .map(call => (call[0] as { endpoint: string }).endpoint) + .filter(endpoint => endpoint.includes('/api/v1/chart/?q=')) + .map(endpoint => { + const query = new URL(endpoint, 'http://localhost').searchParams.get('q'); + return rison.decode(query!); + }); + expect(listQueries).toHaveLength(2); + expect(listQueries).toEqual([ + expect.objectContaining({ viz_type_order: ['slug_z', 'slug_a'] }), + expect.objectContaining({ viz_type_order: ['slug_z', 'slug_a'] }), + ]); +}); + +test('useListViewResource: extra parameters cannot replace list controls', async () => { + const getSpy = jest.spyOn(SupersetClient, 'get').mockResolvedValue({ + json: { result: [], count: 0 }, + } as unknown as JsonResponse); + + const { result } = renderHook(() => + useListViewResource('chart', 'Charts', jest.fn()), + ); + + await act(async () => { + await result.current.fetchData({ + pageIndex: 0, + pageSize: 25, + sortBy: [{ id: 'viz_type' }], + filters: [], + extraQueryParams: { + custom_param: 'preserved', + filters: [{ col: 'slice_name', opr: 'eq', value: 'injected' }], + order_column: 'slice_name', + order_direction: 'desc', + page: 99, + page_size: 1, + select_columns: ['slice_name'], + }, + }); + }); + + const endpoint = findEndpoint(getSpy, '/api/v1/chart/?q='); + const query = new URL(endpoint, 'http://localhost').searchParams.get('q'); + expect(rison.decode(query!)).toEqual({ + custom_param: 'preserved', + order_column: 'viz_type', + order_direction: 'asc', + page: 0, + page_size: 25, + }); +}); + // useSingleViewResource test('useSingleViewResource: initial state has loading false and null resource', () => { const { result } = renderHook(() => diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index ea320372e58d..631dcc72a125 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -61,6 +61,15 @@ interface ListViewResourceState { lastFetched?: string; } +const reservedListQueryParams = new Set([ + 'filters', + 'order_column', + 'order_direction', + 'page', + 'page_size', + 'select_columns', +]); + const parsedErrorMessage = ( errorMessage: Record | string, ) => { @@ -156,6 +165,7 @@ export function useListViewResource( pageSize, sortBy, filters: filterValues, + extraQueryParams, }: FetchDataConfig) => { const requestId = latestRequestIdRef.current + 1; latestRequestIdRef.current = requestId; @@ -165,6 +175,7 @@ export function useListViewResource( pageIndex, pageSize, sortBy, + extraQueryParams, }; lastFetchDataConfigRef.current = config; // set loading state, cache the last config for refreshing data. @@ -186,7 +197,13 @@ export function useListViewResource( : value, })); + const safeExtraQueryParams = Object.fromEntries( + Object.entries(extraQueryParams ?? {}).filter( + ([key]) => !reservedListQueryParams.has(key), + ), + ); const queryParams = rison.encode_uri({ + ...safeExtraQueryParams, order_column: sortBy[0].id, order_direction: sortBy[0].desc ? 'desc' : 'asc', page: pageIndex, diff --git a/superset/charts/api.py b/superset/charts/api.py index 605bd2cf7dac..0e17924d25cf 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -16,17 +16,35 @@ # under the License. # pylint: disable=too-many-lines import logging +from contextvars import ContextVar from datetime import datetime from io import BytesIO from typing import Any, cast, Optional from zipfile import is_zipfile, ZipFile from flask import current_app, redirect, request, Response, url_for -from flask_appbuilder.api import expose, protect, rison as parse_rison, safe +from flask_appbuilder import permission_name +from flask_appbuilder.api import ( + expose, + merge_response_func, + protect, + rison as parse_rison, + safe, +) +from flask_appbuilder.const import ( + API_DESCRIPTION_COLUMNS_RIS_KEY, + API_LABEL_COLUMNS_RIS_KEY, + API_LIST_COLUMNS_RIS_KEY, + API_LIST_TITLE_RIS_KEY, + API_ORDER_COLUMNS_RIS_KEY, +) from flask_appbuilder.hooks import before_request from flask_appbuilder.models.sqla.interface import SQLAInterface from flask_babel import ngettext from marshmallow import ValidationError +from sqlalchemy import asc, case, desc +from sqlalchemy.orm import Query +from sqlalchemy.orm.util import AliasedClass from werkzeug.wrappers import Response as WerkzeugResponse from werkzeug.wsgi import FileWrapper @@ -46,6 +64,7 @@ ChartTagNameFilter, ) from superset.charts.schemas import ( + chart_get_list_schema, CHART_SCHEMAS, ChartCacheWarmUpRequestSchema, ChartGetResponseSchema, @@ -142,9 +161,49 @@ delete_failed=ChartDeleteFailedError, ) +_viz_type_order: ContextVar[dict[str, int] | None] = ContextVar( + "chart_viz_type_order", default=None +) + + +class ChartSQLAInterface(SQLAInterface): + """Chart model interface with request-scoped display viz type ordering.""" + + def apply_order_by( + self, + query: Query, + order_column: str, + order_direction: str, + aliases_mapping: dict[str, AliasedClass] | None = None, + bypass_many_to_many: bool = False, + add_pk: bool = False, + ) -> Query: + viz_type_order = _viz_type_order.get() + if order_column != "viz_type" or not viz_type_order: + return super().apply_order_by( + query, + order_column, + order_direction, + aliases_mapping=aliases_mapping, + bypass_many_to_many=bypass_many_to_many, + add_pk=add_pk, + ) + + order_expression = case( + viz_type_order, + value=Slice.viz_type, + else_=len(viz_type_order), + ) + direction = asc if order_direction == "asc" else desc + order_by_columns = [direction(order_expression), direction(Slice.viz_type)] + primary_key = self.get_pk() + if add_pk and primary_key is not None: + order_by_columns.append(direction(primary_key)) + return query.order_by(*order_by_columns) + class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): - datamodel = SQLAInterface(Slice) + datamodel = ChartSQLAInterface(Slice) resource_name = "chart" allow_browser_login = True @@ -312,6 +371,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: openapi_spec_component_schemas = CHART_SCHEMAS + (VersionListItemSchema,) apispec_parameter_schemas = { + "chart_get_list_schema": chart_get_list_schema, "screenshot_query_schema": screenshot_query_schema, "get_delete_ids_schema": get_delete_ids_schema, "get_export_ids_schema": get_export_ids_schema, @@ -422,6 +482,88 @@ def pre_get_list(self, data: dict[str, Any]) -> None: if row_id in extra_editors_by_id: row["extra_editors"] = extra_editors_by_id[row_id] + @expose("/", methods=("GET",)) + @protect() + @safe + @permission_name("get") + @parse_rison(chart_get_list_schema) + @merge_response_func( + BaseSupersetModelRestApi.merge_order_columns, API_ORDER_COLUMNS_RIS_KEY + ) + @merge_response_func( + BaseSupersetModelRestApi.merge_list_label_columns, API_LABEL_COLUMNS_RIS_KEY + ) + @merge_response_func( + BaseSupersetModelRestApi.merge_description_columns, + API_DESCRIPTION_COLUMNS_RIS_KEY, + ) + @merge_response_func( + BaseSupersetModelRestApi.merge_list_columns, API_LIST_COLUMNS_RIS_KEY + ) + @merge_response_func( + BaseSupersetModelRestApi.merge_list_title, API_LIST_TITLE_RIS_KEY + ) + def get_list(self, **kwargs: Any) -> Response: + """Get a list of charts. + --- + get: + summary: Get a list of charts + parameters: + - in: query + name: q + description: >- + Rison-encoded list query. viz_type_order may contain up to 256 + unique visualization type slugs, each at most 250 characters, + in the display-name order to use when sorting by viz_type. + content: + application/json: + schema: + $ref: '#/components/schemas/chart_get_list_schema' + responses: + 200: + description: Charts + content: + application/json: + schema: + type: object + properties: + ids: + type: array + items: + type: integer + count: + type: integer + result: + type: array + items: + $ref: >- + #/components/schemas/{{self.__class__.__name__}}.get_list + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + return self.get_list_headless(**kwargs) + + def get_list_headless(self, **kwargs: Any) -> Response: + """Apply client display ordering before list pagination.""" + args = kwargs.get("rison", {}) + viz_types = args.get("viz_type_order") + if args.get("order_column") != "viz_type" or not viz_types: + return super().get_list_headless(**kwargs) + + token = _viz_type_order.set( + {viz_type: index for index, viz_type in enumerate(viz_types)} + ) + try: + return super().get_list_headless(**kwargs) + finally: + _viz_type_order.reset(token) + @expose("//deck_layers/", methods=("GET",)) @protect() @safe diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py index 3c6465fbbaff..458320aac48c 100644 --- a/superset/charts/schemas.py +++ b/superset/charts/schemas.py @@ -20,6 +20,7 @@ from typing import Any, TYPE_CHECKING from flask import current_app +from flask_appbuilder.api.schemas import get_list_schema from flask_babel import gettext as _ from marshmallow import ( EXCLUDE, @@ -113,6 +114,26 @@ def validate_prophet_periods(value: int) -> None: # # RISON/JSON schemas for query parameters # +MAX_VIZ_TYPE_ORDER_LENGTH = 256 +MAX_VIZ_TYPE_LENGTH = 250 + +chart_get_list_schema = { + **get_list_schema, + "properties": { + **get_list_schema["properties"], + "viz_type_order": { + "type": "array", + "items": {"type": "string", "maxLength": MAX_VIZ_TYPE_LENGTH}, + "maxItems": MAX_VIZ_TYPE_ORDER_LENGTH, + "uniqueItems": True, + "description": ( + "Visualization type slugs in display-name order. Used only when " + "order_column is viz_type." + ), + }, + }, +} + get_delete_ids_schema = { "type": "array", "items": {"type": "integer"}, diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index 9338121c3579..71a0aefaa3d0 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -28,6 +28,7 @@ from sqlalchemy import and_ from sqlalchemy.sql import func +from superset.charts.schemas import chart_get_list_schema from superset.commands.chart.data.get_data_command import ChartDataCommand from superset.commands.chart.exceptions import ChartDataQueryFailedError from superset.connectors.sqla.models import SqlaTable @@ -1545,6 +1546,119 @@ def test_get_charts_filter(self): data = json.loads(rv.data.decode("utf-8")) assert data["count"] == 5 + def test_chart_list_openapi_documents_viz_type_order(self): + """Chart API: display ordering is part of the documented list contract.""" + self.login(ADMIN_USERNAME) + rv = self.client.get("api/v1/_openapi") + + assert rv.status_code == 200 + spec = json.loads(rv.data.decode("utf-8")) + query_parameter = spec["paths"]["/api/v1/chart/"]["get"]["parameters"][0] + assert query_parameter["content"]["application/json"]["schema"] == { + "$ref": "#/components/schemas/chart_get_list_schema" + } + viz_type_order = spec["components"]["schemas"]["chart_get_list_schema"][ + "properties" + ]["viz_type_order"] + assert viz_type_order == chart_get_list_schema["properties"]["viz_type_order"] + + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_get_charts_orders_display_viz_types_before_pagination(self): + """Chart API: display chart type ordering happens before pagination.""" + admin = self.get_user("admin") + charts = [ + self.insert_chart("display_type_sort_a", [admin.id], 1, viz_type="slug_a"), + self.insert_chart( + "display_type_sort_middle", [admin.id], 1, viz_type="middle" + ), + self.insert_chart("display_type_sort_z", [admin.id], 1, viz_type="slug_z"), + self.insert_chart( + "display_type_sort_unknown", [admin.id], 1, viz_type="unknown" + ), + ] + self.login(ADMIN_USERNAME) + + arguments = { + "filters": [ + { + "col": "slice_name", + "opr": "sw", + "value": "display_type_sort_", + } + ], + "order_column": "viz_type", + "order_direction": "asc", + "page_size": 2, + "viz_type_order": ["slug_z", "middle", "slug_a"], + } + + try: + for direction, expected in ( + ("asc", ["slug_z", "middle", "slug_a", "unknown"]), + ("desc", ["unknown", "slug_a", "middle", "slug_z"]), + ): + arguments["order_direction"] = direction + pages = [] + for page in (0, 1): + arguments["page"] = page + uri = f"api/v1/chart/?q={rison.dumps(arguments)}" + rv = self.get_assert_metric(uri, "get_list") + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert data["count"] == 4 + pages.extend(item["viz_type"] for item in data["result"]) + + assert pages == expected + + arguments.update( + { + "order_column": "slice_name", + "order_direction": "asc", + "page": 0, + "page_size": 4, + } + ) + uri = f"api/v1/chart/?q={rison.dumps(arguments)}" + rv = self.get_assert_metric(uri, "get_list") + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert [item["slice_name"] for item in data["result"]] == sorted( + chart.slice_name for chart in charts + ) + + arguments.update( + { + "order_column": "viz_type", + "viz_type_order": [], + } + ) + uri = f"api/v1/chart/?q={rison.dumps(arguments)}" + rv = self.get_assert_metric(uri, "get_list") + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert [item["viz_type"] for item in data["result"]] == [ + "middle", + "slug_a", + "slug_z", + "unknown", + ] + + arguments.pop("viz_type_order") + uri = f"api/v1/chart/?q={rison.dumps(arguments)}" + rv = self.get_assert_metric(uri, "get_list") + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert [item["viz_type"] for item in data["result"]] == [ + "middle", + "slug_a", + "slug_z", + "unknown", + ] + finally: + for chart in charts: + db.session.delete(chart) + db.session.commit() + @pytest.fixture def load_energy_charts(self): with app.app_context(): diff --git a/tests/unit_tests/charts/test_schemas.py b/tests/unit_tests/charts/test_schemas.py index 153e8a9734b1..e22d8cb928c1 100644 --- a/tests/unit_tests/charts/test_schemas.py +++ b/tests/unit_tests/charts/test_schemas.py @@ -18,10 +18,13 @@ import pandas as pd import pytest from flask import current_app +from jsonschema import validate as validate_json_schema +from jsonschema.exceptions import ValidationError as JSONSchemaValidationError from marshmallow import ValidationError from pytest_mock import MockerFixture from superset.charts.schemas import ( + chart_get_list_schema, ChartDataAdhocMetricSchema, ChartDataExtrasSchema, ChartDataPostProcessingOperationSchema, @@ -35,9 +38,48 @@ DEFAULT_MAX_PROPHET_PERIODS, get_max_prophet_periods, get_time_grain_choices, + MAX_VIZ_TYPE_LENGTH, + MAX_VIZ_TYPE_ORDER_LENGTH, ) +def test_chart_get_list_schema_accepts_viz_type_display_order() -> None: + validate_json_schema( + instance={ + "order_column": "viz_type", + "viz_type_order": ["slug_z", "slug_a"], + }, + schema=chart_get_list_schema, + ) + validate_json_schema( + instance={"order_column": "viz_type", "viz_type_order": []}, + schema=chart_get_list_schema, + ) + + +@pytest.mark.parametrize( + "viz_type_order", + [ + "slug_a", + [1], + ["slug_a", "slug_a"], + ["a" * (MAX_VIZ_TYPE_LENGTH + 1)], + [f"slug_{index}" for index in range(MAX_VIZ_TYPE_ORDER_LENGTH + 1)], + ], +) +def test_chart_get_list_schema_rejects_invalid_viz_type_display_order( + viz_type_order: object, +) -> None: + with pytest.raises(JSONSchemaValidationError): + validate_json_schema( + instance={ + "order_column": "viz_type", + "viz_type_order": viz_type_order, + }, + schema=chart_get_list_schema, + ) + + def test_get_time_grain_choices(app_context: None) -> None: """Test that get_time_grain_choices returns values with config addons""" # Save original config