feat(mcp): deleted_state trash listing for list_charts and list_dashboards#41855
feat(mcp): deleted_state trash listing for list_charts and list_dashboards#41855gkneighb wants to merge 1 commit into
Conversation
…oards Expose the soft-delete trash (apache#40128, apache#40129) to MCP list tools via a deleted_state param ('include'/'only'). Reuses the REST Chart/DashboardDeletedStateFilter classes — which own the restore- audience scoping (owners or admins only) — through BaseDAO.list's custom_filters hook, with a DeletedStateBoundFilter adapter binding the caller's value since the DAO invokes custom filters with value=None. The DAO call is wrapped in the skip_visibility_filter context manager so the soft-delete ORM listener is bypassed for both the count and fetch executions, with guaranteed release. deleted_at is forced into loaded columns and added to ChartInfo/DashboardInfo so trashed rows are distinguishable in 'include' mode. 10 unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014z1fQxTadTEvpCJLx9WnB8
dosubot
Bot
added
api
Code Review Agent Run #811aeaActionable Suggestions - 0Filtered 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_auth(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type hint to this fixture function so it complies with the required type-annotation rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new fixture function mock_auth omits a return type annotation, which violates the Python type-hint requirement for newly added or modified functions that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_list_dashboards_deleted_state.py
**Line:** 45:45
**Comment:**
*Custom Rule: Add an explicit return type hint to this fixture function so it complies with the required type-annotation rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| dao_kwargs = { | ||
| "filters": filters, | ||
| "order_column": order_column or "changed_on", | ||
| "order_direction": str(order_direction or "desc"), | ||
| "page": page, | ||
| "page_size": page_size, | ||
| "search": search, | ||
| "columns_to_load": columns_to_load, | ||
| } |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation to this dictionary to satisfy the type-hint requirement for relevant local variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced local dictionary in Python code and it has no explicit type annotation. That matches the rule requiring type hints for relevant variables that can be annotated, so the suggestion is valid.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/mcp_core.py
**Line:** 395:403
**Comment:**
*Custom Rule: Add an explicit type annotation to this dictionary to satisfy the type-hint requirement for relevant local variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| return mcp | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) |
There was a problem hiding this comment.
Suggestion: Add an explicit return type hint to this fixture function (for example, a generator/yield-compatible return annotation) to satisfy required Python typing. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The rule requires new or modified Python functions to include type hints when they can be annotated. This fixture function is defined without any return type annotation, so the suggestion correctly identifies a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_list_charts_deleted_state.py
**Line:** 47:47
**Comment:**
*Custom Rule: Add an explicit return type hint to this fixture function (for example, a generator/yield-compatible return annotation) to satisfy required Python typing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The suggestion to add an explicit return type hint to the fixture function is correct and aligns with standard Python type-annotation practices. To resolve this, you should update the function signature to include the appropriate return type, for example: Regarding other comments on this PR, I have checked the available review data, and there are no additional comments recorded in the provided review file. Therefore, no further fixes are required at this time. tests/unit_tests/mcp_service/dashboard/tool/test_list_dashboards_deleted_state.py |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41855 +/- ##
==========================================
- Coverage 64.77% 64.72% -0.06%
==========================================
Files 2689 2690 +1
Lines 148945 149128 +183
Branches 34369 34409 +40
==========================================
+ Hits 96486 96526 +40
- Misses 50693 50831 +138
- Partials 1766 1771 +5
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:
|
1 participant
SUMMARY
Adds a
deleted_stateparameter ("include"/"only") to thelist_chartsandlist_dashboardsMCP tools, mirroring the RESTchart_deleted_state/dashboard_deleted_staterison filters introduced with soft-delete (#40128 / #40129). Without it, an MCP client has no way to enumerate trashed objects — which makes trash effectively invisible to LLM agents (and unrestorable, once restore tools land: #41842).Design notes:
ChartDeletedStateFilter/DashboardDeletedStateFilterown the restore-audience scoping (only owners — or admins — may enumerate soft-deleted rows).ModelListCorepasses them throughBaseDAO.list's existingcustom_filtershook via a smallDeletedStateBoundFilteradapter, because the DAO invokes custom filters asapply(query, None)and the FAB filter treatsNoneas "live rows only" — the adapter binds the caller's actual value. Keeping the audience scoping in one class means the cross-entity deleted-state contract can't drift between REST and MCP.skip_visibility_filtercontext manager. The soft-delete ORM listener appendsdeleted_at IS NULLat execution time, andBaseDAO.listexecutes twice (count + fetch), so a per-query bypass is not enough. The context manager guarantees release even on exceptions, so code running later in the same request sees normal filtered visibility — the same scope-discipline the REST layer implements viaSoftDeleteApiMixin.deleted_atis forced into loaded columns wheneverdeleted_stateis used (and added to theChartInfo/DashboardInfoschemas + serializers), so trashed rows are distinguishable from live ones inincludemode.ModelListCoredoesn't configure a deleted-state filter reject the parameter with a clear error.Part of a series filling out MCP resource-lifecycle coverage (delete: #41472, restore: #41842).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (MCP service tools; no UI changes)
TESTING INSTRUCTIONS
Unit tests (10 new):
pytest tests/unit_tests/mcp_service/chart/tool/test_list_charts_deleted_state.py \ tests/unit_tests/mcp_service/dashboard/tool/test_list_dashboards_deleted_state.py \ tests/unit_tests/mcp_service/test_mcp_core.pyThe underlying filter semantics (audience scoping, include/only behavior) are covered by the existing REST integration suite (
tests/integration_tests/charts/soft_delete_tests.py), and theskip_visibility_filtercontext manager bytests/unit_tests/models/test_soft_delete_mixin.py— this PR reuses both mechanisms rather than duplicating them.Manual: with
FEATURE_FLAGS = {"SOFT_DELETE": True}, delete a chart you own, then calllist_chartswith{"deleted_state": "only"}— the trashed chart is returned with a non-nulldeleted_at. As a non-admin, another user's trashed charts do not appear.ADDITIONAL INFORMATION
SOFT_DELETE(to produce trashed rows; the parameter itself is not gated)🤖 Generated with Claude Code
https://claude.ai/code/session_014z1fQxTadTEvpCJLx9WnB8