Copilot commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3382618177
##########
superset-frontend/src/components/GridTable/types.ts:
##########
@@ -59,4 +59,12 @@ export interface TableProps<RecordType> {
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.
+ */
Review Comment:
The `onServerSort` docstring still claims only the primary sort column is
included due to a single-column API constraint, but `GridTable` now forwards
multi-column sort state to the caller. This comment is misleading and should be
updated to match the new behavior.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1524,10 +1536,13 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
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],
);
Review Comment:
`handleSortByChange` reads `serverPagination` inside the callback, but
`serverPagination` was removed from the dependency array. If `serverPagination`
changes over the component lifecycle, this callback can hold a stale value and
ignore sort changes (or run when it shouldn't).
##########
superset/charts/data/api.py:
##########
@@ -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
Review Comment:
The new sort validation only inspects `queries[0].orderby`. Chart-data
requests can contain multiple query objects; an invalid sort direction in any
query after the first will bypass this check. Consider validating `orderby` for
every query object (and ideally do so before loading the query context to avoid
unnecessary work).
##########
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)],
+ },
Review Comment:
`SamplesPayloadSchema.orderby` says it expects a "list of lists" (JSON), but
the OpenAPI `example` uses Python tuples. This makes the schema docs
inconsistent with the actual request shape clients should send.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
