mikebridge commented on code in PR #45360:
URL: https://github.com/apache/superset/pull/45360#discussion_r4289625656
##########
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:
Good catch, and confirmed. I reproduced it end to end, with no mocked
exception: a semantic view with an empty `IN` filter, run through the real
`QueryContextProcessor` → `ChartDataCommand`, came back as `DataError` /
`ChartDataQueryFailedError`.
Fixed in 6573d120a9 and 90e721ed5f:
- The processor now records the swallowed error as
`QueryObjectValidationMessage`, a `str` subclass. It serializes as the same
text, so payload consumers are unchanged, but it keeps its cause.
- `ChartDataCommand` raises `ChartDataQueryValidationError` from that cause.
It subclasses `ChartDataQueryFailedError`, so the chart-data API's 400 and
`compile.py`'s catch behave exactly as before.
- MCP maps it to `ValidationError` with the cause's bounded text, so
validate-time and run-time refusals return the same message.
- Failed payloads are never cached, and copy/pickle degrade the marker to
plain text.
Tests (all red before the fix, green after):
- `test_saved_chart_mapper_refusal_is_validation_error`
- `test_unsaved_chart_mapper_refusal_is_validation_error`
- `test_validation_failure_raises_typed_error[run|execute]`
- `test_get_df_payload_keeps_dataset_validation_type`
Also added `test_other_query_failure_stays_untyped`,
`test_query_result_type_returns_validation_failure` and
`tests/unit_tests/exceptions_test.py`.
One scope note: mapper checks that raise a bare `ValueError` (for example a
time grain without a time column) still land in `DataError`, so I scoped the
docs to query validation errors. Converting those is the job of #45292.
_(Reply drafted with Claude (AI) on behalf of @mikebridge.)_
##########
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:
Applied your wording in 6573d120a9. I scoped the category sentence to query
validation errors in 90e721ed5f.
`test_saved_chart_mapper_refusal_is_validation_error` now asserts that
`ctx.error` is not called for these errors.
_(Reply drafted with Claude (AI) on behalf of @mikebridge.)_
--
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]
