You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/05/27 18:10:47 UTC
[PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
aminghadersohi opened a new pull request, #40473:
URL: https://github.com/apache/superset/pull/40473
## Summary
The three `*_info` tools (`get_dashboard_info`, `get_chart_info`, `get_dataset_info`) could return responses exceeding 91KB for large resources, causing `ResponseSizeGuardMiddleware` to truncate responses into an unusable state for MCP clients.
Root cause: `DashboardInfo`, `ChartInfo`, and `DatasetInfo` already have `_filter_fields_by_context` model serializers that support context-based field filtering, but none of their request schemas exposed a `select_columns` parameter β so the filtering was never invoked.
### Changes
- **`get_dashboard_info`**: adds `select_columns` to `GetDashboardInfoRequest` with a lean default that excludes `css` (raw CSS, can be many KB) and `filter_state` (only relevant with `permalink_key`).
- **`get_chart_info`**: adds `select_columns` to `GetChartInfoRequest` with a lean default that excludes `form_data` (full chart config dict, can be 50KB+). Callers that need the raw config can add `'form_data'` explicitly.
- **`get_dataset_info`**: adds `select_columns` (lean top-level field set) and `column_fields` (per-column field set, defaults to `column_name/type/is_dttm`) to `GetDatasetInfoRequest`. Adds `_filter_column_fields_by_context` to `TableColumnInfo` so per-column filtering applies during serialization.
All three tools now call `result.model_dump(context={"select_columns": ...})` so filtering fires on every call. Default response size for a large dashboard drops from ~91KB to a few KB. Callers can opt in to wider responses by passing explicit `select_columns` lists.
## Testing
- Existing tests for all three tools continue to pass
- Default response for large dashboards stays within the MCP token limit
- Explicit `select_columns` override returns only the requested fields
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4568400536
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4581679055
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 3c917c7f9968177b467aebd71bfc8cc218c47a73 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a1a6a18c4363b0008515139 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-40473--superset-docs-preview.netlify.app](https://deploy-preview-40473--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctRCoQgEIDhu8zzSooVbTfoGNNoq6COqEER3X19-OF_-R7IhWNusAJ8YC-YyPXH6NPPobGlsvPD4a8hUhY-HSwac6ii2mCpCeJwxlS7NTYHvjfT9YwKewuNeta7lHKZ1KT0F94_) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40473--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4585139852
## Review feedback addressed β commit be1a407188
Addressing the BLOCKER, HIGH, and MEDIUM items from the review:
**BLOCKER: Inline imports β module top-level** (`chart/schemas.py`, `dashboard/schemas.py`, `dataset/schemas.py`)
- Moved all `from superset.mcp_service.utils.schema_utils import parse_json_or_list / parse_json_or_model_list` from inside validator methods to module-level imports. This eliminates the anti-pattern in both the new validators and the pre-existing `parse_filters` / `parse_select_columns` validators in `ListXxxRequest`.
**HIGH: `List[str]` β `list[str]`**
- Updated `DEFAULT_GET_CHART_INFO_COLUMNS`, `DEFAULT_GET_DASHBOARD_INFO_COLUMNS`, `DEFAULT_GET_DATASET_INFO_COLUMNS`, `DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS` to `list[str]`.
- Updated `select_columns` and `column_fields` field annotations on `GetChartInfoRequest`, `GetDashboardInfoRequest`, `GetDatasetInfoRequest` to `list[str]`.
**MEDIUM: Validator return types `-> Any` β `-> list[str]`**
- Updated `_parse_select_columns` in all three request schemas and `_parse_column_fields` in `GetDatasetInfoRequest`.
**MEDIUM: `-> None` on test methods**
- Added `-> None` return annotations to all 15 test methods in `TestGetChartInfoRequestSchema`, `TestResolveFilterOperatorAndValue`, and `TestBuildAppliedDashboardFilters`.
**MEDIUM: Tests for lean default behavior**
- Added `test_get_dashboard_info_default_select_columns_excludes_css` β verifies that a default `get_dashboard_info` call omits `css` and `filter_state` while including `id`, `dashboard_title`, `charts`, `native_filters`.
- Added `test_get_dataset_info_default_select_columns_excludes_verbose_fields` β verifies that a default `get_dataset_info` call omits `params`, `template_params`, `extra` while including `id`, `table_name`, `columns`, `metrics`.
**NIT: `filter_state` in description vs defaults** β The description already says the default excludes `filter_state`, and the default list at the current HEAD does not include it (removed in commit 3c917c7f99). The description is accurate.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328083527
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99: `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
Fixed in 3c917c7f99: same empty-list fallback applied.
##########
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:
Review Comment:
Fixed in 3c917c7f99.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
Fixed in 3c917c7f99.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3327885004
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
_parse_select_columns treats an empty string / empty list as a valid value, which results in select_columns=[] and disables filtering (because the model serializer checks truthiness). That can unexpectedly return the full (potentially huge) payload when a caller passes "" or [] for select_columns.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
_parse_select_columns treats an empty string / empty list as a valid value, which results in select_columns=[] and disables filtering (because the model serializer checks truthiness). That can unexpectedly return the full (potentially huge) payload when a caller passes "" or [] for select_columns.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
_parse_select_columns treats "" / [] as select_columns=[], which disables filtering (because ChartInfo._filter_fields_by_context checks truthiness). This can accidentally re-enable full responses if a client sends an empty value; consider defaulting empty to the lean default (or raising).
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
GetDatasetInfoRequest._parse_column_fields treats "" / [] as column_fields=[], which disables the TableColumnInfo per-column filtering (the serializer checks truthiness) and can massively bloat datasets with many columns. Consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS (or raising).
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
_parse_select_columns treats "" / [] as select_columns=[], which disables filtering (because ChartInfo._filter_fields_by_context checks truthiness). This can accidentally re-enable full responses if a client sends an empty value; consider defaulting empty to the lean default (or raising).
##########
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 currently includes "filter_state", but the field docstring for GetDashboardInfoRequest says the default lean set excludes it (and the PR description indicates it should be excluded by default to avoid huge permalink payloads). Including it defeats the size reduction when permalink_key is provided.
##########
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:
Review Comment:
get_chart_info now returns a dict for the saved-chart path (model_dump with context), but the form_data_key-only (unsaved chart) path still returns a ChartInfo/ChartError directly and bypasses the select_columns filtering. This makes response shape inconsistent and can reintroduce large outputs for unsaved charts.
##########
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 currently includes "filter_state", but the field docstring for GetDashboardInfoRequest says the default lean set excludes it (and the PR description indicates it should be excluded by default to avoid huge permalink payloads). Including it defeats the size reduction when permalink_key is provided.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
GetDatasetInfoRequest._parse_select_columns treats "" / [] as select_columns=[], which disables DatasetInfo filtering (the serializer checks truthiness). That can unexpectedly return the full dataset payload; consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMNS (or raising).
##########
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 currently includes "filter_state", but the field docstring for GetDashboardInfoRequest says the default lean set excludes it (and the PR description indicates it should be excluded by default to avoid huge permalink payloads). Including it defeats the size reduction when permalink_key is provided.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
GetDatasetInfoRequest._parse_select_columns treats "" / [] as select_columns=[], which disables DatasetInfo filtering (the serializer checks truthiness). That can unexpectedly return the full dataset payload; consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMNS (or raising).
##########
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:
Review Comment:
get_chart_info now returns a dict for the saved-chart path (model_dump with context), but the form_data_key-only (unsaved chart) path still returns a ChartInfo/ChartError directly and bypasses the select_columns filtering. This makes response shape inconsistent and can reintroduce large outputs for unsaved charts.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
_parse_select_columns treats an empty string / empty list as a valid value, which results in select_columns=[] and disables filtering (because the model serializer checks truthiness). That can unexpectedly return the full (potentially huge) payload when a caller passes "" or [] for select_columns.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
GetDatasetInfoRequest._parse_column_fields treats "" / [] as column_fields=[], which disables the TableColumnInfo per-column filtering (the serializer checks truthiness) and can massively bloat datasets with many columns. Consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS (or raising).
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
GetDatasetInfoRequest._parse_select_columns treats "" / [] as select_columns=[], which disables DatasetInfo filtering (the serializer checks truthiness). That can unexpectedly return the full dataset payload; consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMNS (or raising).
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
_parse_select_columns treats "" / [] as select_columns=[], which disables filtering (because ChartInfo._filter_fields_by_context checks truthiness). This can accidentally re-enable full responses if a client sends an empty value; consider defaulting empty to the lean default (or raising).
##########
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:
Review Comment:
get_chart_info now returns a dict for the saved-chart path (model_dump with context), but the form_data_key-only (unsaved chart) path still returns a ChartInfo/ChartError directly and bypasses the select_columns filtering. This makes response shape inconsistent and can reintroduce large outputs for unsaved charts.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
GetDatasetInfoRequest._parse_column_fields treats "" / [] as column_fields=[], which disables the TableColumnInfo per-column filtering (the serializer checks truthiness) and can massively bloat datasets with many columns. Consider defaulting empty to DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS (or raising).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328111109
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
Fixed in commit 1f4b65d9ab β `_parse_select_columns` (and `_parse_column_fields` for datasets) now falls back to the lean default when `parse_json_or_list` returns an empty list, so empty string / empty list input no longer disables filtering.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3329456791
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,80 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
Review Comment:
Fixed in be1a407188: added `test_get_dataset_info_default_select_columns_excludes_verbose_fields` which calls `get_dataset_info` with default args and asserts (a) `params`, `template_params`, `extra` are absent from the result, and (b) core fields `id`, `table_name`, `columns`, `metrics` are present. Also added `test_get_dashboard_info_default_select_columns_excludes_css` with equivalent coverage for the dashboard side.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4624684311
Addressed the open review feedback:
**`richardfogaca`'s comment (get_dashboard_info filter_state):** When `permalink_key` is supplied and the caller has not explicitly overridden `select_columns`, the tool now explicitly ensures `filter_state` is in the effective columns before calling `model_dump`. This makes the intent clear and future-proofs against the default list changing β `filter_state` is always guaranteed to be present when a permalink was provided. Added `test_get_dashboard_info_permalink_key_includes_filter_state` to verify the behavior.
**Bito suggestion (GetDatasetInfoRequest.column_fields validators):** Added `TestGetDatasetInfoRequestValidators` in `test_dataset_tools.py` with four cases: JSON string input, explicit empty list, `None` (falls back to default), and field omitted (falls back to default).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3362915954
##########
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:
<!-- Bito Reply -->
The suggestion to remove the redundant condition is appropriate. Since the preceding check `effective_select_columns == list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)` already ensures the list matches the default columns, and those defaults do not include `filter_state`, the membership test `and "filter_state" not in effective_select_columns` was indeed always true and unnecessary.
**superset/mcp_service/dashboard/tool/get_dashboard_info.py**
```
if (
request.permalink_key
and effective_select_columns == list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
):
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328110750
##########
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:
Fixed in 1f4b65d9ab β the unsaved-chart branch (form_data_key only, no identifier) now calls `result.model_dump(mode="json", context={"select_columns": request.select_columns})`, matching the saved-chart path.
##########
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:
Fixed β `filter_state` was removed from `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` in commit 1f4b65d9ab. The lean default no longer includes it; callers who need it can pass it explicitly via `select_columns`.
##########
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:
Fixed β `filter_state` was removed from `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` in commit 1f4b65d9ab. The lean default no longer includes it; callers who need it can pass it explicitly via `select_columns`.
##########
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:
Fixed β `filter_state` was removed from `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` in commit 1f4b65d9ab. The lean default no longer includes it; callers who need it can pass it explicitly via `select_columns`.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,29 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ result = parse_json_or_list(value, "select_columns")
+ return result or list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
Review Comment:
Fixed β `filter_state` was removed from `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` in commit 1f4b65d9ab. The lean default no longer includes it; callers who need it can pass it explicitly via `select_columns`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4585493485
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4633454822
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328082870
##########
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:
Fixed in the follow-up commit 3c917c7f99. The unsaved-chart path now calls `result.model_dump(mode="json", context={"select_columns": request.select_columns})` β same pattern as the saved-chart path.
##########
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:
Added a new test `test_unsaved_chart_select_columns_filters_response` in 3c917c7f99 that verifies: (1) default select_columns excludes form_data from the unsaved-chart response, and (2) explicitly passing `select_columns=["viz_type", "form_data"]` includes it.
##########
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:
The `filter_state` and `permalink_key` fields are not included in the current `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` list β the docstring and defaults are consistent. The copilot reviewed an earlier version of the diff; the current HEAD already has the correct lean set.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3358257445
##########
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 says the default lean set excludes `filter_state` (and `get_dashboard_info` has logic to append it only when `permalink_key` is provided). Including it in the default makes permalink responses large by default and makes the `effective_select_columns` conditional a no-op.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
`_parse_select_columns` can return an empty list when callers send `""` or `[]` (since `parse_json_or_list` returns `[]`). Because `DashboardInfo._filter_fields_by_context` only filters when `select_columns` is truthy, an empty list disables filtering and returns the full payload (including large fields like `css`), which is the opposite of what `select_columns` implies and can re-trigger response truncation.
##########
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:
`GetChartInfoRequest._parse_select_columns` can return `[]` for `""`/`[]` inputs (because `parse_json_or_list` returns `[]`). Since `ChartInfo._filter_fields_by_context` only filters when `select_columns` is truthy, an empty list disables filtering and returns the full payload (including `form_data`), undermining the lean-default behavior and potentially re-triggering ResponseSizeGuard truncation.
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:
##########
@@ -1641,6 +1641,47 @@ async def test_default_columns_filters_actual_response_data(
)
+class TestGetDatasetInfoRequestValidators:
+ """Unit tests for GetDatasetInfoRequest field validators."""
+
+ def test_column_fields_json_string_parses_to_list(self):
+ """JSON string input for column_fields is decoded into a list."""
+ from superset.mcp_service.dataset.schemas import GetDatasetInfoRequest
+
+ request = GetDatasetInfoRequest(
+ identifier=1,
+ column_fields='["column_name","type"]',
+ )
+ assert request.column_fields == ["column_name", "type"]
+
+ def test_column_fields_empty_list_stays_empty(self):
+ """An explicit empty list for column_fields is preserved as-is."""
+ from superset.mcp_service.dataset.schemas import GetDatasetInfoRequest
+
+ request = GetDatasetInfoRequest(identifier=1, column_fields=[])
+ assert request.column_fields == []
Review Comment:
This test locks in behavior where `column_fields=[]` is preserved. With the current serializers (`if column_fields:`), an empty list disables filtering and returns *all* per-column fields, which is likely to bloat responses and defeat the lean-default goal. If the intention is βempty means default lean fieldsβ (same motivation as `select_columns`), update the expectation accordingly.
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -333,6 +334,11 @@ async def get_chart_info(
error = await _attach_dashboard_filters(result, request.dashboard_id, ctx)
if error is not None:
return error
+
+ return result.model_dump(
+ mode="json",
+ context={"select_columns": request.select_columns},
+ )
Review Comment:
Only the saved-chart path returns `result.model_dump(..., context={"select_columns": ...})`. The form_data_key-only (unsaved) path still returns a `ChartInfo` model directly, so `select_columns` filtering is never applied there and large `form_data` payloads can still be returned by default. To achieve consistent lean defaults, the unsaved path should also apply redaction (if needed) and then `model_dump(mode="json", context={"select_columns": request.select_columns})` before returning.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
`GetDatasetInfoRequest` validators can return empty lists for `""`/`[]` inputs (via `parse_json_or_list`). Because `DatasetInfo._filter_fields_by_context` and `TableColumnInfo._filter_column_fields_by_context` only filter when the lists are truthy, an empty list disables filtering and returns the full dataset payload / full per-column payload (the opposite of what `select_columns` / `column_fields` implies), which can reintroduce oversized responses.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4582127703
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3329455876
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,28 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -297,6 +331,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:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,78 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
+
+
class GetDatasetInfoRequest(MetadataCacheControl):
"""Request schema for get_dataset_info with support for ID or UUID."""
identifier: Annotated[
int | str,
Field(description="Dataset identifier - can be numeric ID or UUID string"),
]
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes verbose fields like params, template_params, "
+ "extra, tags, certification_details. Pass an explicit list to "
+ "override (e.g. ['id','table_name','columns'] for minimal output)."
+ ),
+ ),
+ ]
+ column_fields: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS),
+ description=(
+ "Per-column fields to include for entries in 'columns'. Defaults "
+ "to ['column_name','type','is_dttm']. Pass a wider list to "
+ "include 'verbose_name','groupby','filterable','description' "
+ "when needed."
+ ),
+ ),
+ ]
+
+ @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_DATASET_INFO_COLUMNS)
+ return parse_json_or_list(value, "select_columns")
+
+ @field_validator("column_fields", mode="before")
+ @classmethod
+ def _parse_column_fields(cls, value: Any) -> Any:
+ from superset.mcp_service.utils.schema_utils import parse_json_or_list
+
+ if value is None:
+ return list(DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS)
+ return parse_json_or_list(value, "column_fields")
Review Comment:
Fixed in 3c917c7f99 (previous commit): `_parse_select_columns` now falls back to the lean default when `parse_json_or_list` returns an empty list (e.g. from `""` or `[]` input). Same fix applied to chart and dataset schemas and the `_parse_column_fields` validator.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328111432
##########
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:
Review Comment:
Fixed in commit 1f4b65d9ab β the unsaved-chart path now calls `result.model_dump(mode="json", context={"select_columns": request.select_columns})`, consistent with the saved-chart path.
##########
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:
Review Comment:
Fixed in commit 1f4b65d9ab β the unsaved-chart path now calls `result.model_dump(mode="json", context={"select_columns": request.select_columns})`, consistent with the saved-chart path.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3329455717
##########
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:
Fixed in be1a407188: `filter_state` is not in `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` (confirmed at line 291β314 of the current HEAD). The docstring and default list are consistent.
##########
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:
Fixed in be1a407188: `filter_state` is not in `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` (confirmed at line 291β314 of the current HEAD). The docstring and default list are consistent.
##########
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:
Fixed in be1a407188: `filter_state` is not in `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` (confirmed at line 291β314 of the current HEAD). The docstring and default list 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4632486634
I pushed a couple of follow-up commits to address the remaining review feedback so we can wrap this PR up.
What changed:
- `get_chart_info`: aligned the unsaved `form_data_key` path with the saved-chart path by applying `select_columns` during serialization, while still returning `ChartError` objects unchanged.
- `get_dataset_info`: made `column_fields=[]` behave as an explicit empty field selection instead of falling through to the full verbose column payload. It now returns the required `column_name` minimum.
- `get_dashboard_info`: kept `filter_state` out of the lean default projection and relies on the permalink-specific path to include it only when a permalink request uses the default selection.
I also added focused regression coverage for these paths and re-ran the relevant MCP tests plus Ruff on the touched files. The existing PR review threads are resolved from my side.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #40473:
URL: https://github.com/apache/superset/pull/40473
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4557298112
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328091129
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,29 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ result = parse_json_or_list(value, "select_columns")
+ return result or list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
Review Comment:
**π Architect Review β HIGH**
DEFAULT_GET_DASHBOARD_INFO_COLUMNS includes "filter_state" even though the select_columns field description says the lean default excludes "filter_state"; default get_dashboard_info responses will therefore still include the permalink filter_state payload when present, contradicting the contract and undermining the size-reduction intent.
**Suggestion:** Remove "filter_state" from DEFAULT_GET_DASHBOARD_INFO_COLUMNS (and only include it when explicitly requested or when permalink_key is present), or update the docs to match and add a regression test for permalink responses to guarantee they stay under the middleware limit.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=db7f9ccd292d4f489f15c3f0c744c228&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=db7f9ccd292d4f489f15c3f0c744c228&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings β not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/dashboard/schemas.py
**Line:** 291:364
**Comment:**
*HIGH: DEFAULT_GET_DASHBOARD_INFO_COLUMNS includes "filter_state" even though the select_columns field description says the lean default excludes "filter_state"; default get_dashboard_info responses will therefore still include the permalink filter_state payload when present, contradicting the contract and undermining the size-reduction intent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix β do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4596407124
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4625930675
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4631882289
Addressed all remaining open review comments:
**1. `filter_state` in `DEFAULT_GET_DASHBOARD_INFO_COLUMNS`** (Copilot #3358257445 and duplicates): Removed `filter_state` from the lean default. The docstring already said it was excluded; now the code matches. The `get_dashboard_info` logic that auto-adds `filter_state` when `permalink_key` is present was previously dead code β it is now live and functional.
**2. Empty-list disables filtering** (Copilot #3358257483, #3358257509, #3358257533 and duplicates): All three `_parse_select_columns` validators (dashboard, chart, dataset) and the dataset `_parse_column_fields` validator now fall back to the lean default when `parse_json_or_list` returns `[]`. Previously, `""` or `[]` input bypassed all filtering because `_filter_fields_by_context` only filters when `select_columns` is truthy.
**3. Unsaved-chart path in `get_chart_info`** (Copilot #3358257584): The `form_data_key`-only branch (no `identifier`) now calls `model_dump(mode="json", context={"select_columns": request.select_columns})` before returning, consistent with the saved-chart path. Large `form_data` payloads are now excluded by the lean default on this path too.
**4. Tests updated**: `test_column_fields_empty_list_stays_empty` β `test_column_fields_empty_list_falls_back_to_default`; privacy test assertion updated (`form_data` is now absent from the response rather than `None` since it's not in the default `select_columns`); added `test_unsaved_chart_select_columns_filters_response` covering the unsaved-chart select_columns path.
**Bito "dead code" comment** (#3358883587): The `and "filter_state" not in effective_select_columns` condition was dead code when `filter_state` was in the default, but is now live code after fix #1 β keeping it as is.
All 118 affected tests pass.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3358883587
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
Remove the redundant `and "filter_state" not in effective_select_columns` condition since the default columns already include 'filter_state'.
</div>
</div>
<small><i>Code Review Run #689af0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328421161
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,80 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing tests for new feature</b></div>
<div id="fix">
No test coverage exists for the new `select_columns` and `column_fields` parameters. The new `model_serializer` on `TableColumnInfo` and the field validators on `GetDatasetInfoRequest` lack unit tests to verify correct behavior under various input scenarios.
</div>
</div>
<small><i>Code Review Run #00da42</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328098579
##########
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:
**Suggestion:** The default dashboard projection still includes `filter_state`, which is the large permalink payload this change intended to avoid by default. When `permalink_key` is used, this can bloat responses back into truncation by `ResponseSizeGuardMiddleware`, so remove `filter_state` from the default list and require callers to opt in explicitly when they need it. [performance]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β get_dashboard_info with permalink returns oversized, truncation-prone payloads.
- β οΈ ResponseSizeGuardMiddleware truncates dashboard info for complex filters.
- β οΈ LLM agents receive partial dashboard filter context unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Inspect the default projection for dashboard info in
`superset/mcp_service/dashboard/schemas.py:22-46`. The
`DEFAULT_GET_DASHBOARD_INFO_COLUMNS` list includes `"filter_state"` as one of the default
top-level fields (line 45 in that file, line 314 in the PR hunk).
2. Inspect how `GetDashboardInfoRequest.select_columns` is populated in
`superset/mcp_service/dashboard/schemas.py:73-95`. The field uses `default_factory=lambda:
list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)` and the `@field_validator("select_columns",
mode="before")` returns `list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)` whenever the incoming
value is `None` or parses to an empty list, so any call that omits `select_columns`
receives a default that *includes* `"filter_state"`.
3. Follow the execution of the MCP tool in
`superset/mcp_service/dashboard/tool/get_dashboard_info.py:107-118` and `:163-175` plus
`:251-254`. FastMCP calls `get_dashboard_info(request: GetDashboardInfoRequest, ctx:
Context)`, which uses `ModelGetInfoCore` to produce a `DashboardInfo` instance and then
serializes it via `result.model_dump(mode="json", context={"select_columns":
request.select_columns})`. Because `DashboardInfo` implements `_filter_fields_by_context`
at `superset/mcp_service/dashboard/schemas.py:75-90` (lines 75-90 of that class), the
presence of `"filter_state"` in `request.select_columns` guarantees that `filter_state` is
included in the serialized payload whenever the field is non-null.
4. Examine how `filter_state` is populated and used when permalink state is present.
`_apply_permalink_state` in
`superset/mcp_service/dashboard/tool/get_dashboard_info.py:75-89` builds a payload from
`result.model_dump(mode="python")`, sets `payload["permalink_key"] = permalink_key`, and
sets `payload["filter_state"]` to a sanitized copy of the permalink state dict (which can
contain nested `dataMask`, `activeTabs`, etc.), then revalidates it into `DashboardInfo`.
The `DashboardInfo.filter_state` field in
`superset/mcp_service/dashboard/schemas.py:47-63` is a `Dict[str, Any]` and is expected to
contain the full permalink state. The unit test
`test_get_dashboard_info_permalink_does_not_double_sanitize` in
`tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py:214-250` and
`:440-50` confirms that, for a call `client.call_tool("get_dashboard_info", {"request":
{"identifier": 1, "permalink_key": "permalink-1"}})` with no `select_columns` provided,
the response includes a deeply nested `filter_state` structure with repeated string values
(e.g. labels, URLs, filters). Since `select_columns` defaults to
`DEFAULT_GET_DASHBOARD_INFO_COLUMNS`, this large `filter_state` is *always* present by
default whenever a permalink is used. That enlarged payload is then passed through
`ResponseSizeGuardMiddleware` (`superset/mcp_service/middleware.py:1158-1200` and
`:1228-95`), which estimates token size and truncates or blocks responses that exceed the
token limit for info tools (see tests
`test_middleware_response_truncation_for_get_dashboard_info` and related cases in
`tests/unit_tests/mcp_service/test_middleware.py:676-693` where `context.message.name =
"get_dashboard_info"` and a large payload triggers truncation). Thus, a real MCP call to
`get_dashboard_info` with only `identifier` and `permalink_key` and no explicit
`select_columns` will, for dashboards with large permalink state, include the full
`filter_state` by default, significantly increasing the response size and making
truncation by `ResponseSizeGuardMiddleware` more likely than if `filter_state` were
omitted from the default projection and only included when explicitly requested.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=44b7505f1d8f4322a6bc0cce4e0d4a1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=44b7505f1d8f4322a6bc0cce4e0d4a1f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/schemas.py
**Line:** 314:314
**Comment:**
*Performance: The default dashboard projection still includes `filter_state`, which is the large permalink payload this change intended to avoid by default. When `permalink_key` is used, this can bloat responses back into truncation by `ResponseSizeGuardMiddleware`, so remove `filter_state` from the default list and require callers to opt in explicitly when they need it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40473&comment_hash=d359f61f5d93488c12bf83c6464b358236d53200645d5484f05a51e9e34069d2&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40473&comment_hash=d359f61f5d93488c12bf83c6464b358236d53200645d5484f05a51e9e34069d2&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3328101600
##########
superset/mcp_service/dashboard/tool/get_dashboard_info.py:
##########
@@ -247,6 +248,10 @@ async def get_dashboard_info(
result.is_permalink_state,
)
)
+ return result.model_dump(
+ mode="json",
+ context={"select_columns": request.select_columns},
+ )
Review Comment:
**Suggestion:** This unconditional serialization with `request.select_columns` still propagates the request default set, and the current default includes `filter_state`; when `permalink_key` is used, that can be very large and can still trigger response truncation. Ensure `filter_state` is excluded from the effective default projection unless explicitly requested, or strip it before dumping when not explicitly selected. [performance]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β `get_dashboard_info` responses can exceed ResponseSizeGuard limits.
- β Dashboards with large permalink filter_state may return unusable payloads.
- β οΈ MCP clients relying on permalink context can see truncated data.
- β οΈ Size guard errors reduce reliability of dashboard inspection tools.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. An MCP client calls the `get_dashboard_info` FastMCP tool (entrypoint
`get_dashboard_info()` in `superset/mcp_service/dashboard/tool/get_dashboard_info.py:116`)
via the MCP core (`run_tool()` in `superset/mcp_service/mcp_core.py:808`), passing a
payload that includes `identifier` and a `permalink_key`, but no explicit `select_columns`
field.
2. The request is parsed into `GetDashboardInfoRequest`
(`superset/mcp_service/dashboard/schemas.py:74-111`). Because `select_columns` is omitted,
the `_parse_select_columns` validator at lines 112-120 receives `value=None`, returns
`list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)` at line 118, and that default list (defined at
lines 47-71) includes `"filter_state"` by default.
3. Inside `get_dashboard_info()`
(`superset/mcp_service/dashboard/tool/get_dashboard_info.py:152-224`), when
`request.permalink_key` is set, `_get_permalink_state()` and `_apply_permalink_state()`
are invoked (lines 183-223). `_apply_permalink_state()` (`_apply_permalink_state()` at
lines 75-89) injects a sanitized but potentially very large `filter_state` structure into
the `DashboardInfo` model.
4. On successful retrieval, `get_dashboard_info()` logs success and then serializes with
`result.model_dump(mode="json", context={"select_columns": request.select_columns})` at
lines 251-254. Because `request.select_columns` still contains `"filter_state"` from the
default, the full permalink `filter_state` is included in the response. For dashboards
with large permalink states this can push the response past the token/size limit enforced
by `ResponseSizeGuardMiddleware` (see
`tests/unit_tests/mcp_service/test_middleware.py:40-89`, where large responses are blocked
with `ToolError`), causing the dashboard-info response to be truncated or rejected for MCP
clients.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a788c7b2b5bd4e738f160742788dc624&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a788c7b2b5bd4e738f160742788dc624&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/get_dashboard_info.py
**Line:** 251:254
**Comment:**
*Performance: This unconditional serialization with `request.select_columns` still propagates the request default set, and the current default includes `filter_state`; when `permalink_key` is used, that can be very large and can still trigger response truncation. Ensure `filter_state` is excluded from the effective default projection unless explicitly requested, or strip it before dumping when not explicitly selected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40473&comment_hash=676be9420bd2429b449a1cbc86d0a8b5bc3741fb7d985aad051890ad018a258d&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40473&comment_hash=676be9420bd2429b449a1cbc86d0a8b5bc3741fb7d985aad051890ad018a258d&reaction=dislike'>π</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3329456112
##########
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:
Fixed in 3c917c7f99 (previous commit): the unsaved-chart path now calls `result.model_dump(mode='json', context={'select_columns': request.select_columns})` β same pattern as the saved-chart path. Also covered by the new `test_unsaved_chart_select_columns_filters_response` test.
##########
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:
Review Comment:
Fixed in 3c917c7f99 (previous commit): the unsaved-chart path now calls `result.model_dump(mode='json', context={'select_columns': request.select_columns})` β same pattern as the saved-chart path. Also covered by the new `test_unsaved_chart_select_columns_filters_response` test.
##########
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:
Review Comment:
Fixed in 3c917c7f99 (previous commit): the unsaved-chart path now calls `result.model_dump(mode='json', context={'select_columns': request.select_columns})` β same pattern as the saved-chart path. Also covered by the new `test_unsaved_chart_select_columns_filters_response` test.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -312,6 +339,29 @@ class GetDashboardInfoRequest(MetadataCacheControl):
"from that permalink."
),
)
+ select_columns: Annotated[
+ List[str],
+ Field(
+ default_factory=lambda: list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS),
+ description=(
+ "Top-level fields to include in the response. Defaults to a lean "
+ "set that excludes 'css' (raw CSS, can be many KB) and 'filter_state' "
+ "(only relevant when permalink_key is provided). Pass an explicit list "
+ "to override, e.g. ['id','dashboard_title','charts'] for minimal "
+ "output, or add 'css' to include raw dashboard CSS."
+ ),
+ ),
+ ]
+
+ @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_DASHBOARD_INFO_COLUMNS)
+ result = parse_json_or_list(value, "select_columns")
+ return result or list(DEFAULT_GET_DASHBOARD_INFO_COLUMNS)
Review Comment:
Fixed in commit be1a407188: `filter_state` was removed from `DEFAULT_GET_DASHBOARD_INFO_COLUMNS` in a previous commit (3c917c7f99). The current HEAD at line 291β314 confirms the default list does not include `filter_state`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40473:
URL: https://github.com/apache/superset/pull/40473#discussion_r3329457037
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -315,13 +334,80 @@ def create(cls, error: str, error_type: str) -> "DatasetError":
)
+DEFAULT_GET_DATASET_INFO_COLUMNS: List[str] = [
+ "id",
+ "table_name",
+ "schema",
+ "database_name",
+ "database_id",
+ "uuid",
+ "is_virtual",
+ "description",
+ "main_dttm_col",
+ "sql",
+ "url",
+ "columns",
+ "metrics",
+]
+
+DEFAULT_GET_DATASET_INFO_COLUMN_FIELDS: List[str] = [
+ "column_name",
+ "type",
+ "is_dttm",
+]
Review Comment:
<!-- Bito Reply -->
The suggestion in the comment is valid and worth applying. It highlights a missing test coverage for the new `select_columns` and `column_fields` parameters, as well as the lack of unit tests for the `model_serializer` and field validators. Adding these tests will ensure the correctness and robustness of the new features under various input scenarios. The suggestion aligns with best practices for testing and should be implemented to improve the code quality.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): add select_columns lean defaults to get_dashboard_info, get_chart_info, get_dataset_info [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40473:
URL: https://github.com/apache/superset/pull/40473#issuecomment-4557353035
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40473?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `46.29630%` with `29 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.36%. Comparing base ([`fb60662`](https://app.codecov.io/gh/apache/superset/commit/fb606623538bf63bc5cddfc88154c0b45290d3f5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ece03b3`](https://app.codecov.io/gh/apache/superset/commit/ece03b3fe94de892e9be2546a083c12358cffac3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40473?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/dataset/schemas.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC9zY2hlbWFzLnB5) | 42.85% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/schemas.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvc2NoZW1hcy5weQ==) | 55.55% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/dashboard/schemas.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdashboard%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGFzaGJvYXJkL3NjaGVtYXMucHk=) | 55.55% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...erset/mcp\_service/dataset/tool/get\_dataset\_info.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Ftool%2Fget_dataset_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC90b29sL2dldF9kYXRhc2V0X2luZm8ucHk=) | 25.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_info.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfaW5mby5weQ==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/mcp\_service/dashboard/tool/get\_dashboard\_info.py](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdashboard%2Ftool%2Fget_dashboard_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGFzaGJvYXJkL3Rvb2wvZ2V0X2Rhc2hib2FyZF9pbmZvLnB5) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40473?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (fb60662) and HEAD (ece03b3). Click for more details.
>
> <details><summary>HEAD has 44 uploads less than BASE</summary>
>
>| Flag | BASE (fb60662) | HEAD (ece03b3) |
>|------|------|------|
>|hive|4|1|
>|python|40|2|
>|presto|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40473 +/- ##
==========================================
- Coverage 64.18% 55.36% -8.83%
==========================================
Files 2592 2592
Lines 139257 139324 +67
Branches 32335 32345 +10
==========================================
- Hits 89385 77139 -12246
- Misses 48337 61519 +13182
+ Partials 1535 666 -869
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.22% <46.29%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <46.29%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.97% <46.29%> (-19.40%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40473/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40473?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org