aminghadersohi commented on code in PR #37183:
URL: https://github.com/apache/superset/pull/37183#discussion_r2770669609
##########
superset/mcp_service/dashboard/tool/get_dashboard_info.py:
##########
@@ -28,6 +28,8 @@
from fastmcp import Context
from superset_core.mcp import tool
+from superset.dashboards.permalink.exceptions import
DashboardPermalinkGetFailedError
+from superset.dashboards.permalink.types import DashboardPermalinkValue
Review Comment:
These imports are correct. `superset/dashboards/permalink/exceptions.py` and
`superset/dashboards/permalink/types.py` exist and import fine. The
`superset.commands.dashboard.permalink` module is a separate location for the
command implementation — the types and exceptions live under
`superset.dashboards.permalink.*`.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -29,6 +30,8 @@
if TYPE_CHECKING:
from superset.models.slice import Slice
+from superset.commands.exceptions import CommandException
+from superset.commands.explore.form_data.parameters import CommandParameters
Review Comment:
False positive. `superset/commands/explore/form_data/parameters.py` exists
and contains `CommandParameters`. Both `superset.commands.explore.parameters`
and `superset.commands.explore.form_data.parameters` exist as separate modules
— the form_data one is the correct import for the form_data command usage here.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -42,6 +45,21 @@
logger = logging.getLogger(__name__)
+def _get_cached_form_data(form_data_key: str) -> str | None:
+ """Retrieve form_data from cache using form_data_key.
+
+ Returns the JSON string of form_data if found, None otherwise.
+ """
+ from superset.commands.explore.form_data.get import GetFormDataCommand
+
+ try:
+ cmd_params = CommandParameters(key=form_data_key)
+ return GetFormDataCommand(cmd_params).run()
+ except (KeyError, ValueError, CommandException) as e:
+ logger.warning("Failed to retrieve form_data from cache: %s", e)
Review Comment:
Already handled. `TemporaryCacheGetFailedError` extends `CommandException`
(see `superset/commands/temporary_cache/exceptions.py:32`), which is already
caught in the `except (KeyError, ValueError, CommandException)` handler.
##########
superset/mcp_service/dashboard/tool/get_dashboard_info.py:
##########
@@ -71,14 +110,45 @@ async def get_dashboard_info(
result = tool.run_tool(request.identifier)
if isinstance(result, DashboardInfo):
+ # If permalink_key is provided, retrieve filter state
+ if request.permalink_key:
+ await ctx.info(
+ "Retrieving filter state from permalink: permalink_key=%s"
+ % (request.permalink_key,)
+ )
+ permalink_value = _get_permalink_state(request.permalink_key)
Review Comment:
This is a codebase-wide pattern — all MCP tools call synchronous Superset
DAOs/commands directly in async functions. Addressing this would be a broader
refactor across all tools, not specific to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
