aminghadersohi commented on code in PR #45360:
URL: https://github.com/apache/superset/pull/45360#discussion_r4300347799
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -1202,11 +1209,15 @@ async def _get_chart_data( # noqa: C901
# dedicated outer handlers return the OAuth redirect message
# instead of a generic DataError.
raise
- except QueryObjectValidationError as ex:
+ except (QueryObjectValidationError, SemanticLayerQueryRejectedError)
as ex:
Review Comment:
Confirmed fixed at bf60175e4d94. `QueryObjectValidationMessage` survives the
processor-to-command hop untouched — query_context_processor.py:337 writes it,
:429 puts it in the payload, :723 hands the payload dicts straight through —
and `ChartDataQueryValidationError` is matched by `_VALIDATION_ERRORS` at
get_chart_data.py:1243 ahead of the generic `(CommandException,
SupersetException, ValueError)` at :1252. Worth noting for anyone reordering
that ladder later: its `__mro__` is `ChartDataQueryValidationError ->
ChartDataQueryFailedError -> CommandException -> SupersetException`, so it *is*
a `CommandException` and only the catch order keeps it out of `DataError`.
Tests: `test_validation_failure_raises_typed_error`,
`test_get_df_payload_keeps_dataset_validation_type`,
`test_saved_chart_mapper_refusal_is_validation_error`,
`test_unsaved_chart_mapper_refusal_is_validation_error`,
`test_other_query_failure_stays_untyped`.
Siblings checked:
- superset/common/query_context_processor.py:335 — fixed in
6573d120a9cffa6bd77ac2d76bb0a47641e6a737
- superset/common/query_actions.py:144 — not affected: `_get_query` is bound
only to `ChartDataResultType.QUERY` at query_actions.py:333, and
`_raise_for_query_error` returns early for that result type, so the plain-str
error stays in the payload by design
- superset/common/query_object.py:474 — not affected: validate-time path,
returns the exception rather than writing a payload error
- superset/charts/data/api.py:235 — not affected: REST handler answering 400
directly, not the MCP path
- superset/charts/data/api.py:335 — not affected: REST handler answering 400
directly, not the MCP path
- superset/datasource/api.py:378 — not affected: REST handler answering 400
directly, not the MCP path
##########
UPDATING.md:
##########
@@ -338,6 +338,13 @@ Connector/C this also verifies the server hostname, so the
switch needs
host. A Doris FE on its default self-signed certificate fails to connect once
the switch is on.
+### MCP get_chart_data error categories
+
+`get_chart_data` returns `AccessDenied` when a query is refused for access, and
+`ValidationError` when a semantic layer provider rejects the query, matching
the
+chart-data API's 403 and 400. Both were previously reported as `DataError`.
+Clients that matched on `DataError` for these cases should handle the new
types.
Review Comment:
Fixed in 6573d120a9cffa6bd77ac2d76bb0a47641e6a737; the wording landed
verbatim and the extra scoping in 90e721ed5ffb6cc4126cee30b0ba8b7edf252d15 is
right. The saved-path validation branch at get_chart_data.py:1243 returns
`_validation_error(ex)` after only a `logger.warning`, with no `ctx.error`,
which is exactly what the new sentence documents. Regression test:
`test_saved_chart_mapper_refusal_is_validation_error`, which asserts
`ctx.error` is not called.
Siblings checked:
- UPDATING.md:379 — fixed in 6573d120a9cffa6bd77ac2d76bb0a47641e6a737
- superset/mcp_service/chart/tool/get_chart_data.py:1243 — not affected:
saved-path validation branch emits no `ctx.error`, matching the documented
behaviour
- superset/mcp_service/chart/tool/get_chart_data.py:1490 — not affected: the
unsaved path never emitted `ctx.error` on any branch, so limiting the sentence
to the saved path is accurate
- docs/admin_docs/configuration/mcp-server.mdx — not affected: describes the
same categories and agrees with the scoped wording
--
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]
