Copilot commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3319853263
##########
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:
`DEFAULT_GET_DASHBOARD_INFO_COLUMNS` includes `filter_state`, but the
`select_columns` field description (and the PR description) says the default
excludes `filter_state`. This makes the default response larger than intended
and is misleading for callers. Either remove `filter_state` from the default
list (and/or include it only when `permalink_key` is set), or update the
description/PR expectations so defaults and behavior match.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_info.py:
##########
@@ -365,7 +365,8 @@ async def
test_restricted_user_redacts_saved_chart_data_model_fields(
assert result["datasource_name"] is None
assert result["datasource_type"] is None
assert result["filters"] is None
- assert result["form_data"] is None
+ # form_data is excluded from default select_columns, so it won't be in
result
+ assert "form_data" not in result
Review Comment:
This test asserts the saved-chart path omits `form_data` by default, but
there’s no assertion covering the unsaved-chart path (`form_data_key` without
`identifier`). Since the tool currently returns a Pydantic model on that path,
`form_data` can still show up (or be redacted to `null`) despite the new
default `select_columns`. Add a unit test that calls `get_chart_info` with only
`form_data_key` and verifies the default response shape matches the saved-chart
behavior (i.e., `form_data` omitted unless explicitly selected).
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -213,7 +214,7 @@ def _apply_unsaved_state_override(result: ChartInfo,
form_data_key: str) -> None
)
async def get_chart_info(
request: GetChartInfoRequest, ctx: Context
-) -> ChartInfo | ChartError:
+) -> dict[str, Any] | ChartError:
"""Get chart metadata by ID or UUID.
Review Comment:
The function signature now advertises a `dict[str, Any]` success payload,
but the unsaved-chart branch (`form_data_key` without `identifier`) still
returns a `ChartInfo` model directly. That bypasses the new `select_columns`
filtering (middleware calls `model_dump()` without context) and reintroduces
large `form_data` responses for unsaved charts, undermining the PR’s goal.
Consider returning `result.model_dump(mode='json', context={'select_columns':
request.select_columns})` (and applying redaction before dumping) on *all*
success paths so defaults are consistent.
--
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]
