aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3362913794
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -288,6 +288,33 @@ def validate_search_and_filters(self) ->
"ListDashboardsRequest":
return self
+DEFAULT_GET_DASHBOARD_INFO_COLUMNS: List[str] = [
+ "id",
+ "dashboard_title",
+ "slug",
+ "description",
+ "certified_by",
+ "certification_details",
+ "published",
+ "is_managed_externally",
+ "external_url",
+ "created_on",
+ "changed_on",
+ "uuid",
+ "url",
+ "created_on_humanized",
+ "changed_on_humanized",
+ "chart_count",
+ "tags",
+ "charts",
+ "native_filters",
+ "cross_filters_enabled",
+ "is_permalink_state",
+ "permalink_key",
+ "filter_state",
+]
Review Comment:
This is reviewing a stale revision. At HEAD `b9d8722f40`,
`DEFAULT_GET_DASHBOARD_INFO_COLUMNS` (schemas.py:291-314) does **not** include
`filter_state` or `css` — the lean default and the field description are
consistent. `filter_state` is only appended at runtime in
`get_dashboard_info.py` when `permalink_key` is supplied and the caller has not
overridden `select_columns`, so default responses stay lean while permalink
callers still get the data they asked for.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -298,6 +332,15 @@ def validate_identifier_or_form_data_key(self) ->
"GetChartInfoRequest":
)
return self
+ @field_validator("select_columns", mode="before")
+ @classmethod
+ def _parse_select_columns(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_CHART_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
Review Comment:
Already handled at HEAD `b9d8722f40`.
`GetChartInfoRequest._parse_select_columns` (chart/schemas.py:335-343) falls
back to the lean default when `parse_json_or_list` returns an empty list:
`return parsed if parsed else list(DEFAULT_GET_CHART_INFO_COLUMNS)`. So `""` /
`[]` inputs resolve to the lean default rather than disabling filtering. The
same empty-list fallback is applied in the dashboard, dataset `select_columns`,
and dataset `column_fields` validators.
##########
superset/mcp_service/dashboard/tool/get_dashboard_info.py:
##########
@@ -247,6 +249,21 @@ async def get_dashboard_info(
result.is_permalink_state,
)
)
+ # When permalink_key is supplied and the caller did not explicitly
+ # override select_columns, ensure filter_state is present so the
+ # caller gets the data they came for.
+ effective_select_columns = list(request.select_columns)
+ if (
+ request.permalink_key
+ and effective_select_columns ==
list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
+ and "filter_state" not in effective_select_columns
Review Comment:
Fixed in `b9d8722f40`. Removed the redundant `and "filter_state" not in
effective_select_columns` clause. (Note: the default columns do **not** include
`filter_state`; the clause was redundant because the preceding
`effective_select_columns == list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)` check
already guarantees the list equals the default, which excludes `filter_state` —
so the membership test was always true.)
--
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]
