fix(mcp): fall back to title match when dashboard slug lookup misses#39567
fix(mcp): fall back to title match when dashboard slug lookup misses#39567
Conversation
Many imported/example dashboards have empty slug fields, so get_dashboard_info with an agent-guessed slug (e.g. "world-banks-data") silently returned not_found. ModelGetInfoCore now tries an exact case-insensitive title match, then a slugified-title match, before giving up. When multiple titles slugify to the same value, returns ambiguous_identifier listing the candidate ids. The column name is sourced from DAO.title_column (set on DashboardDAO) so other tools can opt in without touching the core. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review Agent Run #3ecdb0Actionable Suggestions - 0Review 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}
| class _FakeOutput(BaseModel): | ||
| id: int | ||
| title: str |
There was a problem hiding this comment.
Suggestion: Add a class docstring describing the purpose of this test schema. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The class is newly introduced and has no docstring. If the custom rule requires
documentation for new class definitions, this is a real violation in the
existing code.
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:** tests/unit_tests/mcp_service/test_mcp_core.py
**Line:** 35:37
**Comment:**
*Custom Rule: Add a class docstring describing the purpose of this test schema.
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| class _FakeError(BaseModel): | ||
| error: str | ||
| error_type: str | ||
| timestamp: datetime |
There was a problem hiding this comment.
Suggestion: Add a class docstring explaining the role of this error schema in the tests. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The class has no docstring in the final file. This matches the suggestion and
constitutes a real documentation omission if the rule requires docstrings on
new class definitions.
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:** tests/unit_tests/mcp_service/test_mcp_core.py
**Line:** 40:43
**Comment:**
*Custom Rule: Add a class docstring explaining the role of this error schema in the tests.
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|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def _patch_id_or_slug_filter(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation for this fixture function (for example, an iterator/generator type) to satisfy typing requirements for new functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The fixture function is defined without a return type annotation. If the rule
requires explicit typing on new functions, this is a genuine violation in the
current code.
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:** tests/unit_tests/mcp_service/test_mcp_core.py
**Line:** 51:51
**Comment:**
*Custom Rule: Add an explicit return type annotation for this fixture function (for example, an iterator/generator type) to satisfy typing requirements for new functions.
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| yield | ||
|
|
||
|
|
||
| def _make_dashboard(id_: int, title: str, slug: str = "") -> MagicMock: |
There was a problem hiding this comment.
Suggestion: Add a short docstring describing what object this helper constructs and what inputs it expects. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The helper function has no docstring in the final file. That is a real
documentation gap if the custom rule mandates docstrings for added helpers.
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:** tests/unit_tests/mcp_service/test_mcp_core.py
**Line:** 59:59
**Comment:**
*Custom Rule: Add a short docstring describing what object this helper constructs and what inputs it expects.
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| ("leading--and--trailing--", "leading-and-trailing"), | ||
| ], | ||
| ) | ||
| def test_slugify_handles_edge_cases(identifier: str, expected_slug: str) -> None: |
There was a problem hiding this comment.
Suggestion: Add a test docstring summarizing the edge-case behavior validated by this parametrized test. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The parametrized test function has no docstring in the final file. If the
custom rule requires docstrings for new test functions, this is a real
violation.
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:** tests/unit_tests/mcp_service/test_mcp_core.py
**Line:** 212:212
**Comment:**
*Custom Rule: Add a test docstring summarizing the edge-case behavior validated by this parametrized test.
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 #39567 +/- ##
==========================================
- Coverage 64.57% 64.56% -0.01%
==========================================
Files 2562 2563 +1
Lines 133535 133569 +34
Branches 31030 31036 +6
==========================================
+ Hits 86228 86238 +10
- Misses 45815 45839 +24
Partials 1492 1492
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| query = db.session.query(model_class) | ||
| if self.query_options: | ||
| query = query.options(*self.query_options) | ||
| matches = [ | ||
| obj | ||
| for obj in query.all() | ||
| if _slugify(getattr(obj, self.title_column_name, "") or "") == target |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The slugified-title fallback in ModelGetInfoCore queries Dashboard rows directly via db.session.query without applying the DAO's base_filter (DashboardAccessFilter), so get_dashboard_info can return dashboards the user is not allowed to access when resolving slug-like identifiers.
Suggestion: Build the fallback query through the DAO (or call BaseDAO._apply_base_filter/DashboardAccessFilter on the query) before scanning titles so that only rows allowed by the base_filter are considered for slugified-title matches.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/mcp_core.py
**Line:** 317:323
**Comment:**
*CRITICAL: The slugified-title fallback in ModelGetInfoCore queries Dashboard rows directly via db.session.query without applying the DAO's base_filter (DashboardAccessFilter), so get_dashboard_info can return dashboards the user is not allowed to access when resolving slug-like identifiers.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| import re | ||
| from abc import ABC, abstractmethod | ||
| from datetime import datetime, timezone |
There was a problem hiding this comment.
Suggestion: This fallback performs query.all() with full query_options, which in dashboard lookups includes heavy eager loads; on large instances this can load every dashboard plus related slices/owners/tags into memory just to find one match. Restrict the scan to lightweight columns (e.g., id/title only, without eager options), then fetch the single matched row with query_options. [performance]
Severity Level: Critical 🚨
- ❌ get_dashboard_info slug-fallback loads every dashboard row eagerly.
- ⚠️ Large installations risk timeouts on slug-like lookups.
- ⚠️ Increased memory pressure from unnecessary relationship loading.Steps of Reproduction ✅
1. In `superset/mcp_service/dashboard/tool/get_dashboard_info.py:111–120`, see that
`eager_options` includes multiple `subqueryload` calls for `Dashboard.slices`,
`Slice.owners`, `Slice.tags`, `Dashboard.owners`, `Dashboard.tags`, and `Dashboard.roles`,
and these are passed to `ModelGetInfoCore` as `query_options=eager_options` at lines
122–131.
2. For a Superset instance with a large number of dashboards and charts (hundreds or
thousands), ensure there exists at least one dashboard whose `slug` is empty but whose
`dashboard_title` slugifies to a predictable value such as `"world-banks-data"`.
3. From any MCP client, call the `get_dashboard_info` tool with `request.identifier` set
to that slug-like value so that `ModelGetInfoCore._find_object` in
`superset/mcp_service/mcp_core.py:47–81` exhausts ID/UUID/slug lookups and falls through
to `_find_by_slugified_title`.
4. In `_find_by_slugified_title` (`superset/mcp_service/mcp_core.py`, lines shown in this
PR from `if self.query_options:` through the `matches = [...]` list comprehension), the
code applies the heavy `query.options(*self.query_options)` and then executes
`query.all()`, loading every dashboard plus all eagerly-loaded related
slices/owners/tags/roles into memory just to filter in Python, causing high memory usage
and slow responses for this fallback path.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/mcp_core.py
**Line:** 18:23
**Comment:**
*Performance: This fallback performs `query.all()` with full `query_options`, which in dashboard lookups includes heavy eager loads; on large instances this can load every dashboard plus related slices/owners/tags into memory just to find one match. Restrict the scan to lightweight columns (e.g., id/title only, without eager options), then fetch the single matched row with `query_options`.
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 fix1 participant
SUMMARY
get_dashboard_infosilently returnederror_type: not_foundwhen an agent passed a slug-like identifier (e.g."world-banks-data") for a dashboard whoseslugfield was empty. Many imported / example dashboards ship with empty slugs, so agents that guess the slug from the dashboard title (the natural thing to do) would hit a dead end.After the normal id / UUID / slug lookups miss,
ModelGetInfoCorenow scans dashboard titles, normalizes each title and the identifier via a shared_slugifyhelper (lowercases, drops apostrophes, collapses non-alphanumerics to hyphens), and returns the first match.If multiple titles slugify to the same value, it logs a warning and returns the first — collisions in real dashboards are rare and the
caller can always disambiguate by id or UUID.
The fallback column is opt-in per entity:
DashboardDAOdeclarestitle_column = "dashboard_title", whichModelGetInfoCorepicks up viagetattr(dao_class, "title_column", None).Other MCP tools can enable the same behavior by setting one attribute on their DAO — no core or tool changes required.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — behavior change is in the MCP service's JSON responses.
Before (master):
{ "error": "DashboardInfo with identifier 'world-banks-data' not found", "error_type": "not_found" }After:
{ "id": 1, "dashboard_title": "World Bank's Data", "slug": "", ... }TESTING INSTRUCTIONS
Find (or import) a dashboard whose slug field is empty but whose title slugifies to something predictable — e.g. the stock world_bank_data.py example ("World Bank's Data").
Call the MCP tool:
get_dashboard_info(request={"identifier": "world-banks-data"})
Expected: returns the dashboard (not error_type: not_found).
Sanity checks that should behave the same as before:
{"identifier": } — resolves by id.
{"identifier": ""} — resolves by UUID.
{"identifier": ""} — resolves by slug when the slug is non-empty.
{"identifier": "definitely-not-a-dashboard"} — still returns error_type: not_found (no over-matching).
Ambiguous case: create two dashboards whose titles slugify to the same value. Call get_dashboard_info with that slug. Expected: returns the first match and the server logs a warning naming all candidate ids.
Unit tests:
pytest tests/unit_tests/mcp_service/test_mcp_core.py \
tests/unit_tests/mcp_service/dashboard/
All 107 pass.
ADDITIONAL INFORMATION