mikebridge opened a new pull request, #45360:
URL: https://github.com/apache/superset/pull/45360
### SUMMARY
**TL;DR:** The MCP `get_chart_data` tool now reports refused queries as
`AccessDenied`, and queries rejected by a semantic layer provider as
`ValidationError`. Both used to be reported as a generic `DataError`.
**Background.** The chart-data API answers a query in one of three ways:
- **403:** the user is not allowed to run it (for example, a guest whose
row-level security rules a semantic view cannot enforce);
- **400:** the semantic layer provider rejected the query as invalid;
- **500:** something failed while running it.
**Problem.** `get_chart_data` caught all three in one handler and returned
`DataError` for each. An MCP client, such as an AI assistant, could not tell
"you may not see this" or "this query is invalid, change it" apart from
"something broke, try again". It might retry a refused query, or report a
permission problem as an outage.
**What this changes.**
- A query refused for access returns `error_type: "AccessDenied"`. This is
the category `list_metrics`, `get_table` and `get_compatible_metrics` already
use.
- A query rejected by a semantic layer provider returns `error_type:
"ValidationError"`, like other invalid queries.
- Provider faults still return `DataError`.
- Both the saved-chart path and the unsaved (`form_data_key`) path behave
the same way.
**Technical details**
- `superset/mcp_service/chart/tool/get_chart_data.py`:
- `SemanticLayerQueryRejectedError` joins the existing
`QueryObjectValidationError` branch.
- A new `SupersetSecurityException` branch comes before the generic
`SupersetException` handler. OAuth errors are still re-raised first.
- The `AccessDenied` message is the same bounded text the `DataError` used
to carry, without the "Error retrieving chart data:" prefix.
- `UPDATING.md` and the MCP server admin docs note the new categories.
**Scope and related work.** This is part of a semantic-layer audit item
about consistent refusal handling. The dashboard `/datasets` and `list_metrics`
parts of that item are not in this PR:
- On `master`, neither path can raise a refusal inside the block that
swallows errors.
- They become reachable with #45251 (provider execution-context refusals)
and #44454 (the dashboard `metadata_error` payload), and will follow once those
merge.
- #45292 (mapper validation errors become `QueryObjectValidationError`)
needs nothing extra here: those errors already reach `get_chart_data`'s
`ValidationError` branch.
- The sibling MCP query tools `get_table`, `query_dataset` and
`get_chart_preview` still return their own generic categories for these errors.
That is left for a follow-up, because each has its own error vocabulary.
Fixes [SC-126718](https://app.shortcut.com/preset/story/126718)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI change.
- **Before:** `get_chart_data` on a semantic-view chart, for an embedded
guest with row-level security rules, returned `{"error_type": "DataError",
"error": "Error retrieving chart data: Semantic views cannot enforce guest
row-level security rules."}`.
- **After:** `{"error_type": "AccessDenied", "error": "Semantic views cannot
enforce guest row-level security rules."}`.
### TESTING INSTRUCTIONS
1. With the MCP service running, call `get_chart_data` on a chart whose
query the caller is refused. Expect `error_type: "AccessDenied"`.
2. Call it on a semantic-view chart whose provider rejects the query (for
example, an unsupported filter). Expect `error_type: "ValidationError"`.
3. Run `pytest
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py -k
keeps_api_category`.
Automated verification:
- The new tests cover refusal, rejection and fault cases on both the saved
and unsaved paths. Against `master` they gave 4 failed and 2 passed: the two
fault controls pass, because faults are unchanged. With the fix, all 6 pass.
The OAuth routing tests still pass.
- `tests/unit_tests/mcp_service`: 10870 passed. Its 8 failures are the known
local-environment dateutil/DST timezone tests that also fail on `master`
(bullet chart ×5, `get_chart_data` ×1, `query_dataset` ×1, `get_table` ×1).
- Changed-file pre-commit hooks pass, including MyPy.
- Full local unit suite: 27457 passed. The only failures are the 11 known
local-environment ones that also fail on `master`: the 8 timezone tests above,
plus one `longdouble` test, one browser numeric-contract test and one Babel
fixture test.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
