feat(mcp): add list and get tools for saved queries and query history#40346
feat(mcp): add list and get tools for saved queries and query history#40346aminghadersohi wants to merge 3 commits into
Conversation
Implements list_saved_queries, get_saved_query_info, list_queries, and get_query_info MCP tools in new saved_query/ and query/ domains. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
api
Review: list/get tools for saved queries and query historyThorough pass over the 4 new MCP tools. Summary: clean, well-tested, correctly secured, and faithful to the existing Verified (all good ✅)
Suggested changes (none blocking)1. 2. 3. 4. Minor test-coverage gaps (optional). No test exercises 5. Notes (consistent with existing code, not PR issues)
Item #1 is the only one I'd suggest addressing before merge; the rest are nits. |
…ies tools - Drop sql from DEFAULT_QUERY_COLUMNS (25 rows × full SQL bodies is too heavy for default LLM responses; callers use select_columns or get_query_info to access SQL) - Add changed_on to SORTABLE_QUERY_COLUMNS for queries (column is indexed on the model, same treatment as saved queries) - Remove QueryError.create() and SavedQueryError.create() dead code - Fix list_queries instruction text in app.py: use '(1-based pagination)' to match the wording used by all other list tools - Add tests: select_columns field projection and invalid order_column rejection
|
Thanks for the thorough review! Addressed in ecec0a0: #1 — Addressed: Dropped #2 — Addressed: Removed both #3 — Addressed: Added #4 — Addressed (partially): Added a #5 — Addressed: Fixed the |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| return SavedQueryError( | ||
| error=f"Failed to get saved query info: {str(e)}", | ||
| error_type="InternalError", | ||
| timestamp=datetime.now(timezone.utc), | ||
| ) |
There was a problem hiding this comment.
Suggestion: The error response includes raw exception text (str(e)) in the payload returned to clients, which can disclose internal implementation details (database errors, query internals, stack context). Return a generic user-facing message instead and keep full exception details only in server logs. [security]
Severity Level: Critical 🚨
- ❌ MCP `get_saved_query_info` leaks raw backend exception text.
- ⚠️ Internal database or ORM errors exposed to MCP clients.
- ⚠️ LLM consumers may see sensitive implementation details.Steps of Reproduction ✅
1. Start the MCP server, which imports and registers the saved query tools in
`superset/mcp_service/app.py` (see imports of `get_saved_query_info` and
`list_saved_queries` at `superset/mcp_service/app.py:628-651`, including the specific
`from superset.mcp_service.saved_query.tool import get_saved_query_info` line reported by
Grep at `app.py:648`).
2. An MCP client (e.g., an LLM tool consumer) invokes the `get_saved_query_info` tool by
name, which causes FastMCP to execute the `get_saved_query_info` coroutine defined with
the `@tool` decorator in
`superset/mcp_service/saved_query/tool/get_saved_query_info.py:43-54`.
3. Inside `get_saved_query_info`, the function constructs a `ModelGetInfoCore` with
`SavedQueryDAO` and calls `get_tool.run_tool(request.identifier)` within the `try` block
at `get_saved_query_info.py:82-95`; if this DAO call or any code in the `try` (for
example, a database connection failure or unexpected ORM error) raises an exception,
control flows into the `except Exception as e` block at `get_saved_query_info.py:115-123`.
4. In the `except` block, the function logs the error via `ctx.error` and then returns a
`SavedQueryError` instance at `get_saved_query_info.py:125-129` where the `error` field is
set to `f"Failed to get saved query info: {str(e)}"`, so the full raw exception message
`str(e)` is serialized according to the `SavedQueryError` schema in
`superset/mcp_service/saved_query/schemas.py:1-3` and sent back to the MCP client,
potentially exposing internal database error messages or other sensitive implementation
details to the caller.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/saved_query/tool/get_saved_query_info.py
**Line:** 125:129
**Comment:**
*Security: The error response includes raw exception text (`str(e)`) in the payload returned to clients, which can disclose internal implementation details (database errors, query internals, stack context). Return a generic user-facing message instead and keep full exception details only in server logs.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40346 +/- ##
==========================================
- Coverage 64.15% 64.11% -0.04%
==========================================
Files 2592 2600 +8
Lines 138861 139243 +382
Branches 32208 32237 +29
==========================================
+ Hits 89085 89278 +193
- Misses 48244 48430 +186
- Partials 1532 1535 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #bd7d73
Actionable Suggestions - 3
-
superset/mcp_service/saved_query/tool/get_saved_query_info.py - 1
- Inconsistent error handling · Line 115-115
-
superset/mcp_service/query/tool/list_queries.py - 1
- CWE-390: Redundant except-Raise · Line 146-156
-
tests/unit_tests/mcp_service/query/tool/test_query_tools.py - 1
- Broad exception catch in test · Line 301-301
Additional Suggestions - 1
-
tests/unit_tests/mcp_service/saved_query/tool/test_saved_query_tools.py - 1
-
Incomplete pagination assertions · Line 258-273Test `test_list_saved_queries_pagination_info` validates pagination metadata but is missing an assertion for the `count` field, which is part of the SavedQueryList schema (line 122 in schemas.py).
Code suggestion
--- a/tests/unit_tests/mcp_service/saved_query/tool/test_saved_query_tools.py +++ b/tests/unit_tests/mcp_service/saved_query/tool/test_saved_query_tools.py @@ -269,6 +269,7 @@ async def test_list_saved_queries_pagination_info(mock_list, mcp_server): ) data = json.loads(result.content[0].text) assert data["total_count"] == 25 + assert data["count"] == 3 assert data["page_size"] == 3 assert data["has_next"] is True assert data["has_previous"] is False
-
Review Details
-
Files reviewed - 17 · Commit Range:
5de1fb7..ecec0a0- superset/mcp_service/app.py
- superset/mcp_service/query/__init__.py
- superset/mcp_service/query/schemas.py
- superset/mcp_service/query/tool/__init__.py
- superset/mcp_service/query/tool/get_query_info.py
- superset/mcp_service/query/tool/list_queries.py
- superset/mcp_service/saved_query/__init__.py
- superset/mcp_service/saved_query/schemas.py
- superset/mcp_service/saved_query/tool/__init__.py
- superset/mcp_service/saved_query/tool/get_saved_query_info.py
- superset/mcp_service/saved_query/tool/list_saved_queries.py
- tests/unit_tests/mcp_service/query/__init__.py
- tests/unit_tests/mcp_service/query/tool/__init__.py
- tests/unit_tests/mcp_service/query/tool/test_query_tools.py
- tests/unit_tests/mcp_service/saved_query/__init__.py
- tests/unit_tests/mcp_service/saved_query/tool/__init__.py
- tests/unit_tests/mcp_service/saved_query/tool/test_saved_query_tools.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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
|
|
||
| return result | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Inconsistent exception handling between tools in the same module: list_saved_queries raises exceptions (line 159) while get_saved_query_info returns SavedQueryError responses (line 125). This inconsistency can confuse callers expecting uniform behavior.
Code Review Run #bd7d73
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| except Exception as e: | ||
| await ctx.error( | ||
| "Query listing failed: page=%s, page_size=%s, error=%s, error_type=%s" | ||
| % ( | ||
| request.page, | ||
| request.page_size, | ||
| str(e), | ||
| type(e).__name__, | ||
| ) | ||
| ) | ||
| raise |
There was a problem hiding this comment.
At line 146, a bare except Exception is caught but then immediately re-raised at line 156, which defeats the purpose of the catch block. The error logging is redundant since the GlobalErrorHandlerMiddleware will log the exception anyway. Compare with get_query_info.py (lines 118-121) which returns a QueryError response instead of re-raising, providing a consistent API contract that callers can handle. If re-raising is intended, remove the catch block entirely. (See also: CWE-390)
Code Review Run #bd7d73
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| """order_column not in SORTABLE_QUERY_COLUMNS must be rejected.""" | ||
| request = ListQueriesRequest(page=1, page_size=10, order_column="tab_name") | ||
| async with Client(mcp_server) as client: | ||
| with pytest.raises(Exception, match="Invalid order_column"): |
There was a problem hiding this comment.
Replace broad Exception catch with ValueError to match what the implementation raises at mcp_core.py:205. This prevents masking unrelated errors.
Code Review Run #bd7d73
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
SUMMARY
Adds four new MCP tools across two new domains (
saved_query/andquery/):list_saved_queries— List saved SQL queries owned by the current user with filtering (label, db_id, schema), search, and paginationget_saved_query_info— Get saved query details by numeric ID or UUIDlist_queries— List SQL query history with filtering (status, database_id, schema), defaulting to most-recent-first (ordered bystart_timedesc, page_size 25)get_query_info— Get query history details by numeric IDBoth domains follow the existing
database/,dataset/,chart/, anddashboard/patterns:ModelListCore/ModelGetInfoCorefor reuse, Pydantic schemas with field-level serialization context,@tooldecorators with RBAC, andevent_loggerinstrumentation.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend API tools only
TESTING INSTRUCTIONS
# Run unit tests for the new tools pytest tests/unit_tests/mcp_service/saved_query/ pytest tests/unit_tests/mcp_service/query/ADDITIONAL INFORMATION