refactor(mcp): dedupe list-tool schemas and delete dead middleware - #41923
Merged
aminghadersohi merged 7 commits intoJul 22, 2026
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41923 +/- ##
==========================================
+ Coverage 65.27% 65.28% +0.01%
==========================================
Files 2769 2769
Lines 156339 155552 -787
Branches 35787 35716 -71
==========================================
- Hits 102047 101551 -496
+ Misses 52325 52047 -278
+ Partials 1967 1954 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
marked this pull request as ready for review
July 10, 2026 20:34
dosubot
Bot
added
the
risk:refactor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs a structural cleanup of superset/mcp_service/ by deduplicating the common request/response envelope used by MCP list_* tools and removing unused/dead MCP middleware and related helpers/tests.
Changes:
- Introduces shared generic base schemas (
PaginatedListRequest/PaginatedResponse) and migrates MCP list-tool schemas to use them. - Removes unused MCP middleware implementations (and related field-permissions utilities) that were not wired into the server pipeline.
- Deletes a few unreferenced constants and an unreachable branch in chart generation error handling.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| superset/mcp_service/common/pagination_schemas.py | Adds shared generic list request/response base models and validators. |
| superset/mcp_service/{annotation_layer,dashboard,database,dataset,query,report,rls,role,saved_query,tag,task,theme,user}/schemas.py | Migrates per-domain list request/response schemas onto the shared pagination bases. |
| superset/mcp_service/middleware.py | Deletes dead/unused middleware (private tool gating, rate limiting, field permissions filtering). |
| superset/mcp_service/utils/permissions_utils.py | Reduces module to get_current_user() after removing dead field-permissions helpers. |
| superset/mcp_service/chart/schemas.py | Migrates chart list schemas onto the shared pagination bases. |
| superset/mcp_service/chart/tool/list_charts.py | Removes an unreferenced sortable-columns constant. |
| superset/mcp_service/chart/tool/generate_chart.py | Removes unreachable error-context extraction branch in exception handling (needs a small follow-up fix). |
| tests/unit_tests/mcp_service/utils/test_permissions_utils.py | Deletes tests for removed dead-code permission helpers. |
Contributor
|
The comment is accurate. Setting |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #df3a26
Actionable Suggestions - 1
-
superset/mcp_service/chart/tool/generate_chart.py - 1
- Error context regression · Line 891-891
Additional Suggestions - 2
-
superset/mcp_service/annotation_layer/schemas.py - 1
-
Type parameter inconsistency in generic class · Line 105-105The type parameter `ColumnOperator` on `AnnotationList` is inconsistent with the request schema's `AnnotationFilter`. Since `PaginatedResponse` is used only for response serialization and the `filters_applied` field is populated by the tool code's own `combined_filters` list (typed `List[ColumnOperator]`), this has no runtime effect, but it misleads callers about the filter type in this domain. Use `AnnotationFilter` to match `ListLayerAnnotationsRequest`.
-
-
superset/mcp_service/utils/permissions_utils.py - 1
-
Catch specific exceptions instead of Exception · Line 31-31Catching generic `Exception` is too broad and can mask unexpected errors. Consider catching only the specific `ImportError` that might occur from the flask import, or remove the try-except entirely if not needed.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/user/schemas.py - 2
- order_direction default mismatch · Line 156-161
- Missing parse_columns validator · Line 238-258
-
superset/mcp_service/task/schemas.py - 1
- Missing docstring on refactored class · Line 114-115
Review Details
-
Files reviewed - 20 · Commit Range:
33034c4..33034c4- superset/mcp_service/annotation_layer/schemas.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/generate_chart.py
- superset/mcp_service/chart/tool/list_charts.py
- superset/mcp_service/common/pagination_schemas.py
- superset/mcp_service/dashboard/schemas.py
- superset/mcp_service/database/schemas.py
- superset/mcp_service/dataset/schemas.py
- superset/mcp_service/middleware.py
- superset/mcp_service/query/schemas.py
- superset/mcp_service/report/schemas.py
- superset/mcp_service/rls/schemas.py
- superset/mcp_service/role/schemas.py
- superset/mcp_service/saved_query/schemas.py
- superset/mcp_service/tag/schemas.py
- superset/mcp_service/task/schemas.py
- superset/mcp_service/theme/schemas.py
- superset/mcp_service/user/schemas.py
- superset/mcp_service/utils/permissions_utils.py
- tests/unit_tests/mcp_service/utils/test_permissions_utils.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
aminghadersohi
force-pushed
the
aminghadersohi/mcp-list-schema-dedup
branch
from
July 15, 2026 16:35
24fbbc2 to
961030c
Compare
Contributor
Code Review Agent Run #f0a467Actionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Introduce PaginatedListRequest/PaginatedResponse generic base models in common/pagination_schemas.py and migrate the 15 list-tool request/response schema pairs (chart, dashboard, database, dataset, report, query, rls, role, saved_query, tag, task, theme, user, annotation_layer x2) onto them, removing ~2,100 lines of copy-pasted filter/pagination boilerplate. Wire format is unchanged; the existing unit test suite (2,663 tests) passes without modification aside from removing tests for deleted dead code. semantic_layer/list_metrics is intentionally excluded — its request/response shape already diverges from the shared envelope (see audit §1.4) and fixing that is separate follow-up work. Also removes ~800 lines of dead infrastructure that was never wired into build_middleware_list() in server.py: FieldPermissionsMiddleware (which fails open on error), PrivateToolMiddleware, RateLimitMiddleware plus its InMemoryRateLimiter/RedisRateLimiter backends, and the now-unused utils/permissions_utils.py helpers that existed solely to serve FieldPermissionsMiddleware (get_current_user is kept — it's used by mcp_core.py). Checked git history for in-progress rate-limiting work before deleting; found none. Deletes three other small dead-code items: SORTABLE_CHART_COLUMNS (list_charts.py, unused), ALL_SAVED_QUERY_COLUMNS (saved_query/schemas.py, unused), and an unreachable isinstance(request.config, dict) branch in generate_chart.py (request.config is always a validated ChartConfig, never a dict). Follow-ups (not in this PR): list/get tool factory to absorb per-tool boilerplate, a unified error_type enum on MCPBaseError, call-time enforcement of @requires_data_model_metadata_access in mcp_auth_hook, and deduping the mcp_auth_hook sync/async bodies.
…chart error path Addresses review feedback (copilot, bito, codeant) noting that request.config is a validated ChartConfig whose variants all define chart_type as a discriminator, so hardcoding "unknown" discarded context that was actually available.
The list-schema-dedup refactor deleted the `Dict` import while leaving one annotation referencing it, which raises NameError at module import since middleware.py has no `from __future__ import annotations`.
…lters Addresses review feedback noting the newly introduced local variable lacked an explicit type hint.
aminghadersohi
force-pushed
the
aminghadersohi/mcp-list-schema-dedup
branch
from
July 17, 2026 16:02
2556804 to
d608d20
Compare
… search/filters test Addresses two 2026-07-15 code-review-bot suggestions on #41923: - annotation_layer/schemas.py: a bito "wrong generic type parameter" flag on AnnotationList(PaginatedResponse[ColumnOperator]) looked like a dedup regression (every other domain parametrizes with its own *Filter type), but list_layer_annotations prepends a synthetic "layer_id" ColumnOperator to filters_applied that AnnotationFilter's col Literal doesn't allow, so the loose ColumnOperator param is intentional. Added a comment to make that explicit for future readers. - dataset/test_dataset_schemas.py: added the missing search+filters-mutual-exclusion test for ListDatasetsRequest, matching the existing per-domain tests on chart/theme/query/saved_query for the same PaginatedListRequest.validate_search_and_filters behavior.
Contributor
Author
|
Follow-up on the 2026-07-15 Bito review run #f0a467 — addressing all 6 items (3 "Additional Suggestions" + 3 "Filtered by Review Rules"): Fixed (pushed in 480a59b):
No change needed:
Re-verified: 0 unresolved review threads, 2,976 |
Contributor
Code Review Agent Run #99fffeActionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
approved these changes
Jul 22, 2026
rusackas
left a comment
rusackas
left a comment
Member
There was a problem hiding this comment.
Nice cleanup. That's a lot of copy-pasted pagination/filter boilerplate collapsed into one generic base, and the description does the legwork of explaining what's intentionally excluded (list_metrics, the dataset string-filter override) rather than making me go dig for it. Also good to see the dead middleware actually confirmed dead (checked for in-progress rate-limiting work before deleting) instead of just assumed.
CI's green across the board. The only open thread is codeant flagging the dataset conflict-validation test for asserting ValueError instead of ValidationError. That one's wrong though, pydantic's ValidationError subclasses ValueError in v2, so pytest.raises(ValueError, ...) catches it fine and the test does pass. Not blocking anything.
Good history of applying the real bot feedback (chart_type, the missing type hint) and pushing back with an actual explanation on the one that wasn't (the AnnotationList ColumnOperator parametrization). Approving.
niteshpurohit
added a commit
to HiMamaInc/superset
that referenced
this pull request
Jul 24, 2026
* refactor(mcp): dedupe list-tool schemas and delete dead middleware (apache#41923) * fix(ag-grid-table): respect row limit with server pagination (apache#41346) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(table/pivot-table): correct non-additive totals/subtotals via DB rollup [SIP-216] (apache#41184) Co-authored-by: Superset Dev <dev@superset.apache.org> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Amin Ghadersohi <amin.ghadersohi@gmail.com> * feat(datasets): add RLS filter indicator badge to dataset list and explore view (apache#38807) Co-authored-by: Evan <evan@preset.io> * fix(chart): updates counties of kenya map (apache#38019) Co-authored-by: Zack Adams <zack@Zacks-Laptop.local> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Evan <evan@preset.io> Co-authored-by: Evan Rusackas <evan@rusackas.com> * chore(deps): bump actions/setup-go from 6.5.0 to 7.0.0 (apache#42303) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump nanoid from 5.0.9 to 6.0.0 in /superset-frontend (apache#42230) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: hainenber <dotronghai96@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: hainenber <dotronghai96@gmail.com> Co-authored-by: Joe Li <joe@preset.io> Co-authored-by: Evan <evan@preset.io> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * test(dashboard): migrate dashboard load smoke test to Playwright (apache#41432) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(superset_app_root): when used with oauth (apache#38033) Signed-off-by: Grégoire Bellon-Gervais <gregoire.bellon-gervais@docaposte.fr> Co-authored-by: Evan Rusackas <evan@preset.io> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * refactor: engine enforce SQLAlchemy 2.0 (apache#42277) * fix(ag-grid-table): avoid ambiguous build query import (apache#42313) * fix: Revert "chore(deps): bump echarts from 5.6.0 to 6.1.0 in /superset-frontend" (apache#42314) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * adding circleci config * fix(plugin-chart-echarts): import the -obj locale build so time axes render (apache#42317) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(pandas_postprocessing): avoid FutureWarning for max/min in boxplot MINMAX (apache#42272) * fix(heatmap): correct tooltip axis value lookup and percentage calculations and add tests (apache#41864) Signed-off-by: yousoph <sophieyou12@gmail.com> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix(forecast): resolve time grain robustly for Prophet forecasting (apache#42145) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix: Added PostgreSQL 17.X to the supported database versions table in (apache#42280) * fix(helm): add MCP HTTPRoute configuration (apache#42219) * fix(dashboard): offer Exit edit mode when there is nothing to discard (apache#42208) Co-authored-by: Claude Code <noreply@anthropic.com> * chore(deps): bump actions/labeler from 6.2.0 to 7.0.0 (apache#42332) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump github/codeql-action/analyze from 4.37.0 to 4.37.1 (apache#42331) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump github/codeql-action/init from 4.37.0 to 4.37.1 (apache#42334) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @formatjs/intl-durationformat from 0.10.17 to 0.10.18 in /superset-frontend (apache#42337) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump caniuse-lite from 1.0.30001805 to 1.0.30001806 in /docs (apache#42333) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump echarts to 6.1.0 with locale and containLabel guards (apache#42315) (apache#42321) Co-authored-by: Claude Code <noreply@anthropic.com> Co-authored-by: Amin Ghadersohi <amin.ghadersohi@gmail.com> * docs: add pattern to the list of organisations using superset (apache#42341) * chore(deps): bump ag-grid from 36.0.0 to 36.0.1 in /superset-frontend (apache#42338) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: hainenber <dotronghai96@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: hainenber <dotronghai96@gmail.com> * fix(mcp): trust dataset is_dttm flag when applying time_grain to VARCHAR temporal columns (apache#42288) * chore(deps-dev): update taos-ws-py requirement from >=0.6.9 to >=0.7.0 (apache#42344) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump the storybook group in /superset-frontend with 5 updates (apache#42355) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(native-filters): keep filter value input caret at inline start (apache#42323) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(mcp): truncate query-tool responses instead of hard-failing (apache#42244) * chore(deps): bump nh3 from 0.3.5 to 0.3.6 (apache#42349) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> * chore(deps): bump pydantic from 2.11.7 to 2.13.4 (apache#42350) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> * chore(deps): bump sqlalchemy-continuum from 1.6.0 to 1.7.0 (apache#42351) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> * fix(embedded): stop rejecting guest chart data built from control-specific params keys (apache#42295) Co-authored-by: Claude Code <noreply@anthropic.com> * fix(explore): show the beginning date on time-series x-axis line charts (apache#42046) * fix(api): add example to get_export_ids_schema so Swagger "Try it out" pre-fills a valid array (apache#42265) * chore: SQLAlchemy User cascade backref warnings are irrelevant (apache#42360) * fix(charts): handle async (202) chart-data responses in StatefulChart (apache#42157) Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Evan Rusackas <evan@preset.io> * feat(KustoKQL): Add support for NULL / IS NOT NULL operator (apache#37890) Co-authored-by: ag-ramachandran <ramacg@microsoft.com> Co-authored-by: Joe Li <joe@preset.io> * chore(deps): bump pillow from 12.2.0 to 12.3.0 (apache#42348) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> * chore(deps): bump flask-compress from 1.17 to 1.24 (apache#42346) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> * chore(deps-dev): bump databricks-sql-connector from 4.2.6 to 4.3.0 (apache#42347) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Joe Li <joe@preset.io> Co-authored-by: Evan Rusackas <evan@preset.io> * fix(native-filters): use FILTER_STATE_CACHE_CONFIG timeout for dynamic filter option queries (apache#38910) * fix(explore): render Jinja before validating legacy chart filters (apache#41996) * fix(dataset): disable duplicate button when name is empty (apache#42217) Co-authored-by: AS-MAC-1123 <as-mac-1123@AS-MAC-1123.local> Co-authored-by: Evan Rusackas <evan@rusackas.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * docs(gunicorn): correct dead links in values.yaml (apache#42385) * chore(deps): bump @deck.gl/mapbox from 9.3.6 to 9.3.7 in /superset-frontend in the deckgl group (apache#42377) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump actions/checkout from 7.0.0 to 7.0.1 (apache#42376) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump immer from 11.1.11 to 11.1.15 in /superset-frontend (apache#42378) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps): bump body-parser from 1.20.5 to 1.20.6 in /docs (apache#42370) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump brace-expansion from 1.1.15 to 1.1.16 in /superset-embedded-sdk (apache#42369) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(security): bump pyasn1 from 0.6.3 to 0.6.4 (apache#42363) * fix(security): bump pillow from 12.2.0 to 12.3.0 (apache#42362) * docs(map-tiles): add Yandex Maps Tiles API configuration (apache#42375) * ci: improve conditional checks for lillio tests and build --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: hainenber <dotronghai96@gmail.com> Signed-off-by: Grégoire Bellon-Gervais <gregoire.bellon-gervais@docaposte.fr> Signed-off-by: yousoph <sophieyou12@gmail.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Amin Ghadersohi <amin.ghadersohi@gmail.com> Co-authored-by: Evan Rusackas <evan@preset.io> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Superset Dev <dev@superset.apache.org> Co-authored-by: SkinnyPigeon <e.blackledge@stuart.com> Co-authored-by: Zack <adams.z.d@gmail.com> Co-authored-by: Zack Adams <zack@Zacks-Laptop.local> Co-authored-by: Evan Rusackas <evan@rusackas.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: hainenber <dotronghai96@gmail.com> Co-authored-by: Joe Li <joe@preset.io> Co-authored-by: Grégoire <greggbg@gmail.com> Co-authored-by: Hans Yu <hans.yu@outlook.de> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: yousoph <sophieyou12@gmail.com> Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com> Co-authored-by: Amitesh Gupta <143833521+singlaamitesh@users.noreply.github.com> Co-authored-by: David <39565245+dmunozv04@users.noreply.github.com> Co-authored-by: Yash Shrivastava <119301033+alephys26@users.noreply.github.com> Co-authored-by: JUST.in DO IT <justin.park@airbnb.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: jesperct <jmilecelento@gmail.com> Co-authored-by: Abdul Rehman <76230556+Abdulrehman-PIAIC80387@users.noreply.github.com> Co-authored-by: jenwitteng <jenwit.amonpongitsara@agoda.com> Co-authored-by: Ramachandran A G <106139410+ag-ramachandran@users.noreply.github.com> Co-authored-by: ag-ramachandran <ramacg@microsoft.com> Co-authored-by: Ujjwal Jain <jainujjwal1609@gmail.com> Co-authored-by: Jean Massucatto <massucattoj@gmail.com> Co-authored-by: suvankardas216 <rohanrohan510@gmail.com> Co-authored-by: AS-MAC-1123 <as-mac-1123@AS-MAC-1123.local> Co-authored-by: Alejandro Solares <219859296+ASolarers-Rodriguez@users.noreply.github.com> Co-authored-by: ViktorGo86 <114023094+ViktorGo86@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
First slice of a DRY cleanup pass over
superset/mcp_service/. Two independent, non-breaking changes: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.pyandquery/schemas.pywere verbatim duplicates modulo naming). IntroducesPaginatedListRequest/PaginatedResponsegeneric base models incommon/pagination_schemas.pyand migrates the 15 list schema pairs onto them:chart,dashboard,database,dataset,report,query,rls,role,saved_query,tag,task,theme,user, andannotation_layer(bothlist_annotation_layersandlist_layer_annotations).semantic_layer/list_metricsis intentionally not migrated — its request/response shape already diverges from the shared envelope (nofilters/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'sListDatasetsRequestis the one domain that doesn't parse JSON-stringfilters/select_columns(unlike its 14 siblings) — that's intentional per an existing test (test_list_datasets_with_string_filters) that asserts aValidationErrorfor string input, so the base validators are explicitly overridden as no-ops there to preserve that behavior exactly.Delete dead infrastructure (~800 lines).
build_middleware_list()inserver.pyonly wiresStructuredContentStripperMiddleware,RBACToolVisibilityMiddleware,LoggingMiddleware, andGlobalErrorHandlerMiddleware. 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, andRateLimitMiddleware(plus itsInMemoryRateLimiter/RedisRateLimiterbackends). Deletes all three, along with theutils/permissions_utils.pyhelpers that existed solely to serveFieldPermissionsMiddleware(keptget_current_user, which is used bymcp_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 unreachableisinstance(request.config, dict)branch ingenerate_chart.py(request.configis always a validatedChartConfig, 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
pytest tests/unit_tests/mcp_service/— full suite (2,663 tests) passes unmodified except for the removal oftest_permissions_utils.py, which tested only the deleted dead-code helpers.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:
error_typeenum + single error model onMCPBaseError(currently ~100 free-formerror_typestrings across 4 incompatible error-response shapes).@requires_data_model_metadata_accessinmcp_auth_hook(currently ~20 tools repeat an inline guard).mcp_auth_hook's sync/async bodies (currently duplicated, with a subtle behavioral divergence around_remove_session_safe()).