gabotorresruiz commented on code in PR #44338:
URL: https://github.com/apache/superset/pull/44338#discussion_r4030095226
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -86,6 +91,106 @@ def _build_unsaved_chart_info(form_data_key: str) ->
ChartInfo | ChartError:
)
+def _get_explore_permalink(
+ permalink_key: str,
+) -> ExplorePermalinkValue | ChartError:
+ """Read an Explore permalink, enforcing the same access checks as Explore.
+
+ ``GetExplorePermalinkCommand`` checks access to the permalink's datasource
+ and, when it references a saved chart, to that chart.
+ """
+ from superset.commands.explore.permalink.get import
GetExplorePermalinkCommand
+
+ try:
+ value = GetExplorePermalinkCommand(permalink_key).run()
+ except ForbiddenError:
+ return ChartError(
+ error="You do not have access to the chart or dataset in this
permalink.",
+ error_type="PermalinkAccessDenied",
+ )
+ except (CommandException, SQLAlchemyError, ValidationError, ValueError) as
ex:
+ # ValidationError: the permalink's datasource no longer exists or has
an
+ # invalid type (raised by the access check).
+ logger.warning("Failed to read explore permalink: %s", ex)
+ return ChartError(
+ error="The explore permalink could not be read. Check the key.",
+ error_type="InvalidPermalink",
+ )
+ if not value:
+ return ChartError(
+ error="No explore permalink found for permalink_key.",
+ error_type="NotFound",
+ )
+ return value
+
+
+def _permalink_chart_id(permalink: ExplorePermalinkValue) -> int | None:
+ """Return the saved chart a permalink was created from, if any.
+
+ ``chartId`` is copied from the client-supplied ``formData.slice_id``, so
+ it is not guaranteed to be an int.
+ """
+ try:
+ return int(permalink.get("chartId") or 0) or None
+ except (TypeError, ValueError):
+ return None
+
+
+def _permalink_form_data(permalink: ExplorePermalinkValue) -> dict[str, Any]:
+ state = permalink.get("state")
+ form_data = state.get("formData") if isinstance(state, dict) else None
+ return dict(form_data) if isinstance(form_data, dict) else {}
+
+
+def _permalink_datasource(
+ permalink: ExplorePermalinkValue,
+) -> tuple[str | None, str | None]:
+ """Return the (name, type) of the datasource a permalink was built on.
+
+ A permalink's form_data carries the datasource as an opaque "<id>__<type>"
+ string, so the name has to be resolved from the ids the permalink stores
+ alongside it. Access to that datasource was already checked by
+ ``GetExplorePermalinkCommand``.
+ """
+ datasource_type = permalink.get("datasourceType") or
DatasourceType.TABLE.value
+ datasource_id = permalink.get("datasourceId") or permalink.get("datasetId")
+ if not datasource_id:
+ return None, str(datasource_type)
+ try:
+ from superset.daos.datasource import DatasourceDAO
+
+ datasource = DatasourceDAO.get_datasource(
+ datasource_type=DatasourceType(datasource_type),
+ database_id_or_uuid=datasource_id,
+ )
+ except Exception: # noqa: BLE001
+ # A deleted or unsupported datasource must not sink the whole read;
+ # the rest of the permalink state is still worth returning.
+ logger.warning(
+ "Could not resolve permalink datasource %s of type %s",
+ datasource_id,
+ datasource_type,
+ )
+ return None, str(datasource_type)
+ return datasource.datasource_name, str(datasource_type)
Review Comment:
Blocker: `datasource.datasource_name` sits outside the `try` above it, and
`Query` has no such attribute, so an unsaved permalink built on a SQL Lab query
raises `AttributeError` out of the tool instead of returning the permalink
state.
`check_datasource_access` accepts `DatasourceType.QUERY`
(`superset/explore/utils.py:75`), and SQL Lab builds the Explore datasource as
`<query_id>__query`
(`superset-frontend/src/SqlLab/components/ResultSet/index.tsx:301`), so
`DatasourceDAO.get_datasource` hands back a `Query`, which only exposes `name`
(`superset/models/sql_lab.py:249`). The `except Exception` wraps only the DAO
call, so this attribute access is unguarded.
I verified it on this branch against a real metadata database. I created two
permalinks with `CreateExplorePermalinkCommand`, one on `<table_id>__table` and
one on `<query_id>__query`, then called the tool through a `fastmcp` `Client`:
* table: `{"datasource_name": "probe_table", "datasource_type": "table",
"is_permalink_state": true}`
* query: `ToolError: Error calling tool 'get_chart_info': 'Query' object has
no attribute 'datasource_name'`
That second one is the "never saved as a chart" case from your testing
instructions, for any Explore session opened from SQL Lab results. Copilot has
the same point in its suppressed list; I am confirming it here with the repro
because it is easy to miss in there.
A fix that leaves the table case untouched:
```suggestion
name = getattr(datasource, "datasource_name", None) or getattr(
datasource, "name", None
)
return name, str(datasource_type)
```
`getattr` with a default is what the saved chart serializer already does for
this same field (`superset/mcp_service/chart/schemas.py:590`), and `.name` is
how `GetExploreCommand` labels a datasource
(`superset/commands/explore/get.py:169`). I applied exactly that locally: the
query permalink then comes back as `datasource_name: "sqllab_untitled_..."`
with `datasource_type: "query"`, the table permalink is unchanged, and all 17
tests in `test_get_chart_info_permalink.py` still pass.
For coverage, `test_permalink_without_saved_chart_names_its_datasource` is
the natural place for a `query` variant. It needs a real `Query()` as the
`get_datasource` return value rather than a `MagicMock`, since a mock answers
to any attribute and would pass either way.
--
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]
