fix(mcp): point get_dataset_info url to explore view instead of legacy tablemodelview edit - #39838
Conversation
…y tablemodelview edit
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39838 +/- ##
==========================================
+ Coverage 64.35% 64.37% +0.01%
==========================================
Files 2569 2569
Lines 134680 134684 +4
Branches 31254 31255 +1
==========================================
+ Hits 86679 86707 +28
+ Misses 46505 46479 -26
- Partials 1496 1498 +2
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:
|
Code Review Agent Run #86f22eActionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left one small test-coverage suggestion below. The URL behavior itself looks directionally right to me, and all line numbers are verified against HEAD c6c1afd.
Other suggestion
-
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:994This fixture now sets the mock dataset URL to the Explore route, but
serialize_dataset_objectbuilds the returnedurlfromget_superset_base_url()anddataset.idrather than fromdataset.url. That means the focusedget_dataset_infotest could still pass if the serializer regressed back to/tablemodelview/edit/....Small suggestion: would it be worth patching
get_superset_base_urlintest_get_dataset_info_successand assertingdata["url"]equals the full Explore URL? Happy to keep as-is if you think the existing coverage elsewhere is enough.
Praise
-
superset/mcp_service/dataset/schemas.py:710Nice alignment with the existing MCP Explore-link shape for datasets. Returning the user-facing Explore route here feels much more useful than sending clients to the legacy FAB edit view.
|
Thanks @richardfogaca! Good catch — the fixture was setting Done in 66209ba: patched |
Code Review Agent Run #d294f3Actionable 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 |
…y tablemodelview edit (apache#39838)
…y tablemodelview edit (apache#39838)
2 participants
SUMMARY
The
get_dataset_infoMCP tool was returning aurlfield pointing to/tablemodelview/edit/{id}— the legacy Flask-AppBuilder admin edit endpoint. This is not a useful user-facing URL.The URL now points to the Explore view:
/explore/?datasource_type=table&datasource_id={id}, which is the standard user-facing entry point for interacting with a dataset. This matches the pattern used by the chart info tool (/explore/?slice_id={id}) and the explore link generation utilities in the MCP service.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — URL string change only.
TESTING INSTRUCTIONS
get_dataset_infowith a valid dataset ID via the MCP client.urlfield contains/explore/?datasource_type=table&datasource_id={id}rather than/tablemodelview/edit/{id}.ADDITIONAL INFORMATION