codeant-ai-for-open-source[bot] commented on code in PR #43338:
URL: https://github.com/apache/superset/pull/43338#discussion_r3815958424
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -451,7 +471,9 @@ async def _handle_chart_sql_request(
# Fallback: build query context from form_data
try:
- return _sql_from_form_data(effective_form_data, chart)
+ return _sql_from_form_data(
+ effective_form_data, chart, request.extra_form_data
+ )
Review Comment:
**Suggestion:** The newly accepted `extra_form_data` is typed as an
unconstrained `dict[str, Any]`, but this call passes malformed filter entries
directly into normalization. For example, `filters=[{"col": "country"}]` causes
`simple_filter_to_adhoc` to raise `KeyError` for the missing `op`; the SQL
fallback handlers and `get_chart_sql` safety net do not catch `KeyError`, so
invalid client input becomes an unhandled tool failure. Validate the filter
structure before merging or convert these normalization errors into a
`ChartError`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Invalid `get_chart_sql` filter requests can fail unhandled.
- ⚠️ Saved and fallback SQL construction paths are affected.
- ⚠️ Clients receive no structured validation error.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_sql.py
**Line:** 474:476
**Comment:**
*Api Mismatch: The newly accepted `extra_form_data` is typed as an
unconstrained `dict[str, Any]`, but this call passes malformed filter entries
directly into normalization. For example, `filters=[{"col": "country"}]` causes
`simple_filter_to_adhoc` to raise `KeyError` for the missing `op`; the SQL
fallback handlers and `get_chart_sql` safety net do not catch `KeyError`, so
invalid client input becomes an unhandled tool failure. Validate the filter
structure before merging or convert these normalization errors into a
`ChartError`.
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%2F43338&comment_hash=449abc796c99509be1779c9fef8ded3a23f88f1c1eb93ca9cea36cbc6e539933&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43338&comment_hash=449abc796c99509be1779c9fef8ded3a23f88f1c1eb93ca9cea36cbc6e539933&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1360,6 +1360,161 @@ async def
test_json_format_also_eager_loads_metrics(self, mcp_server, mock_auth)
assert _extract_metrics_load_path(query_options[0]) == ["table",
"metrics"]
+class TestSavedChartExtraFormDataFilters:
+ """Regression tests: extra_form_data filters passed alongside a saved
+ chart identifier must reach the executed query, not just the cached
+ form_data / unsaved-chart path already covered elsewhere.
+
+ A chart with a saved query_context is the common case (any chart that
+ has been opened and saved through Explore), so this is the primary path
+ exercised when a caller passes extra_form_data with a chart identifier.
+ """
+
+ def _chart(self) -> SimpleNamespace:
+ from superset.utils import json as utils_json
+
+ return SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context=utils_json.dumps(
+ {
+ "datasource": {"id": 1, "type": "table"},
+ "queries": [
+ {
+ "columns": ["country"],
+ "metrics": ["count"],
+ "filters": [],
+ "row_limit": 100,
+ }
+ ],
+ "result_format": "json",
+ "result_type": "full",
+ }
+ ),
+ params=None,
+ )
+
+ async def _run(self, extra_form_data: dict[str, Any], mcp_server: Any) ->
Any:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+
+ captured: dict[str, Any] = {}
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ captured["loaded_query_context_json"] = data
+ return SimpleNamespace(queries=[SimpleNamespace(filter=[])])
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [
+ {
+ "data": [{"country": "USA"}],
+ "colnames": ["country"],
+ "rowcount": 1,
+ }
+ ]
+ }
Review Comment:
**Suggestion:** The added tests replace both schema loading and command
execution with fakes, and the fake command returns a USA row regardless of its
`query_context`. Consequently, these tests only verify that a filter-shaped
dictionary was inserted into raw JSON; they cannot detect a
schema-normalization or execution-path regression that causes the filter not to
affect the executed query. Keep the helper assertion if desired, but add
coverage using the real schema/query construction or assert that the fake
command receives a query context containing the normalized filter. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Filter tests can pass despite execution-path regressions.
- ⚠️ `get_chart_data` query filtering remains only partially covered.
- ⚠️ The fake command does not validate received query context.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 1411:1427
**Comment:**
*Incomplete Implementation: The added tests replace both schema loading
and command execution with fakes, and the fake command returns a USA row
regardless of its `query_context`. Consequently, these tests only verify that a
filter-shaped dictionary was inserted into raw JSON; they cannot detect a
schema-normalization or execution-path regression that causes the filter not to
affect the executed query. Keep the helper assertion if desired, but add
coverage using the real schema/query construction or assert that the fake
command receives a query context containing the normalized filter.
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%2F43338&comment_hash=c67514bd47411d9dd53e9139387050202469826afbd0c57a08f9913218eb0ceb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43338&comment_hash=c67514bd47411d9dd53e9139387050202469826afbd0c57a08f9913218eb0ceb&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]
