codeant-ai-for-open-source[bot] commented on code in PR #40961:
URL: https://github.com/apache/superset/pull/40961#discussion_r3493324829
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1528,3 +1543,225 @@ def dashboard_layout_serializer(dashboard: "Dashboard")
-> DashboardLayout:
has_layout=bool(position_json_str),
)
)
+
+
+# Per-dataset caps keep responses small enough for LLM context: wide
+# datasets can have hundreds of columns, which would dwarf the fields an
+# agent actually needs to configure native filters.
+MAX_DASHBOARD_DATASET_COLUMNS = 100
+MAX_DASHBOARD_DATASET_METRICS = 50
+
+
+class DashboardDatasetColumn(BaseModel):
+ """Lean column representation for dashboard dataset context."""
+
+ column_name: str = Field(..., description="Column name")
+ verbose_name: str | None = Field(None, description="Verbose (display)
name")
+ type: str | None = Field(None, description="Column data type")
+ is_dttm: bool | None = Field(None, description="Is datetime column")
+
+
+class DashboardDatasetMetric(BaseModel):
+ """Lean metric representation for dashboard dataset context."""
+
+ metric_name: str = Field(..., description="Saved metric name")
+ verbose_name: str | None = Field(None, description="Verbose (display)
name")
+ expression: str | None = Field(None, description="SQL expression")
+
+
+class DashboardDatasetDatabaseInfo(BaseModel):
+ """Database connection summary for a dashboard dataset."""
+
+ id: int | None = Field(None, description="Database ID")
+ name: str | None = Field(None, description="Database name")
+ backend: str | None = Field(None, description="Database backend (engine)")
+
+
+class DashboardDatasetSummary(BaseModel):
+ """A dataset used by a dashboard's charts, with columns and metrics."""
+
+ id: int | None = Field(None, description="Dataset ID")
+ uuid: str | None = Field(None, description="Dataset UUID")
+ table_name: str | None = Field(None, description="Table name")
+ schema_name: str | None = Field(None, description="Schema name")
+ database: DashboardDatasetDatabaseInfo | None = Field(
+ None, description="Database the dataset belongs to"
+ )
+ chart_count: int = Field(
+ 0, description="Number of charts on the dashboard using this dataset"
+ )
+ columns: List[DashboardDatasetColumn] = Field(
+ default_factory=list, description="Dataset columns"
+ )
+ metrics: List[DashboardDatasetMetric] = Field(
+ default_factory=list, description="Dataset metrics"
+ )
+ total_column_count: int = Field(
+ 0, description="Total number of columns on the dataset"
+ )
+ total_metric_count: int = Field(
+ 0, description="Total number of metrics on the dataset"
+ )
+ columns_truncated: bool = Field(
+ False,
+ description=(
+ "True when the columns list was truncated to keep the response
small"
+ ),
+ )
+ metrics_truncated: bool = Field(
+ False,
+ description=(
+ "True when the metrics list was truncated to keep the response
small"
+ ),
+ )
+
+ @model_serializer(mode="wrap")
+ def _rename_schema_field(self, serializer: Any, info: Any) -> Dict[str,
Any]:
+ """Serialize 'schema_name' as 'schema' to match API conventions."""
+ data = serializer(self)
+ if "schema_name" in data:
+ data["schema"] = data.pop("schema_name")
+ return data
+
+
+class DashboardDatasets(BaseModel):
+ """Response schema for get_dashboard_datasets."""
+
+ id: int | None = Field(None, description="Dashboard ID")
+ dashboard_title: str | None = Field(None, description="Dashboard title")
+ uuid: str | None = Field(None, description="Dashboard UUID")
+ dataset_count: int = Field(
+ 0, description="Number of accessible datasets used by the dashboard"
+ )
+ inaccessible_dataset_count: int = Field(
+ 0,
+ description=(
+ "Number of datasets used by the dashboard that the current user "
+ "cannot access (excluded from 'datasets')"
+ ),
+ )
+ datasets: List[DashboardDatasetSummary] = Field(
+ default_factory=list,
+ description="Datasets used by the dashboard's charts",
+ )
+
+
+def _serialize_dashboard_dataset(
+ datasource: Any, chart_count: int
+) -> DashboardDatasetSummary:
+ """Serialize a datasource to a lean, LLM-safe dataset summary."""
+ all_columns = list(getattr(datasource, "columns", None) or [])
+ all_metrics = list(getattr(datasource, "metrics", None) or [])
+
+ columns = [
+ DashboardDatasetColumn(
+ column_name=escape_llm_context_delimiters(
+ getattr(column, "column_name", None) or ""
+ ),
+ verbose_name=sanitize_for_llm_context(
+ getattr(column, "verbose_name", None),
+ field_path=("columns", str(index), "verbose_name"),
+ ),
+ type=getattr(column, "type", None),
+ is_dttm=getattr(column, "is_dttm", None),
+ )
+ for index, column in
enumerate(all_columns[:MAX_DASHBOARD_DATASET_COLUMNS])
+ ]
+ metrics = [
+ DashboardDatasetMetric(
+ metric_name=escape_llm_context_delimiters(
+ getattr(metric, "metric_name", None) or ""
+ ),
+ verbose_name=sanitize_for_llm_context(
+ getattr(metric, "verbose_name", None),
+ field_path=("metrics", str(index), "verbose_name"),
+ ),
+ expression=sanitize_for_llm_context(
+ getattr(metric, "expression", None),
+ field_path=("metrics", str(index), "expression"),
+ ),
+ )
+ for index, metric in
enumerate(all_metrics[:MAX_DASHBOARD_DATASET_METRICS])
+ ]
+
+ database = getattr(datasource, "database", None)
+ database_info = (
+ DashboardDatasetDatabaseInfo(
+ id=getattr(database, "id", None),
+ name=escape_llm_context_delimiters(
+ getattr(database, "database_name", None)
+ ),
+ backend=getattr(database, "backend", None),
+ )
+ if database is not None
+ else None
+ )
+
+ dataset_uuid = getattr(datasource, "uuid", None)
+ return DashboardDatasetSummary(
+ id=getattr(datasource, "id", None),
+ uuid=str(dataset_uuid) if dataset_uuid else None,
+ table_name=escape_llm_context_delimiters(
+ getattr(datasource, "table_name", None)
+ ),
+ schema_name=escape_llm_context_delimiters(getattr(datasource,
"schema", None)),
+ database=database_info,
+ chart_count=chart_count,
+ columns=columns,
+ metrics=metrics,
+ total_column_count=len(all_columns),
+ total_metric_count=len(all_metrics),
+ columns_truncated=len(all_columns) > MAX_DASHBOARD_DATASET_COLUMNS,
+ metrics_truncated=len(all_metrics) > MAX_DASHBOARD_DATASET_METRICS,
+ )
+
+
+def dashboard_datasets_serializer(dashboard: "Dashboard") -> DashboardDatasets:
+ """Serialize a Dashboard model to the datasets used by its charts.
+
+ Groups the dashboard's charts by datasource (mirroring
+ ``Dashboard.datasets_trimmed_for_slices``) but keeps the full column and
+ metric lists (capped) since native-filter configuration regularly needs
+ columns that no chart references. Datasets the current user cannot
+ access are excluded and only counted.
+ """
+ from superset.mcp_service.auth import has_dataset_access
+
+ slices_by_datasource: Dict[int, List[Any]] = {}
+ for slc in getattr(dashboard, "slices", None) or []:
+ datasource_id = getattr(slc, "datasource_id", None)
+ if datasource_id is None:
+ continue
+ slices_by_datasource.setdefault(datasource_id, []).append(slc)
+
+ datasets: List[DashboardDatasetSummary] = []
Review Comment:
**Suggestion:** The grouping key uses only `datasource_id`, but chart
datasources in Superset are identified by both ID and type. If a dashboard
contains slices from different datasource types that share the same numeric ID,
they will be merged into one dataset entry, producing incorrect `chart_count`,
wrong dataset metadata, and incorrect access filtering. Group by
`(datasource_id, datasource_type)` instead of ID alone. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ MCP get_dashboard_datasets can merge distinct datasets incorrectly.
⚠️ Chart_count and dataset metadata may not match dashboard.
⚠️ Access-control reporting may miscount inaccessible dashboard datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a dashboard in the Superset DB that includes slices
from at least
two different datasource types which can share numeric IDs (e.g., an SQL
dataset and
another connector both having `id=5`). Slice model fields `datasource_id` and
`datasource_type` that allow this are defined at
`superset/models/slice.py:79-80`.
2. Call the MCP FastMCP tool `get_dashboard_datasets` for that dashboard
identifier (ID,
UUID, or slug). This entry point is implemented at
`superset/mcp_service/dashboard/tool/get_dashboard_datasets.py:50-88`, which
loads the
`Dashboard` and passes it to `dashboard_datasets_serializer`.
3. In `dashboard_datasets_serializer` at
`superset/mcp_service/dashboard/schemas.py:220-237`, the code builds
`slices_by_datasource: Dict[int, List[Any]] = {}` and groups slices using
only
`datasource_id` (lines corresponding to diff 1732-1737), ignoring
`datasource_type`, even
though datasource uniqueness across types is modeled via `BaseDatasource.uid
=
f"{self.id}__{self.type}"` in `superset/connectors/sqla/models.py:97-99` and
the
datasource->slices relationship joins on both `Slice.datasource_id` and
`Slice.datasource_type` at `superset/connectors/sqla/models.py:83-86`.
4. When `dashboard_datasets_serializer` later iterates these merged groups,
it picks a
single `datasource` from the mixed-type slices (schemas.py:241-247) and runs
access
filtering and dataset serialization on that one object. As a result, charts
from different
datasource types that happen to share the same numeric `datasource_id` are
incorrectly
merged: `chart_count` aggregates slices from multiple distinct datasets,
dataset metadata
reflects only one datasource, and `has_dataset_access` (schemas.py:251) is
evaluated only
once for the group, potentially excluding accessible datasets or including
slices from
inaccessible ones under the wrong dataset entry in the MCP response.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4449883719a34d449cc912cb8fe5f5d8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4449883719a34d449cc912cb8fe5f5d8&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:** 1732:1737
**Comment:**
*Logic Error: The grouping key uses only `datasource_id`, but chart
datasources in Superset are identified by both ID and type. If a dashboard
contains slices from different datasource types that share the same numeric ID,
they will be merged into one dataset entry, producing incorrect `chart_count`,
wrong dataset metadata, and incorrect access filtering. Group by
`(datasource_id, datasource_type)` instead of ID alone.
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%2F40961&comment_hash=f66da2a9e787c8bd0268137c7ba240bf437e8ceced8f6e1a1443fc132ebb3273&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40961&comment_hash=f66da2a9e787c8bd0268137c7ba240bf437e8ceced8f6e1a1443fc132ebb3273&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
