aminghadersohi opened a new pull request, #41923:
URL: https://github.com/apache/superset/pull/41923
## SUMMARY
First slice of a DRY cleanup pass over `superset/mcp_service/`. Two
independent, non-breaking changes:
1. **Shared list-tool schema base.** Every `list_*` tool's request/response
schema pair re-declared the same 7 filter/pagination fields, the same 2 field
validators, and the same 13-field response envelope (`tag/schemas.py` and
`query/schemas.py` were verbatim duplicates modulo naming). Introduces
`PaginatedListRequest`/`PaginatedResponse` generic base models in
`common/pagination_schemas.py` and migrates the 15 list schema pairs onto them:
`chart`, `dashboard`, `database`, `dataset`, `report`, `query`, `rls`, `role`,
`saved_query`, `tag`, `task`, `theme`, `user`, and `annotation_layer` (both
`list_annotation_layers` and `list_layer_annotations`).
`semantic_layer/list_metrics` is intentionally **not** migrated — its
request/response shape already diverges from the shared envelope (no
`filters`/`select_columns`/`order_direction`, a different response envelope
entirely), so folding it in would be a behavior change rather than a pure
dedup. Left as-is for separate follow-up work.
Wire format is unchanged. `dataset`'s `ListDatasetsRequest` is the one
domain that doesn't parse JSON-string `filters`/`select_columns` (unlike its 14
siblings) — that's intentional per an existing test
(`test_list_datasets_with_string_filters`) that asserts a `ValidationError` for
string input, so the base validators are explicitly overridden as no-ops there
to preserve that behavior exactly.
2. **Delete dead infrastructure (~800 lines).** `build_middleware_list()` in
`server.py` only wires `StructuredContentStripperMiddleware`,
`RBACToolVisibilityMiddleware`, `LoggingMiddleware`, and
`GlobalErrorHandlerMiddleware`. Three middleware classes were never registered
anywhere and have zero test coverage: `FieldPermissionsMiddleware` (which fails
open on error — "Return original response if filtering fails"),
`PrivateToolMiddleware`, and `RateLimitMiddleware` (plus its
`InMemoryRateLimiter`/`RedisRateLimiter` backends). Deletes all three, along
with the `utils/permissions_utils.py` helpers that existed solely to serve
`FieldPermissionsMiddleware` (kept `get_current_user`, which is used by
`mcp_core.py`). Checked git history for any in-progress work wiring up rate
limiting before deleting — found none; happy to hold this off if that's
incorrect.
Also deletes three small dead-code items: `SORTABLE_CHART_COLUMNS`
(`list_charts.py`, unreferenced), `ALL_SAVED_QUERY_COLUMNS`
(`saved_query/schemas.py`, unreferenced), and an unreachable
`isinstance(request.config, dict)` branch in `generate_chart.py`
(`request.config` is always a validated `ChartConfig`, never a dict).
Net: **-2,112 lines** (308 insertions, 2,420 deletions) across 20 files.
## BEFORE/AFTER
No behavior or API change — this is a structural refactor. Request/response
JSON shape, defaults, and validation error messages are identical to before.
## TESTING INSTRUCTIONS
1. `pytest tests/unit_tests/mcp_service/` — full suite (2,663 tests) passes
unmodified except for the removal of `test_permissions_utils.py`, which tested
only the deleted dead-code helpers.
2. `pre-commit run --files <changed files>` — mypy, ruff, ruff-format,
pylint all pass.
## ADDITIONAL INFORMATION
Follow-up work identified but intentionally out of scope for this PR:
- List/get tool factory to absorb the remaining ~80-120 lines of per-tool
wrapper boilerplate.
- Unified `error_type` enum + single error model on `MCPBaseError`
(currently ~100 free-form `error_type` strings across 4 incompatible
error-response shapes).
- Call-time enforcement of `@requires_data_model_metadata_access` in
`mcp_auth_hook` (currently ~20 tools repeat an inline guard).
- Dedupe `mcp_auth_hook`'s sync/async bodies (currently duplicated, with a
subtle behavioral divergence around `_remove_session_safe()`).
--
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]
