codeant-ai-for-open-source[bot] commented on code in PR #37371:
URL: https://github.com/apache/superset/pull/37371#discussion_r3510852523
##########
superset/connectors/sqla/models.py:
##########
@@ -1687,9 +1690,13 @@ def adhoc_metric_to_sqla(
)
else:
sqla_column = column(column_name)
- sqla_metric =
self.sqla_aggregations[metric["aggregate"]](sqla_column)
+ sqla_metric = self.sqla_aggregations[aggregate](sqla_column)
elif expression_type == utils.AdhocMetricExpressionType.SQL:
- expression = metric.get("sqlExpression")
+ expression: str | None = metric.get("sqlExpression")
+ if not isinstance(expression, str) or not expression:
Review Comment:
**Suggestion:** The SQL-expression validation accepts whitespace-only
strings, so obviously invalid expressions pass this guard and fail later during
SQL generation/execution. Trim the string (or check `expression.strip()`) so
blank SQL is rejected early with `QueryObjectValidationError`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart data API accepts clearly invalid blank SQL metrics.
- ⚠️ Users see backend SQL errors instead of validation feedback.
- ⚠️ Embedded dashboards may fail on crafted whitespace metrics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a chart query request to the chart data endpoint (e.g., POST
/api/v1/chart/data)
that builds a query using `SqlaTable.adhoc_metric_to_sqla` in
`superset/connectors/sqla/models.py:1678-1700`.
2. In the request JSON, include an adhoc metric with `"expressionType":
"SQL"` and set
`"sqlExpression"` to a whitespace-only string, for example `"sqlExpression":
" "` while
still marking the metric as active.
3. During processing, `adhoc_metric_to_sqla` reads `expression` at
`superset/connectors/sqla/models.py:1695` and evaluates the guard `if not
isinstance(expression, str) or not expression:` at line 1696; the
whitespace-only string
passes because it is a non-empty `str`.
4. The function proceeds to build a SQLAlchemy expression from the blank SQL
later in the
function, leading to a malformed or empty SQL fragment and a downstream
SQLAlchemy/DB
error during query execution rather than an early, clear
`QueryObjectValidationError` at
validation time.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a9e8734ce9547d381a6ed539277f201&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3a9e8734ce9547d381a6ed539277f201&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/connectors/sqla/models.py
**Line:** 1695:1696
**Comment:**
*Logic Error: The SQL-expression validation accepts whitespace-only
strings, so obviously invalid expressions pass this guard and fail later during
SQL generation/execution. Trim the string (or check `expression.strip()`) so
blank SQL is rejected early with `QueryObjectValidationError`.
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%2F37371&comment_hash=cde1434bf512448cd6282599f58d35a2767c5ad3ef71acb88daf3a3e89a89979&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=cde1434bf512448cd6282599f58d35a2767c5ad3ef71acb88daf3a3e89a89979&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1678,6 +1678,9 @@ def adhoc_metric_to_sqla(
label = utils.get_metric_name(metric, self.verbose_map)
if expression_type == utils.AdhocMetricExpressionType.SIMPLE:
+ aggregate: str | None = metric.get("aggregate")
+ if aggregate not in self.sqla_aggregations:
Review Comment:
**Suggestion:** The new aggregate validation can still raise an uncaught
runtime error when `aggregate` is an unhashable JSON type (for example, a
list/object). `aggregate not in self.sqla_aggregations` attempts a dict-key
hash and will throw `TypeError`, resulting in a 500 instead of a controlled
`QueryObjectValidationError`. Validate `aggregate` is a string before the
membership check. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart data API can 500 on malformed aggregate payload.
- ⚠️ Embedded/guest dashboards can crash on crafted adhoc metric.
- ⚠️ Error handling bypasses intended QueryObjectValidationError validation
path.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send a chart query request to the chart data endpoint (e.g., POST
/api/v1/chart/data)
that ultimately builds a query using `SqlaTable.adhoc_metric_to_sqla` in
`superset/connectors/sqla/models.py:1678-1700`.
2. In the request JSON, include an adhoc metric with `"expressionType":
"SIMPLE"` and set
`"aggregate"` to an unhashable JSON type, for example `"aggregate": ["SUM"]`
or
`"aggregate": {"op": "SUM"}` within the metric object.
3. When the request is processed, `adhoc_metric_to_sqla` reads `aggregate`
from the metric
at `superset/connectors/sqla/models.py:1681` and then executes `if aggregate
not in
self.sqla_aggregations:` at line 1682.
4. Because `aggregate` is a list/dict, Python attempts to hash it for dict
key membership,
raising `TypeError: unhashable type` which is not caught, causing a 500
error instead of a
controlled `QueryObjectValidationError` response.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cfa2bb0afd12424b832763852c8e0939&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cfa2bb0afd12424b832763852c8e0939&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/connectors/sqla/models.py
**Line:** 1681:1682
**Comment:**
*Type Error: The new aggregate validation can still raise an uncaught
runtime error when `aggregate` is an unhashable JSON type (for example, a
list/object). `aggregate not in self.sqla_aggregations` attempts a dict-key
hash and will throw `TypeError`, resulting in a 500 instead of a controlled
`QueryObjectValidationError`. Validate `aggregate` is a string before the
membership check.
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%2F37371&comment_hash=838e04ccc1faa4ff1a9447409e2cef63b86416ba1e01c3e4abb462ab73cb0ade&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=838e04ccc1faa4ff1a9447409e2cef63b86416ba1e01c3e4abb462ab73cb0ade&reaction=dislike'>👎</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1519,6 +1601,442 @@ def test_query_context_modified_time_grain_in_orderby(
assert not query_context_modified(query_context)
+def test_query_context_modified_orderby_direction_change_allowed(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Test that changing sort direction (ASC/DESC) is allowed for visible
columns.
+ """
+ query_context = mocker.MagicMock()
+ query_context.slice_.id = 42
+ query_context.slice_.query_context = None
+ query_context.slice_.params_dict = {
+ "columns": ["name"],
+ "groupby": [],
+ "metrics": ["count"],
+ "orderby": [["name", True]], # Original: DESC
+ }
+ query_context.form_data = {
+ "slice_id": 42,
+ "columns": ["name"],
+ "metrics": ["count"],
+ "orderby": [["name", False]], # Changed to ASC - should be allowed
Review Comment:
**Suggestion:** The inline direction comments are reversed: in this codebase
the second `orderby` boolean means ascending (`True`) vs descending (`False`),
so these comments currently document the opposite behavior and can mislead
future maintenance of sorting/security tests. Update the comments to match the
actual boolean semantics. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Comments in security manager tests misrepresent orderby direction
semantics.
⚠️ Future maintainers might misread True/False effects on sorting.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/tests/unit_tests/security/manager_test.py` and locate
`test_query_context_modified_orderby_direction_change_allowed` around lines
1610–1625;
observe the comments on the two `orderby` entries at lines 1617 and 1623
stating `"True" =
Original: DESC` and `"False" = Changed to ASC`.
2. Open `superset/superset/security/manager.py` and inspect
`_is_valid_orderby_entry` at
lines 754–763; its docstring says order-by entries have ``[term,
ascending]`` shape,
indicating the boolean flag represents ascending when `True`.
3. Open
`superset/tests/integration_tests/db_engine_specs/datastore_tests.py` and
inspect
the test around lines 252–271; it builds a query object with `"orderby":
[["gender_cc",
True]]` and asserts the generated SQL contains `ORDER BY gender_cc ASC`,
confirming that
`True` means ascending in actual query building.
4. Compare these semantics with the unit-test comments in `manager_test.py`:
they describe
`True` as DESC and `False` as ASC, which contradicts the verified behavior
from
`datastore_tests.py` and the `_is_valid_orderby_entry` docstring, showing
the comments are
inverted documentation rather than reflecting real code behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87a91385e9f54f0e931de72c96a56437&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=87a91385e9f54f0e931de72c96a56437&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:** tests/unit_tests/security/manager_test.py
**Line:** 1617:1623
**Comment:**
*Comment Mismatch: The inline direction comments are reversed: in this
codebase the second `orderby` boolean means ascending (`True`) vs descending
(`False`), so these comments currently document the opposite behavior and can
mislead future maintenance of sorting/security tests. Update the comments to
match the actual boolean semantics.
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%2F37371&comment_hash=a6d210ea317d19fe6ec69d6d98e67b0fb5209ae6d1e7a13e60a3309c8c7c3443&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37371&comment_hash=a6d210ea317d19fe6ec69d6d98e67b0fb5209ae6d1e7a13e60a3309c8c7c3443&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]
