codeant-ai-for-open-source[bot] commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3382279501
##########
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:
Review Comment:
**Suggestion:** The new sort-direction validation only inspects
`queries[0]`, so any additional query objects in the same request bypass this
strict boolean check. This creates inconsistent behavior where malformed sort
directions in later queries are silently accepted (or coerced) instead of being
rejected. Validate `orderby` for every entry in `json_body["queries"]`, not
just the first one. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Multi-query chart requests have inconsistent sort validation.
- ⚠️ Later queries can carry non-boolean sort directions.
- ⚠️ Downstream SQL builders see unexpected `orderby` value types.
- ⚠️ API contract "boolean sort direction" only partially enforced.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note that the Chart Data endpoint `ChartDataRestApi.data` in
`superset/charts/data/api.py:93-200` accepts a query context JSON body where
`"queries"`
is documented as "one or many query objects" (docstring lines 115–116).
2. Observe that this method computes `orderby` using only the first query: at
`superset/charts/data/api.py:152-161`, it assigns `orderby =
(json_body.get("queries",
[{}])[0].get("orderby", []) if isinstance(json_body, dict) and
json_body.get("queries")
else [])` and then calls `sort_error = validate_sort_params(orderby)`.
3. Note that the query context is built from *all* query objects:
`QueryContextFactory.create` in
`superset/common/query_context_factory.py:45-89` iterates
over every element in the incoming `queries` list and passes each
`query_obj` dict
(including its `orderby`) into `QueryObjectFactory.create`, producing a list
of
`QueryObject` instances assigned to `QueryContext.queries`.
4. Craft a POST request to the `api/v1/chart/data` endpoint (which is wired
to
`ChartDataRestApi.data` via `ChartRestApi` in `superset/charts/api.py`) with
a JSON body
like:
`{"datasource": {...}, "queries": [{"orderby": [("metric1", true)]},
{"orderby":
[("metric2", "asc")]}, ...], "result_type": "full", "form_data": {...}}`.
The first
query's `orderby` uses a valid boolean direction, while the second query
uses the
invalid string `"asc"`.
5. When this request reaches `ChartDataRestApi.data`, the first query's
`orderby` list is
extracted and passed to `validate_sort_params`, which returns `None` because
all
directions in that list are booleans (see
`superset/tests/unit_tests/charts/data/test_api.py:13-20` where valid
boolean directions
are accepted). No validation is ever performed on the second query's
`orderby` list, so
the non-boolean `"asc"` is never rejected.
6. The method then constructs a `ChartDataCommand` with a `QueryContext`
that includes
*all* queries, including the second query whose `orderby` contains a
non-boolean
direction. Since `QueryObject` in `superset/common/query_object.py:112-152`
simply assigns
`self.orderby = orderby or []` without type-checking, this malformed sort
directive is
accepted and propagated, meaning later queries bypass the sort-direction
validation that
the new helper `validate_sort_params` was meant to enforce.
7. This leads to inconsistent behavior: malformed sort directions in the
first query
result in an immediate HTTP 400 from `validate_sort_params`, while the same
malformed
directions in any subsequent query are silently accepted and only expressed
(as
mis-sorting or backend errors) deeper in the query pipeline instead of being
rejected
uniformly at the API boundary.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c2926952bd3437ea230a2867b663a77&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5c2926952bd3437ea230a2867b663a77&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 397:401
**Comment:**
*Incomplete Implementation: The new sort-direction validation only
inspects `queries[0]`, so any additional query objects in the same request
bypass this strict boolean check. This creates inconsistent behavior where
malformed sort directions in later queries are silently accepted (or coerced)
instead of being rejected. Validate `orderby` for every entry in
`json_body["queries"]`, not just the first one.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=00dfbd9537da094439901b531951d30c9af46addc1d03ee88f016da1d501b153&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=00dfbd9537da094439901b531951d30c9af46addc1d03ee88f016da1d501b153&reaction=dislike'>👎</a>
--
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]
