codeant-ai-for-open-source[bot] commented on code in PR #43478:
URL: https://github.com/apache/superset/pull/43478#discussion_r3846242607
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -1177,3 +1418,99 @@ async def test_dataset_not_accessible(self, mock_find,
mock_validate, mcp_server
data = result.structured_content.get("result",
result.structured_content)
assert data["error_type"] == "DatasetNotAccessible"
assert "Access denied" in data["error"]
+
+ @patch.object(_get_chart_sql_mod, "_sql_from_form_data")
+ @patch.object(_get_chart_sql_mod, "_get_cached_form_data")
+ @pytest.mark.asyncio
+ async def test_unsaved_chart_extra_form_data_reaches_sql_builder(
+ self, mock_cached, mock_form_data_sql, mcp_server
+ ):
+ """Regression test: extra_form_data must reach the SQL builder on the
+ form_data_key-only (unsaved chart) path too, not just the saved-chart
+ paths."""
+ from fastmcp import Client
+
+ from superset.utils import json as _json
+
+ cached_form_data = {"datasource_id": 1, "datasource_type": "table"}
+ mock_cached.return_value = _json.dumps(cached_form_data)
+ mock_form_data_sql.return_value = ChartSql(
+ chart_id=0,
+ chart_name=None,
+ sql="SELECT * FROM sales WHERE country = 'USA'",
+ language="sql",
+ datasource_name="sales",
+ )
+
+ extra_form_data = {"filters": [{"col": "country", "op": "==", "val":
"USA"}]}
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_sql",
+ {
+ "request": {
+ "form_data_key": "cached-key",
+ "extra_form_data": extra_form_data,
+ }
+ },
+ )
+
+ data = result.structured_content.get("result",
result.structured_content)
+ assert "WHERE country = 'USA'" in data["sql"]
+
+ mock_form_data_sql.assert_called_once_with(
+ cached_form_data, chart=None, extra_form_data=extra_form_data
+ )
+
+ @patch.object(_get_chart_sql_mod, "validate_chart_dataset")
+ @patch.object(_get_chart_sql_mod, "_find_chart_by_identifier")
+ @pytest.mark.asyncio
+ async def test_malformed_extra_form_data_filter_returns_clean_error(
+ self, mock_find, mock_validate, mcp_server
+ ):
+ """A malformed extra_form_data filter (missing 'op') must return a
+ structured ChartError, not crash with an unhandled KeyError.
+
+ Regression test: merge_extra_form_data_filters_into_query normalizes
+ filters via simple_filter_to_adhoc, which raises KeyError on a filter
+ entry missing "col" or "op". That KeyError previously propagated out
+ of get_chart_sql uncaught.
+ """
+ from fastmcp import Client
+
+ from superset.mcp_service.chart.chart_utils import
DatasetValidationResult
+ from superset.utils import json as _json
+
+ mock_chart = Mock()
+ mock_chart.id = 40
+ mock_chart.slice_name = "Sales"
+ mock_chart.viz_type = "table"
+ mock_chart.query_context = _json.dumps(
+ {
+ "datasource": {"id": 1, "type": "table"},
+ "queries": [
+ {"columns": ["country"], "metrics": ["count"], "filters":
[]}
+ ],
+ }
+ )
+ mock_find.return_value = mock_chart
+
+ mock_validate.return_value = DatasetValidationResult(
+ is_valid=True, dataset_id=1, dataset_name="ds", warnings=[]
+ )
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_sql",
+ {
+ "request": {
+ "identifier": 40,
+ # missing "op" — malformed filter entry
+ "extra_form_data": {"filters": [{"col": "country"}]},
+ }
+ },
+ )
+
+ data = result.structured_content.get("result",
result.structured_content)
+ assert data["error_type"] == "ValidationError"
+ assert "Invalid chart query data" in data["error"]
Review Comment:
**Suggestion:** The assertion expects the fallback-path message, but
malformed filters on the saved `query_context` path are caught earlier and
return an error beginning with `Invalid extra_form_data filter`, so this test
will fail even when the production error handling behaves as implemented.
Assert the actual saved-context validation message or change the production
path if the fallback message is intended. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ The new malformed-filter regression test fails during the chart SQL test
suite.
- ⚠️ CI cannot pass until the expected error message matches the
saved-context path.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2169b5382d0249e6840c8f081cfef173&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=2169b5382d0249e6840c8f081cfef173&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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_sql.py
**Line:** 1515:1516
**Comment:**
*Logic Error: The assertion expects the fallback-path message, but
malformed filters on the saved `query_context` path are caught earlier and
return an error beginning with `Invalid extra_form_data filter`, so this test
will fail even when the production error handling behaves as implemented.
Assert the actual saved-context validation message or change the production
path if the fallback message is intended.
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%2F43478&comment_hash=792cc5ce691bcc171fdc019b2a834ddec41cad830c9d5f5370fd0380d290e496&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43478&comment_hash=792cc5ce691bcc171fdc019b2a834ddec41cad830c9d5f5370fd0380d290e496&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]
