diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts index bbee0e3ab0bb..ccc82300e9ff 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts @@ -131,13 +131,18 @@ const buildQuery: BuildQuery = ( if (queryMode === QueryMode.Aggregate) { metrics = metrics || []; - // override orderby with timeseries metric when in aggregation mode - if (sortByMetric) { - orderby = [[sortByMetric, !orderDesc]]; - } else if (metrics?.length > 0) { - // default to ordering by first metric in descending order - // when no "sort by" metric is set (regardless if "SORT DESC" is set to true) - orderby = [[metrics[0], false]]; + // Fall back to a metric-based default sort only when no explicit orderby + // was supplied (e.g. a column sort from the "View as table" results pane). + // An explicit orderby from form data takes precedence. + if (orderby.length === 0) { + // override orderby with timeseries metric when in aggregation mode + if (sortByMetric) { + orderby = [[sortByMetric, !orderDesc]]; + } else if (metrics?.length > 0) { + // default to ordering by first metric in descending order + // when no "sort by" metric is set (regardless if "SORT DESC" is set to true) + orderby = [[metrics[0], false]]; + } } // add postprocessing for percent metrics only when in aggregation mode if (percentMetrics && percentMetrics.length > 0) { diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 1b9c77c33eea..08d95402230d 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -1305,6 +1305,18 @@ export default function TableChart( } }} role="columnheader button" + // Expose sort state to assistive technology. Only + // sortable headers advertise aria-sort; the value tracks the + // ascending/descending/none cycle rendered by . + aria-sort={ + col.canSort + ? col.isSorted + ? col.isSortedDesc + ? 'descending' + : 'ascending' + : 'none' + : undefined + } onClick={onClick} data-column-name={col.id} {...(allowRearrangeColumns && { @@ -1524,10 +1536,13 @@ export default function TableChart( const modifiedOwnState = { ...serverPaginationData, sortBy, + // Changing the sort re-queries the full dataset, so the + // previous page offset is meaningless — return to the first page. + currentPage: 0, }; updateTableOwnState(setDataMask, modifiedOwnState); }, - [serverPagination, serverPaginationData, setDataMask], + [serverPaginationData, setDataMask], ); const handleSearch = (searchText: string) => { diff --git a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts index c23a8a49c565..e81b94ef6243 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts @@ -128,13 +128,18 @@ const buildQuery: BuildQuery = ( if (queryMode === QueryMode.Aggregate) { metrics = metrics || []; - // override orderby with timeseries metric when in aggregation mode - if (sortByMetric) { - orderby = [[sortByMetric, !orderDesc]]; - } else if (metrics?.length > 0) { - // default to ordering by first metric in descending order - // when no "sort by" metric is set (regardless if "SORT DESC" is set to true) - orderby = [[metrics[0], false]]; + // Fall back to a metric-based default sort only when no explicit orderby + // was supplied (e.g. a column sort from the "View as table" results pane). + // An explicit orderby from form data takes precedence. + if (orderby.length === 0) { + // override orderby with timeseries metric when in aggregation mode + if (sortByMetric) { + orderby = [[sortByMetric, !orderDesc]]; + } else if (metrics?.length > 0) { + // default to ordering by first metric in descending order + // when no "sort by" metric is set (regardless if "SORT DESC" is set to true) + orderby = [[metrics[0], false]]; + } } // add postprocessing for percent metrics only when in aggregation mode type PercentMetricCalculationMode = 'row_limit' | 'all_records'; diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 474298599fdf..d23af55ff341 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -2457,3 +2457,26 @@ describe('Drill-to-Detail Temporal Range Logic', () => { expect(filter.val).toBeNull(); }); }); + +// Server-side full sort: accessibility of sortable headers. +// Every sortable column header must expose its sort state via +// aria-sort, and the value must track the asc/desc/none cycle. +const VALID_ARIA_SORT = ['none', 'ascending', 'descending']; + +test('sortable column headers expose and update aria-sort (F3-T1, NFR-ACC-01)', () => { + render(); + + const headerCell = screen.getByText('Sum of Num').closest('th') as HTMLElement; + expect(headerCell).toBeInTheDocument(); + + // A sortable header must advertise a valid aria-sort token (the exact + // initial value depends on any default sort in formData, so accept any). + const initial = headerCell.getAttribute('aria-sort'); + expect(VALID_ARIA_SORT).toContain(initial); + + // Clicking the header changes the sort, and aria-sort tracks that change. + fireEvent.click(headerCell); + const afterClick = headerCell.getAttribute('aria-sort'); + expect(VALID_ARIA_SORT).toContain(afterClick); + expect(afterClick).not.toEqual(initial); +}); diff --git a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx index 98e3b46d9adc..b3ba576e7e1e 100644 --- a/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx +++ b/superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx @@ -45,6 +45,9 @@ import { EmptyState, Loading } from '@superset-ui/core/components'; import { getDatasourceSamples } from 'src/components/Chart/chartAction'; import Table, { ColumnsType, + ETableAction, + SorterResult, + SortOrder, TableSize, } from '@superset-ui/core/components/Table'; import { RootState } from 'src/dashboard/types'; @@ -94,6 +97,8 @@ export default function DrillDetailPane({ const theme = useTheme(); const [pageIndex, setPageIndex] = useState(0); const [pageSize, setPageSize] = useState(DEFAULT_PAGE_SIZE); + const [sortColumn, setSortColumn] = useState(); + const [sortOrder, setSortOrder] = useState(null); const lastPageIndex = useRef(pageIndex); const [filters, setFilters] = useState(initialFilters); const [isLoading, setIsLoading] = useState(false); @@ -147,6 +152,10 @@ export default function DrillDetailPane({ resultsPage?.colNames.map((column, index) => ({ key: column, dataIndex: column, + // Server-side sort: antd only renders the indicator and fires onChange; + // the sorted page is fetched from the server (see `orderby`). + sorter: true, + sortOrder: sortColumn === column ? sortOrder : null, title: resultsPage?.colTypes[index] === GenericDataType.Temporal ? ( ( + () => + sortColumn && sortOrder ? [[sortColumn, sortOrder === 'ascend']] : [], + [sortColumn, sortOrder], + ); + // Clear cache on reload button click const handleReload = useCallback(() => { setResponseError(''); @@ -306,7 +326,10 @@ export default function DrillDetailPane({ useEffect(() => { if (!responseError && !isLoading && !resultsPages.has(pageIndex)) { setIsLoading(true); - const jsonPayload = getDrillPayload(formData, filters) ?? {}; + const jsonPayload = { + ...getDrillPayload(formData, filters), + ...(orderby.length > 0 && { orderby }), + }; const cachePageLimit = Math.ceil(SAMPLES_ROW_LIMIT / pageSize); getDatasourceSamples( datasourceType as DatasourceType, @@ -348,6 +371,7 @@ export default function DrillDetailPane({ filters, formData, isLoading, + orderby, pageIndex, pageSize, responseError, @@ -389,7 +413,22 @@ export default function DrillDetailPane({ recordCount={resultsPage?.total} usePagination loading={isLoading} - onChange={pagination => { + onChange={(pagination, _filters, sorter, extra) => { + if (extra?.action === ETableAction.Sort) { + const nextSorter = ( + Array.isArray(sorter) ? sorter[0] : sorter + ) as SorterResult; + const column = nextSorter?.columnKey as string | undefined; + const order = nextSorter?.order ?? null; + // antd cycles ascend -> descend -> unsorted; clear when unsorted. + setSortColumn(order ? column : undefined); + setSortOrder(order); + // Cached pages reflect the previous sort, so drop them and restart + // from the first page with the new ordering. + setResultsPages(new Map()); + setPageIndex(0); + return; + } const newPageSize = pagination.pageSize ?? pageSize; if (newPageSize !== pageSize) { setPageSize(newPageSize); diff --git a/superset-frontend/src/components/GridTable/index.tsx b/superset-frontend/src/components/GridTable/index.tsx index cafe7c677364..fbf96d2160f1 100644 --- a/superset-frontend/src/components/GridTable/index.tsx +++ b/superset-frontend/src/components/GridTable/index.tsx @@ -32,9 +32,6 @@ const gridComponents = { agColumnHeader: Header, }; -const onSortChanged: AgGridReactProps['onSortChanged'] = ({ api }) => - api.refreshCells(); - export function GridTable({ data, columns, @@ -46,8 +43,28 @@ export function GridTable({ enableActions, size = GridSize.Middle, striped, + onServerSort, }: TableProps) { const theme = useTheme(); + const onSortChanged = useCallback< + NonNullable + >( + ({ api }) => { + api.refreshCells(); + if (!onServerSort) { + return; + } + const orderby = api + .getColumnState() + .filter(state => state.sort) + // Order the columns by the sequence in which the user applied each + // sort so multi-column sort reaches the server in the right priority. + .sort((a, b) => (a.sortIndex ?? 0) - (b.sortIndex ?? 0)) + .map(state => [state.colId, state.sort === 'asc'] as [string, boolean]); + onServerSort(orderby); + }, + [onServerSort], + ); const isExternalFilterPresent = useCallback( () => Boolean(externalFilter), [externalFilter], diff --git a/superset-frontend/src/components/GridTable/types.ts b/superset-frontend/src/components/GridTable/types.ts index 8dda781f6cff..2b93f0cf340d 100644 --- a/superset-frontend/src/components/GridTable/types.ts +++ b/superset-frontend/src/components/GridTable/types.ts @@ -59,4 +59,12 @@ export interface TableProps { usePagination?: boolean; striped?: boolean; + + /** + * Called when the sort state changes, with it translated to a query + * `orderby` (`[[columnId, isAscending]]`) so the consumer can re-request + * server-sorted data. Only the primary sort column is included, matching the + * chart data API's single-column sort constraint. + */ + onServerSort?: (orderby: [string, boolean][]) => void; } diff --git a/superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx b/superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx index 8cd279f41643..2a96926f5053 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx @@ -60,6 +60,8 @@ export const SingleQueryResultPane = ({ onDownloadCSV, onDownloadXLSX, onReload, + onServerSort, + isLoading = false, }: SingleQueryResultPaneProp) => { const [filterText, setFilterText] = useState(''); const { gridHeight, measuredRef } = useGridHeight(); @@ -81,7 +83,7 @@ export const SingleQueryResultPane = ({ rowcount={rowcount} datasourceId={datasourceId} onInputChange={handleInputChange} - isLoading={false} + isLoading={isLoading} canDownload={canDownload} rowLimit={rowLimit} rowLimitOptions={rowLimitOptions} @@ -99,6 +101,7 @@ export const SingleQueryResultPane = ({ size={GridSize.Small} externalFilter={keywordFilter} showRowNumber + onServerSort={onServerSort} /> diff --git a/superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx b/superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx index 302be4ea51bc..1b8552dfcfc5 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx @@ -23,10 +23,12 @@ import { ensureIsArray, getChartMetadataRegistry, getClientErrorObject, + QueryFormData, } from '@superset-ui/core'; import { styled } from '@apache-superset/core/theme'; import { EmptyState, Loading } from '@superset-ui/core/components'; import { getChartDataRequest } from 'src/components/Chart/chartAction'; +import { buildV1ChartDataPayload } from 'src/explore/exploreUtils'; import { ResultsPaneProps, QueryResultInterface } from '../types'; import { SingleQueryResultPane } from './SingleQueryResultPane'; import { TableControls, ROW_LIMIT_OPTIONS } from './DataTableControls'; @@ -62,6 +64,13 @@ export const useResultsPane = ({ const chartRowLimit = Number(queryFormData?.row_limit) || 10000; const [rowLimit, setRowLimit] = useState(1000); + const [orderby, setOrderby] = useState<[string, boolean][]>([]); + // Server-side sort is only valid when the displayed columns map directly to + // the SQL result. When the query has post-processing (e.g. pivot/cum/rolling), + // `orderby` + `row_limit` are applied to the raw SQL *before* post-processing, + // which changes the rows that feed those operations and corrupts the result. + // In that case we fall back to client-side sorting of what the chart produced. + const [hasPostProcessing, setHasPostProcessing] = useState(false); const [resultResp, setResultResp] = useState([]); const [isLoading, setIsLoading] = useState(true); const [responseError, setResponseError] = useState(''); @@ -74,8 +83,14 @@ export const useResultsPane = ({ const effectiveRowLimit = Math.min(rowLimit, chartRowLimit); const cappedFormData = useMemo( - () => ({ ...queryFormData, row_limit: effectiveRowLimit }), - [queryFormData, effectiveRowLimit], + () => ({ + ...queryFormData, + row_limit: effectiveRowLimit, + // A new `orderby` produces a new object, missing the cache below and + // triggering a server-side re-query in the sorted order. + ...(orderby.length > 0 && { orderby }), + }), + [queryFormData, effectiveRowLimit, orderby], ); const handleRowLimitChange = useCallback( @@ -86,6 +101,10 @@ export const useResultsPane = ({ [cappedFormData], ); + const handleServerSort = useCallback((nextOrderby: [string, boolean][]) => { + setOrderby(nextOrderby); + }, []); + useEffect(() => { // it's an invalid formData when gets a errorMessage if (errorMessage) return; @@ -133,7 +152,44 @@ export const useResultsPane = ({ } }, [errorMessage]); - if (isLoading) { + // Detect whether the chart's query uses post-processing so server-side sort + // can be disabled for it. Building the payload is query construction only + // (no network), and the result does not depend on `orderby`, so it is keyed + // on the base form data rather than `cappedFormData`. + useEffect(() => { + let cancelled = false; + buildV1ChartDataPayload({ + formData: queryFormData as QueryFormData, + force: false, + resultFormat: 'json', + resultType: 'results', + }) + .then(payload => { + if (!cancelled) { + setHasPostProcessing( + ensureIsArray(payload?.queries).some( + query => (query?.post_processing?.length ?? 0) > 0, + ), + ); + } + }) + .catch(() => { + // If the payload can't be built, fall back to disabling server sort to + // avoid producing incorrect results. + if (!cancelled) { + setHasPostProcessing(true); + } + }); + return () => { + cancelled = true; + }; + }, [queryFormData]); + + // Only replace the whole pane with a loader on the initial fetch. On a + // re-sort/refetch we keep the existing pane mounted (with stale rows) so the + // grid does not remount and lose its sort state; the loading state is surfaced + // in-place instead. + if (isLoading && resultResp.length === 0) { return Array(queryCount).fill(); } @@ -187,6 +243,16 @@ export const useResultsPane = ({ rowLimit={rowLimit} rowLimitOptions={ROW_LIMIT_OPTIONS} onRowLimitChange={handleRowLimitChange} + isLoading={isLoading} + onServerSort={ + // Client-side sort is exact when the full result fits within the row + // limit, so only re-query when the rows were truncated. Post-processed + // queries are always sorted client-side, since server-side `orderby` + // would change the rows feeding post-processing (see hasPostProcessing). + !hasPostProcessing && result.rowcount >= effectiveRowLimit + ? handleServerSort + : undefined + } /> )); diff --git a/superset-frontend/src/explore/components/DataTablesPane/types.ts b/superset-frontend/src/explore/components/DataTablesPane/types.ts index 6135bb6fe38e..9869c510a97e 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/types.ts +++ b/superset-frontend/src/explore/components/DataTablesPane/types.ts @@ -102,4 +102,10 @@ export interface SingleQueryResultPaneProp rowLimit?: number; rowLimitOptions?: { value: number; label: string }[]; onRowLimitChange?: (limit: number) => void; + // Re-request data sorted server-side; omitted when the result fits within the + // row limit and client-side sorting is already exact. + onServerSort?: (orderby: [string, boolean][]) => void; + // True while a re-sort/refetch is in flight; surfaced in-place so the grid + // stays mounted (and keeps its sort state) instead of being replaced. + isLoading?: boolean; } diff --git a/superset/charts/data/api.py b/superset/charts/data/api.py index 0be87a4f8b37..1c9fee75ca7f 100644 --- a/superset/charts/data/api.py +++ b/superset/charts/data/api.py @@ -21,7 +21,7 @@ from datetime import datetime from typing import Any, Callable, TYPE_CHECKING -from flask import current_app as app, g, make_response, request, Response +from flask import current_app as app, g, jsonify, make_response, request, Response from flask_appbuilder.api import expose, protect from flask_babel import gettext as _ from marshmallow import ValidationError @@ -75,6 +75,53 @@ logger = logging.getLogger(__name__) +def validate_sort_params( + orderby: list[Any], +) -> Response | None: + """ + Validate sort parameters before a query is executed. + + Every ``orderby`` entry must have a boolean sort direction. Multiple sort + columns are supported. + + Returns a Flask ``Response`` (HTTP 400) when validation fails, or ``None`` + when the parameters are valid. Callers must return a non-``None`` result + immediately, before constructing ``ChartDataCommand``. + """ + if not orderby: + return None + + for column_name, is_ascending in orderby: + if not isinstance(is_ascending, bool): + return make_response( + jsonify( + { + "message": _( + "Sort direction for column '%(column)s' must be a boolean", + column=column_name, + ), + "errors": [ + { + "error_type": "SORT_DIRECTION_INVALID", + "message": _( + "Expected bool for sort direction, got %(type)s", + type=type(is_ascending).__name__, + ), + "level": "error", + "extra": { + "column": column_name, + "direction_value": is_ascending, + }, + } + ], + } + ), + 400, + ) + + return None + + class ChartDataRestApi(ChartRestApi): include_route_methods = {"get_data", "data", "data_from_cache"} @@ -343,6 +390,16 @@ def data( # noqa: C901 try: query_context = self._create_query_context_from_form(json_body) + # Validate sort parameters before executing the query so bad sort + # directions are rejected early, before the query builder runs. + orderby = ( + json_body.get("queries", [{}])[0].get("orderby", []) + if isinstance(json_body, dict) and json_body.get("queries") + else [] + ) + sort_error = validate_sort_params(orderby) + if sort_error is not None: + return sort_error command = ChartDataCommand(query_context) command.validate() except DatasourceNotFound: diff --git a/superset/common/query_actions.py b/superset/common/query_actions.py index 5479e12d880a..501d2369951a 100644 --- a/superset/common/query_actions.py +++ b/superset/common/query_actions.py @@ -242,7 +242,11 @@ def _get_drill_detail( else: qry_obj_cols.append(o.column_name) query_obj.columns = qry_obj_cols - query_obj.orderby = [(query_obj.columns[0], True)] + # Preserve a caller-supplied sort (e.g. clicking a column header in the + # drill-detail table). Only fall back to ordering by the first column when + # no orderby was provided. + if not query_obj.orderby: + query_obj.orderby = [(query_obj.columns[0], True)] return _get_full(query_context, query_obj, force_cached) diff --git a/superset/views/datasource/schemas.py b/superset/views/datasource/schemas.py index dedab22731f2..b7302e5f05f3 100644 --- a/superset/views/datasource/schemas.py +++ b/superset/views/datasource/schemas.py @@ -83,6 +83,22 @@ class SamplesPayloadSchema(Schema): metadata={"description": "Extra parameters to add to the query."}, allow_none=True, ) + orderby = fields.List( + fields.Tuple( + ( + fields.Raw(allow_none=False), + fields.Boolean(), + ) + ), + required=False, + allow_none=True, + metadata={ + "description": "Expects a list of lists where the first element is the " + "column name to sort by, and the second element is a boolean " + "(true = ascending).", + "example": [("my_col_1", False), ("my_col_2", True)], + }, + ) @pre_load # pylint: disable=unused-argument diff --git a/superset/views/datasource/utils.py b/superset/views/datasource/utils.py index df974e696e45..d6c446772d35 100644 --- a/superset/views/datasource/utils.py +++ b/superset/views/datasource/utils.py @@ -158,7 +158,16 @@ def get_samples( # pylint: disable=too-many-arguments "type": datasource.type, "id": datasource.id, }, - queries=[{**payload, **count_star_metric} if payload else count_star_metric], + # `orderby` is omitted from the count query: ordering a bare COUNT(*) + # by a data column is meaningless and errors on stricter databases. + queries=[ + { + **{key: value for key, value in payload.items() if key != "orderby"}, + **count_star_metric, + } + if payload + else count_star_metric + ], form_data=form_data, result_type=ChartDataResultType.FULL, force=force, diff --git a/tests/integration_tests/datasource_tests.py b/tests/integration_tests/datasource_tests.py index b9da228ea3c8..a061668e6af7 100644 --- a/tests/integration_tests/datasource_tests.py +++ b/tests/integration_tests/datasource_tests.py @@ -647,6 +647,37 @@ def test_get_samples(test_client, login_as_admin, virtual_dataset): assert eager_samples == rv2.json["result"]["data"] +def test_get_samples_with_orderby(test_client, login_as_admin, virtual_dataset): + """ + /datasource/samples should honor a caller-supplied ``orderby``, sorting the + returned rows server-side (drill-detail header-click sort). ``col1`` holds + the distinct values 0..9. + """ + uri = ( + f"/datasource/samples?datasource_id={virtual_dataset.id}&datasource_type=table" + ) + + # Descending by col1 -> 9..0. This also proves the caller's orderby + # overrides the default first-column-ascending ordering. + rv_desc = test_client.post(uri, json={"orderby": [["col1", False]]}) + assert rv_desc.status_code == 200 + col1_desc = [row["col1"] for row in rv_desc.json["result"]["data"]] + assert col1_desc == sorted(col1_desc, reverse=True) + assert col1_desc[0] == 9 + + # Ascending by col1 -> 0..9. + rv_asc = test_client.post(uri, json={"orderby": [["col1", True]]}) + assert rv_asc.status_code == 200 + col1_asc = [row["col1"] for row in rv_asc.json["result"]["data"]] + assert col1_asc == sorted(col1_asc) + assert col1_asc[0] == 0 + + # The two orderings differ and produce distinct cache keys, confirming + # `orderby` is part of the query rather than ignored. + assert col1_desc != col1_asc + assert rv_desc.json["result"]["cache_key"] != rv_asc.json["result"]["cache_key"] + + def test_get_samples_with_incorrect_cc(test_client, login_as_admin, virtual_dataset): if get_example_database().backend == "sqlite": return diff --git a/tests/unit_tests/charts/data/__init__.py b/tests/unit_tests/charts/data/__init__.py new file mode 100644 index 000000000000..13a83393a912 --- /dev/null +++ b/tests/unit_tests/charts/data/__init__.py @@ -0,0 +1,16 @@ +# 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. diff --git a/tests/unit_tests/charts/data/test_api.py b/tests/unit_tests/charts/data/test_api.py new file mode 100644 index 000000000000..76bcf333efdc --- /dev/null +++ b/tests/unit_tests/charts/data/test_api.py @@ -0,0 +1,56 @@ +# 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. +"""Unit tests for the chart data API sort-parameter validator. + +The autouse ``app_context`` fixture in +``tests/unit_tests/conftest.py`` supplies the Flask application context that +``flask.jsonify`` / ``flask.make_response`` require. +""" + +from superset.charts.data.api import validate_sort_params + + +def test_validate_sort_params_no_orderby() -> None: + # Empty orderby passes (returns None) + assert validate_sort_params([]) is None + + +def test_validate_sort_params_valid() -> None: + # Single entry with a bool direction passes + assert validate_sort_params([("a", True)]) is None + + +def test_validate_sort_params_multi_column() -> None: + # Multiple columns with bool directions pass + assert validate_sort_params([("a", True), ("b", False)]) is None + + +def test_validate_sort_params_invalid_direction() -> None: + # Non-boolean direction -> 400 + resp = validate_sort_params([("a", "asc")]) + assert resp is not None + assert resp.status_code == 400 + assert resp.json["errors"][0]["error_type"] == "SORT_DIRECTION_INVALID" + + +def test_validate_sort_params_multi_column_validates_each_entry() -> None: + # Every entry is validated; a bad direction anywhere in the list -> 400 + resp = validate_sort_params([("a", True), ("b", "desc")]) + assert resp is not None + assert resp.status_code == 400 + assert resp.json["errors"][0]["error_type"] == "SORT_DIRECTION_INVALID" + assert resp.json["errors"][0]["extra"]["column"] == "b" diff --git a/tests/unit_tests/common/test_query_actions.py b/tests/unit_tests/common/test_query_actions.py new file mode 100644 index 000000000000..1ba112f69f29 --- /dev/null +++ b/tests/unit_tests/common/test_query_actions.py @@ -0,0 +1,72 @@ +# 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. +"""Unit tests for superset.common.query_actions.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from superset.common import query_actions + + +def _make_query_obj(orderby: list) -> SimpleNamespace: + """A minimal stand-in for QueryObject (so copy.copy behaves predictably).""" + return SimpleNamespace( + orderby=orderby, + is_timeseries=True, + metrics=["a_metric"], + post_processing=["an_op"], + columns=[], + ) + + +def _make_datasource(column_names: list[str]) -> SimpleNamespace: + return SimpleNamespace( + columns=[SimpleNamespace(column_name=name) for name in column_names] + ) + + +@patch("superset.common.query_actions._get_full") +@patch("superset.common.query_actions._get_datasource") +def test_get_drill_detail_preserves_supplied_orderby( + mock_get_datasource: MagicMock, + mock_get_full: MagicMock, +) -> None: + """A caller-supplied orderby (e.g. column-header sort) must be preserved.""" + mock_get_datasource.return_value = _make_datasource(["order_date", "sales"]) + query_obj = _make_query_obj([("sales", False)]) + + query_actions._get_drill_detail(MagicMock(), query_obj, force_cached=False) + + # _get_full(query_context, query_obj, force_cached) -> inspect the query_obj. + passed_query_obj = mock_get_full.call_args.args[1] + assert passed_query_obj.orderby == [("sales", False)] + + +@patch("superset.common.query_actions._get_full") +@patch("superset.common.query_actions._get_datasource") +def test_get_drill_detail_defaults_orderby_to_first_column( + mock_get_datasource: MagicMock, + mock_get_full: MagicMock, +) -> None: + """With no orderby supplied, it falls back to the first column ascending.""" + mock_get_datasource.return_value = _make_datasource(["order_date", "sales"]) + query_obj = _make_query_obj([]) + + query_actions._get_drill_detail(MagicMock(), query_obj, force_cached=False) + + passed_query_obj = mock_get_full.call_args.args[1] + assert passed_query_obj.orderby == [("order_date", True)] diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py index c93f6121dbd5..cff1ea6b8df9 100644 --- a/tests/unit_tests/models/helpers_test.py +++ b/tests/unit_tests/models/helpers_test.py @@ -2706,6 +2706,163 @@ def test_format_time_humanized_skips_activation_for_english( mock_activate.assert_not_called() +def _orderby_sql(database: Database, orderby: list) -> str: + """Build a SqlaTable query with the given orderby and return compiled SQL. + + Shared by the tests below. The column is resolved through + the dataset's ``columns`` ORM relationship (no raw SQL interpolation). + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a", type="INTEGER"), + TableColumn(column_name="b", type="TEXT"), + ], + ) + sqla_query = table.get_sqla_query( + columns=["a", "b"], + filter=[], + extras={}, + is_timeseries=False, + metrics=[], + orderby=orderby, + ) + with database.get_sqla_engine() as engine: + return str( + sqla_query.sqla_query.compile( + dialect=engine.dialect, + compile_kwargs={"literal_binds": True}, + ) + ).upper() + + +def test_orderby_ascending_applies_order_by_clause(database: Database) -> None: + """ + A validated single-column ``orderby`` is applied as ``ORDER BY ... ASC`` by + the existing ORM query builder. The column comes from the ``dataset.columns`` + relationship, confirming the backend already supports server-side sort once + ``orderby`` is populated (ORM-resolved, no raw SQL). + """ + sql = _orderby_sql(database, [("a", True)]) + assert "ORDER BY" in sql + assert "ASC" in sql + + +def test_orderby_descending_applies_order_by_clause(database: Database) -> None: + """Descending direction yields ``ORDER BY ... DESC``.""" + sql = _orderby_sql(database, [("a", False)]) + assert "ORDER BY" in sql + assert "DESC" in sql + + +def test_orderby_absent_no_order_by_clause(database: Database) -> None: + """With empty orderby, no ORDER BY clause is added, + so existing (unsorted) query behaviour is unchanged.""" + sql = _orderby_sql(database, []) + assert "ORDER BY" not in sql + + +def test_orderby_compiles_across_dialects(database: Database) -> None: + """The ORDER BY clause is emitted by SQLAlchemy per + dialect rather than hardcoded. Compile the same sorted query against the + PostgreSQL and MySQL dialects and confirm each produces ORDER BY ... DESC. + """ + from sqlalchemy.dialects import mysql, postgresql + + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a", type="INTEGER"), + TableColumn(column_name="b", type="TEXT"), + ], + ) + sqla_query = table.get_sqla_query( + columns=["a", "b"], + filter=[], + extras={}, + is_timeseries=False, + metrics=[], + orderby=[("a", False)], + ) + for dialect in (postgresql.dialect(), mysql.dialect()): + sql = str( + sqla_query.sqla_query.compile( + dialect=dialect, + compile_kwargs={"literal_binds": True}, + ) + ).upper() + assert "ORDER BY" in sql, f"missing ORDER BY for {dialect.name}" + assert "DESC" in sql, f"missing DESC for {dialect.name}" + + +def test_orderby_applied_after_where_filter(database: Database) -> None: + """The ORDER BY clause is appended after the WHERE + filters, so filtering (incl. RLS, which uses the same WHERE stage) is not + affected by the sort. Asserts both clauses are present and ordered. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a", type="INTEGER"), + TableColumn(column_name="b", type="TEXT"), + ], + ) + sqla_query = table.get_sqla_query( + columns=["a", "b"], + filter=[{"col": "a", "op": "==", "val": 1}], + extras={}, + is_timeseries=False, + metrics=[], + orderby=[("a", True)], + ) + with database.get_sqla_engine() as engine: + sql = str( + sqla_query.sqla_query.compile( + dialect=engine.dialect, + compile_kwargs={"literal_binds": True}, + ) + ).upper() + assert "WHERE" in sql + assert "ORDER BY" in sql + assert sql.index("WHERE") < sql.index("ORDER BY") + + +def test_orderby_unknown_column_raises(database: Database) -> None: + """If the orderby column cannot be resolved on the + datasource the builder raises QueryObjectValidationError rather than + silently dropping the sort or falling back to unsafe SQL. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.exceptions import QueryObjectValidationError + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[TableColumn(column_name="a", type="INTEGER")], + ) + with pytest.raises(QueryObjectValidationError): + table.get_sqla_query( + columns=["a"], + filter=[], + extras={}, + is_timeseries=False, + metrics=[], + orderby=[("does_not_exist", True)], + ) + # ----------------------------------------------------------------------------- # _process_sql_expression denylist gate (adhoc expression validation) # ----------------------------------------------------------------------------- diff --git a/tests/unit_tests/views/datasource/utils_test.py b/tests/unit_tests/views/datasource/utils_test.py index 4788fea92faf..3e22f91b3267 100644 --- a/tests/unit_tests/views/datasource/utils_test.py +++ b/tests/unit_tests/views/datasource/utils_test.py @@ -215,3 +215,67 @@ def test_get_samples_count_star_access_denied(mock_get_limit_clause: MagicMock): mock_samples_context.raise_for_access.assert_called_once() # Verify count context was also checked mock_count_context.raise_for_access.assert_called_once() + + +@patch("superset.views.datasource.utils.get_limit_clause") +def test_get_samples_forwards_orderby_to_data_query_only( + mock_get_limit_clause: MagicMock, +): + """ + get_samples() should forward a caller-supplied ``orderby`` to the drill-detail + data query, but omit it from the COUNT(*) query (ordering a bare count by a + data column is meaningless and errors on stricter databases). + """ + mock_get_limit_clause.return_value = {"row_offset": 0, "row_limit": 100} + + mock_datasource = MagicMock() + mock_datasource.type = "table" + mock_datasource.id = 1 + mock_datasource.columns = [] + + mock_samples_context = MagicMock() + mock_count_context = MagicMock() + mock_samples_context.raise_for_access.return_value = None + mock_count_context.raise_for_access.return_value = None + mock_count_context.get_payload.return_value = { + "queries": [{"data": [{"COUNT(*)": 1}], "status": "success"}] + } + mock_samples_context.get_payload.return_value = { + "queries": [{"data": [{"col1": "val1"}], "status": "success", "cache_key": "k"}] + } + + orderby = [["col1", True]] + + with ( + patch( + "superset.views.datasource.utils.DatasourceDAO.get_datasource", + return_value=mock_datasource, + ), + patch( + "superset.views.datasource.utils.QueryContextFactory" + ) as mock_factory_class, + ): + mock_factory = MagicMock() + mock_factory_class.return_value = mock_factory + mock_factory.create.side_effect = [mock_samples_context, mock_count_context] + + from superset.views.datasource.utils import get_samples + + get_samples( + datasource_type="table", + datasource_id=1, + force=False, + page=1, + per_page=100, + payload={"filters": [], "orderby": orderby}, + ) + + # The samples (drill-detail) context is created first, the COUNT(*) second. + samples_call, count_call = mock_factory.create.call_args_list + samples_query = samples_call.kwargs["queries"][0] + count_query = count_call.kwargs["queries"][0] + + # The data query carries the caller's orderby... + assert samples_query["orderby"] == orderby + # ...while the COUNT(*) query does not. + assert "orderby" not in count_query