mirror of
https://github.com/apache/superset.git
synced 2026-07-18 20:55:47 +00:00
fix(mcp): fix crashes in list tools, dataset info, chart preview, and add owner/favorite filters (#38277)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -34,8 +34,8 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
# Custom filterable fields for charts
|
||||
CHART_CUSTOM_FIELDS = {
|
||||
"viz_type": ["eq", "in_", "like"],
|
||||
"datasource_name": ["eq", "in_", "like"],
|
||||
"viz_type": ["eq", "in", "like"],
|
||||
"datasource_name": ["eq", "in", "like"],
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -23,6 +23,8 @@ from typing import Any, Dict, List
|
||||
|
||||
from flask import g
|
||||
from flask_appbuilder.models.sqla.interface import SQLAInterface
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.orm import Query
|
||||
|
||||
from superset import is_feature_enabled, security_manager
|
||||
from superset.commands.dashboard.exceptions import (
|
||||
@@ -31,7 +33,7 @@ from superset.commands.dashboard.exceptions import (
|
||||
DashboardNotFoundError,
|
||||
DashboardUpdateFailedError,
|
||||
)
|
||||
from superset.daos.base import BaseDAO
|
||||
from superset.daos.base import BaseDAO, ColumnOperator, ColumnOperatorEnum
|
||||
from superset.dashboards.filters import DashboardAccessFilter, is_uuid
|
||||
from superset.exceptions import SupersetSecurityException
|
||||
from superset.extensions import db
|
||||
@@ -47,15 +49,66 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
# Custom filterable fields for dashboards
|
||||
DASHBOARD_CUSTOM_FIELDS = {
|
||||
"tags": ["eq", "in_", "like"],
|
||||
"owners": ["eq", "in_"],
|
||||
"tags": ["eq", "in", "like"],
|
||||
"owners": ["eq", "in"],
|
||||
"published": ["eq"],
|
||||
"owner": ["eq", "in"],
|
||||
"favorite": ["eq"],
|
||||
}
|
||||
|
||||
|
||||
class DashboardDAO(BaseDAO[Dashboard]):
|
||||
base_filter = DashboardAccessFilter
|
||||
|
||||
@classmethod
|
||||
def apply_column_operators(
|
||||
cls,
|
||||
query: Query,
|
||||
column_operators: list[ColumnOperator] | None = None,
|
||||
) -> Query:
|
||||
"""Override to handle owner and favorite filters via subqueries.
|
||||
|
||||
- owner: filters dashboards by owner user ID via dashboard_user M2M table
|
||||
- favorite: filters dashboards by whether the current user has favorited them
|
||||
"""
|
||||
if not column_operators:
|
||||
return query
|
||||
|
||||
remaining_operators: list[ColumnOperator] = []
|
||||
for c in column_operators:
|
||||
if not isinstance(c, ColumnOperator):
|
||||
c = ColumnOperator.model_validate(c)
|
||||
if c.col == "owner":
|
||||
from superset.models.dashboard import dashboard_user
|
||||
|
||||
operator_enum = ColumnOperatorEnum(c.opr)
|
||||
subq = select(dashboard_user.c.dashboard_id).where(
|
||||
operator_enum.apply(dashboard_user.c.user_id, c.value)
|
||||
)
|
||||
query = query.filter(
|
||||
Dashboard.id.in_(subq) # type: ignore[attr-defined,unused-ignore]
|
||||
)
|
||||
elif c.col == "favorite":
|
||||
user_id = get_user_id()
|
||||
fav_subq = select(FavStar.obj_id).where(
|
||||
FavStar.class_name == FavStarClassName.DASHBOARD,
|
||||
FavStar.user_id == user_id,
|
||||
)
|
||||
if c.value is True or c.value == 1:
|
||||
query = query.filter(
|
||||
Dashboard.id.in_(fav_subq) # type: ignore[attr-defined,unused-ignore]
|
||||
)
|
||||
else:
|
||||
query = query.filter(
|
||||
~Dashboard.id.in_(fav_subq) # type: ignore[attr-defined,unused-ignore]
|
||||
)
|
||||
else:
|
||||
remaining_operators.append(c)
|
||||
|
||||
if remaining_operators:
|
||||
query = super().apply_column_operators(query, remaining_operators)
|
||||
return query
|
||||
|
||||
@classmethod
|
||||
def get_filterable_columns_and_operators(cls) -> Dict[str, List[str]]:
|
||||
filterable = super().get_filterable_columns_and_operators()
|
||||
|
||||
@@ -40,6 +40,7 @@ logger = logging.getLogger(__name__)
|
||||
# Custom filterable fields for datasets (not direct model columns)
|
||||
DATASET_CUSTOM_FIELDS: dict[str, list[str]] = {
|
||||
"database_name": ["eq", "like", "ilike"],
|
||||
"owner": ["eq", "in"],
|
||||
}
|
||||
|
||||
|
||||
@@ -77,6 +78,16 @@ class DatasetDAO(BaseDAO[SqlaTable]):
|
||||
operator_enum.apply(Database.database_name, c.value)
|
||||
)
|
||||
query = query.filter(SqlaTable.database_id.in_(subq))
|
||||
elif c.col == "owner":
|
||||
from superset.connectors.sqla.models import sqlatable_user
|
||||
|
||||
operator_enum = ColumnOperatorEnum(c.opr)
|
||||
subq = select(sqlatable_user.c.table_id).where(
|
||||
operator_enum.apply(sqlatable_user.c.user_id, c.value)
|
||||
)
|
||||
query = query.filter(
|
||||
SqlaTable.id.in_(subq) # type: ignore[attr-defined,unused-ignore]
|
||||
)
|
||||
else:
|
||||
remaining_operators.append(c)
|
||||
|
||||
|
||||
@@ -489,6 +489,8 @@ def configure_temporal_handling(
|
||||
For temporal columns, enables standard time series handling.
|
||||
For non-temporal columns (e.g., BIGINT year), disables DATE_TRUNC
|
||||
by setting categorical sorting options.
|
||||
|
||||
Stores any warnings in ``form_data["_mcp_warnings"]``.
|
||||
"""
|
||||
if x_is_temporal:
|
||||
if time_grain:
|
||||
@@ -499,6 +501,12 @@ def configure_temporal_handling(
|
||||
form_data["x_axis_sort_series_ascending"] = True
|
||||
form_data["time_grain_sqla"] = None
|
||||
form_data["granularity_sqla"] = None
|
||||
if time_grain:
|
||||
form_data.setdefault("_mcp_warnings", []).append(
|
||||
f"time_grain='{time_grain}' was ignored because the x-axis "
|
||||
f"column is not a temporal type. time_grain only applies to "
|
||||
f"DATE/DATETIME/TIMESTAMP columns."
|
||||
)
|
||||
|
||||
|
||||
def map_xy_config(
|
||||
|
||||
@@ -23,6 +23,7 @@ from form data without requiring a saved chart object.
|
||||
"""
|
||||
|
||||
import logging
|
||||
import math
|
||||
from typing import Any, Dict, List
|
||||
|
||||
from superset.commands.chart.data.get_data_command import ChartDataCommand
|
||||
@@ -183,19 +184,21 @@ def _calculate_column_widths(
|
||||
def _format_value(val: Any, width: int) -> str:
|
||||
"""Format a value based on its type."""
|
||||
if isinstance(val, float):
|
||||
if abs(val) >= 1000000:
|
||||
if math.isnan(val):
|
||||
val_str = "N/A"
|
||||
elif math.isfinite(val) and val.is_integer():
|
||||
# Integer-like float (e.g. 1988.0) — format without decimals
|
||||
val_str = str(int(val))
|
||||
elif abs(val) >= 1000000:
|
||||
val_str = f"{val:.2e}" # Scientific notation for large numbers
|
||||
elif abs(val) >= 1000:
|
||||
val_str = f"{val:,.2f}" # Thousands separator
|
||||
else:
|
||||
val_str = f"{val:.2f}"
|
||||
val_str = f"{val:g}"
|
||||
elif isinstance(val, int):
|
||||
if abs(val) >= 1000:
|
||||
val_str = f"{val:,}" # Thousands separator
|
||||
else:
|
||||
val_str = str(val)
|
||||
val_str = str(val)
|
||||
elif val is None:
|
||||
val_str = "NULL"
|
||||
val_str = "N/A"
|
||||
else:
|
||||
val_str = str(val)
|
||||
|
||||
|
||||
@@ -1200,7 +1200,8 @@ class GetChartPreviewRequest(QueryCacheControl):
|
||||
format: Literal["url", "ascii", "table", "vega_lite"] = Field(
|
||||
default="url",
|
||||
description=(
|
||||
"Preview format: 'url' for image URL, 'ascii' for text art, "
|
||||
"Preview format: 'url' for explore link (default), "
|
||||
"'ascii' for text art, "
|
||||
"'table' for data table, "
|
||||
"'vega_lite' for interactive JSON specification"
|
||||
),
|
||||
|
||||
@@ -39,6 +39,7 @@ from superset.mcp_service.chart.chart_utils import (
|
||||
)
|
||||
from superset.mcp_service.chart.schemas import (
|
||||
AccessibilityMetadata,
|
||||
ChartError,
|
||||
GenerateChartRequest,
|
||||
GenerateChartResponse,
|
||||
PerformanceMetadata,
|
||||
@@ -257,7 +258,7 @@ async def generate_chart( # noqa: C901
|
||||
chart_id = None
|
||||
explore_url = None
|
||||
form_data_key = None
|
||||
response_warnings: list[str] = []
|
||||
response_warnings: list[str] = form_data.pop("_mcp_warnings", [])
|
||||
|
||||
# Save chart by default (unless save_chart=False)
|
||||
if request.save_chart:
|
||||
@@ -636,7 +637,12 @@ async def generate_chart( # noqa: C901
|
||||
preview_request, ctx
|
||||
)
|
||||
|
||||
if hasattr(preview_result, "content"):
|
||||
if isinstance(preview_result, ChartError):
|
||||
await ctx.warning(
|
||||
"Preview '%s' failed: %s"
|
||||
% (format_type, preview_result.error)
|
||||
)
|
||||
elif hasattr(preview_result, "content"):
|
||||
previews[format_type] = preview_result.content
|
||||
else:
|
||||
# For preview-only mode (save_chart=false)
|
||||
@@ -674,7 +680,12 @@ async def generate_chart( # noqa: C901
|
||||
preview_format=format_type,
|
||||
)
|
||||
|
||||
if not hasattr(preview_result, "error"):
|
||||
if isinstance(preview_result, ChartError):
|
||||
await ctx.warning(
|
||||
"Preview '%s' failed: %s"
|
||||
% (format_type, preview_result.error)
|
||||
)
|
||||
else:
|
||||
previews[format_type] = preview_result
|
||||
|
||||
except (CommandException, ValueError, KeyError) as e:
|
||||
|
||||
@@ -959,7 +959,7 @@ def _export_data_as_csv(
|
||||
chart_id=chart.id,
|
||||
chart_name=chart.slice_name or f"Chart {chart.id}",
|
||||
chart_type=chart.viz_type or "unknown",
|
||||
columns=[], # Not needed for CSV export
|
||||
columns=[], # Column names are embedded in CSV content
|
||||
data=[], # CSV content is in csv_data field
|
||||
row_count=len(data),
|
||||
total_rows=len(data),
|
||||
@@ -1114,7 +1114,7 @@ def _create_excel_chart_data(
|
||||
chart_id=chart.id,
|
||||
chart_name=chart_name,
|
||||
chart_type=chart.viz_type or "unknown",
|
||||
columns=[],
|
||||
columns=[], # Column names are embedded in the Excel file
|
||||
data=[],
|
||||
row_count=len(data),
|
||||
total_rows=len(data),
|
||||
@@ -1147,7 +1147,7 @@ def _create_excel_chart_data_xlsxwriter(
|
||||
chart_id=chart.id,
|
||||
chart_name=chart_name,
|
||||
chart_type=chart.viz_type or "unknown",
|
||||
columns=[],
|
||||
columns=[], # Column names are embedded in the Excel file
|
||||
data=[],
|
||||
row_count=len(data),
|
||||
total_rows=len(data),
|
||||
|
||||
@@ -89,19 +89,20 @@ class PreviewFormatStrategy:
|
||||
|
||||
|
||||
class URLPreviewStrategy(PreviewFormatStrategy):
|
||||
"""Generate URL-based image preview."""
|
||||
"""Generate URL-based preview with explore link."""
|
||||
|
||||
def generate(self) -> URLPreview | ChartError:
|
||||
# Screenshot-based URL previews are not supported.
|
||||
# Users should use the explore_url to view the chart interactively,
|
||||
# or use other preview formats like 'ascii', 'table', or 'vega_lite'.
|
||||
return ChartError(
|
||||
error=(
|
||||
"URL-based screenshot previews are not supported. "
|
||||
"Use the explore_url to view the chart interactively, "
|
||||
"or try formats: 'ascii', 'table', or 'vega_lite'."
|
||||
),
|
||||
error_type="UnsupportedFormat",
|
||||
chart = self.chart
|
||||
if not chart.id:
|
||||
return ChartError(
|
||||
error="URL preview not available for transient charts without an ID",
|
||||
error_type="UnsupportedFormat",
|
||||
)
|
||||
explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart.id}"
|
||||
return URLPreview(
|
||||
preview_url=explore_url,
|
||||
width=self.request.width or 800,
|
||||
height=self.request.height or 600,
|
||||
)
|
||||
|
||||
|
||||
@@ -2143,7 +2144,12 @@ async def _get_chart_preview_internal( # noqa: C901
|
||||
elif isinstance(content, TablePreview):
|
||||
result.format = "table"
|
||||
result.table_data = content.table_data
|
||||
# Base64 preview support removed
|
||||
elif isinstance(content, VegaLitePreview):
|
||||
result.format = "vega_lite"
|
||||
elif isinstance(content, URLPreview):
|
||||
result.format = "url"
|
||||
result.width = content.width
|
||||
result.height = content.height
|
||||
|
||||
return result
|
||||
|
||||
|
||||
@@ -130,6 +130,7 @@ async def update_chart(
|
||||
# Get dataset_id from existing chart for column type checking
|
||||
dataset_id = chart.datasource_id if chart.datasource_id else None
|
||||
new_form_data = map_config_to_form_data(request.config, dataset_id=dataset_id)
|
||||
new_form_data.pop("_mcp_warnings", None)
|
||||
|
||||
# Update chart using Superset's command
|
||||
from superset.commands.chart.update import UpdateChartCommand
|
||||
|
||||
@@ -72,6 +72,7 @@ def update_chart_preview(
|
||||
new_form_data = map_config_to_form_data(
|
||||
request.config, dataset_id=request.dataset_id
|
||||
)
|
||||
new_form_data.pop("_mcp_warnings", None)
|
||||
|
||||
# Generate new explore link with updated form_data
|
||||
explore_url = generate_explore_link(request.dataset_id, new_form_data)
|
||||
|
||||
@@ -152,8 +152,9 @@ class DashboardFilter(ColumnOperator):
|
||||
col: Literal[
|
||||
"dashboard_title",
|
||||
"published",
|
||||
"favorite",
|
||||
"created_by_fk",
|
||||
"owner",
|
||||
"favorite",
|
||||
] = Field(
|
||||
...,
|
||||
description=(
|
||||
@@ -465,6 +466,12 @@ class GenerateDashboardResponse(BaseModel):
|
||||
|
||||
|
||||
def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo:
|
||||
from superset.mcp_service.utils.url_utils import get_superset_base_url
|
||||
|
||||
base_url = get_superset_base_url()
|
||||
relative_url = dashboard.url # e.g. "/superset/dashboard/{slug_or_id}/"
|
||||
absolute_url = f"{base_url}{relative_url}" if relative_url else None
|
||||
|
||||
return DashboardInfo(
|
||||
id=dashboard.id,
|
||||
dashboard_title=dashboard.dashboard_title or "Untitled",
|
||||
@@ -487,7 +494,7 @@ def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo:
|
||||
if dashboard.changed_by
|
||||
else None,
|
||||
uuid=str(dashboard.uuid) if dashboard.uuid else None,
|
||||
url=dashboard.url,
|
||||
url=absolute_url,
|
||||
created_on_humanized=dashboard.created_on_humanized,
|
||||
changed_on_humanized=dashboard.changed_on_humanized,
|
||||
chart_count=len(dashboard.slices) if dashboard.slices else 0,
|
||||
@@ -517,16 +524,28 @@ def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo:
|
||||
|
||||
def serialize_dashboard_object(dashboard: Any) -> DashboardInfo:
|
||||
"""Simple dashboard serializer that safely handles object attributes."""
|
||||
from superset.mcp_service.utils.url_utils import get_superset_base_url
|
||||
|
||||
# Construct URL from id/slug (the model's @property isn't available on
|
||||
# column-only query tuples returned by DAO.list with select_columns)
|
||||
dashboard_id = getattr(dashboard, "id", None)
|
||||
slug = getattr(dashboard, "slug", None)
|
||||
dashboard_url = None
|
||||
if dashboard_id is not None:
|
||||
dashboard_url = (
|
||||
f"{get_superset_base_url()}/superset/dashboard/{slug or dashboard_id}/"
|
||||
)
|
||||
|
||||
return DashboardInfo(
|
||||
id=getattr(dashboard, "id", None),
|
||||
id=dashboard_id,
|
||||
dashboard_title=getattr(dashboard, "dashboard_title", None),
|
||||
slug=getattr(dashboard, "slug", None),
|
||||
url=getattr(dashboard, "url", None),
|
||||
slug=slug,
|
||||
url=dashboard_url,
|
||||
published=getattr(dashboard, "published", None),
|
||||
changed_by_name=getattr(dashboard, "changed_by_name", None),
|
||||
changed_by=getattr(dashboard, "changed_by_name", None),
|
||||
changed_on=getattr(dashboard, "changed_on", None),
|
||||
changed_on_humanized=getattr(dashboard, "changed_on_humanized", None),
|
||||
created_by_name=getattr(dashboard, "created_by_name", None),
|
||||
created_by=getattr(dashboard, "created_by_name", None),
|
||||
created_on=getattr(dashboard, "created_on", None),
|
||||
created_on_humanized=getattr(dashboard, "created_on_humanized", None),
|
||||
description=getattr(dashboard, "description", None),
|
||||
@@ -541,9 +560,24 @@ def serialize_dashboard_object(dashboard: Any) -> DashboardInfo:
|
||||
if getattr(dashboard, "uuid", None)
|
||||
else None,
|
||||
chart_count=len(getattr(dashboard, "slices", [])),
|
||||
owners=getattr(dashboard, "owners", []),
|
||||
tags=getattr(dashboard, "tags", []),
|
||||
roles=getattr(dashboard, "roles", []),
|
||||
owners=[
|
||||
UserInfo.model_validate(owner, from_attributes=True)
|
||||
for owner in getattr(dashboard, "owners", [])
|
||||
]
|
||||
if getattr(dashboard, "owners", None)
|
||||
else [],
|
||||
tags=[
|
||||
TagInfo.model_validate(tag, from_attributes=True)
|
||||
for tag in getattr(dashboard, "tags", [])
|
||||
]
|
||||
if getattr(dashboard, "tags", None)
|
||||
else [],
|
||||
roles=[
|
||||
RoleInfo.model_validate(role, from_attributes=True)
|
||||
for role in getattr(dashboard, "roles", [])
|
||||
]
|
||||
if getattr(dashboard, "roles", None)
|
||||
else [],
|
||||
charts=[
|
||||
serialize_chart_object(chart) for chart in getattr(dashboard, "slices", [])
|
||||
]
|
||||
|
||||
@@ -29,6 +29,7 @@ from fastmcp import Context
|
||||
from superset_core.mcp.decorators import tool
|
||||
|
||||
from superset.extensions import event_logger
|
||||
from superset.mcp_service.chart.schemas import serialize_chart_object
|
||||
from superset.mcp_service.dashboard.constants import (
|
||||
generate_id,
|
||||
GRID_COLUMN_COUNT,
|
||||
@@ -431,7 +432,10 @@ def add_chart_to_existing_dashboard(
|
||||
if serialize_tag_object(tag) is not None
|
||||
],
|
||||
roles=[],
|
||||
charts=[],
|
||||
charts=[
|
||||
serialize_chart_object(chart)
|
||||
for chart in getattr(updated_dashboard, "slices", [])
|
||||
],
|
||||
)
|
||||
|
||||
dashboard_url = (
|
||||
|
||||
@@ -28,6 +28,7 @@ from fastmcp import Context
|
||||
from superset_core.mcp.decorators import tool
|
||||
|
||||
from superset.extensions import event_logger
|
||||
from superset.mcp_service.chart.schemas import serialize_chart_object
|
||||
from superset.mcp_service.dashboard.constants import (
|
||||
generate_id,
|
||||
GRID_COLUMN_COUNT,
|
||||
@@ -295,7 +296,11 @@ def generate_dashboard(
|
||||
if serialize_tag_object(tag) is not None
|
||||
],
|
||||
roles=[], # Dashboard roles not typically set at creation
|
||||
charts=[], # Chart details not needed in response
|
||||
charts=[
|
||||
obj
|
||||
for chart in getattr(dashboard, "slices", [])
|
||||
if (obj := serialize_chart_object(chart)) is not None
|
||||
],
|
||||
)
|
||||
|
||||
dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
|
||||
|
||||
@@ -57,7 +57,6 @@ class DatasetFilter(ColumnOperator):
|
||||
"schema",
|
||||
"database_name",
|
||||
"owner",
|
||||
"favorite",
|
||||
] = Field(
|
||||
...,
|
||||
description="Column to filter on. Use get_schema(model_type='dataset') for "
|
||||
@@ -154,14 +153,16 @@ class DatasetInfo(BaseModel):
|
||||
# Get full serialization
|
||||
data = serializer(self)
|
||||
|
||||
# Normalize alias: Pydantic serializes as 'schema_name' (field name)
|
||||
# but the DAO column and API convention is 'schema'
|
||||
if "schema_name" in data:
|
||||
data["schema"] = data.pop("schema_name")
|
||||
|
||||
# Check if we have a context with select_columns
|
||||
if info.context and isinstance(info.context, dict):
|
||||
select_columns = info.context.get("select_columns")
|
||||
if select_columns:
|
||||
# Handle alias: 'schema' -> 'schema_name'
|
||||
requested_fields = set(select_columns)
|
||||
if "schema" in requested_fields:
|
||||
requested_fields.add("schema_name")
|
||||
|
||||
# Filter to only requested fields
|
||||
return {k: v for k, v in data.items() if k in requested_fields}
|
||||
@@ -285,6 +286,20 @@ class GetDatasetInfoRequest(MetadataCacheControl):
|
||||
]
|
||||
|
||||
|
||||
def _parse_json_field(obj: Any, field_name: str) -> Dict[str, Any] | None:
|
||||
"""Parse a field that may be stored as a JSON string into a dict."""
|
||||
value = getattr(obj, field_name, None)
|
||||
if isinstance(value, str):
|
||||
try:
|
||||
parsed = json.loads(value)
|
||||
if isinstance(parsed, dict):
|
||||
return parsed
|
||||
except (ValueError, TypeError):
|
||||
pass
|
||||
return None
|
||||
return value
|
||||
|
||||
|
||||
def serialize_dataset_object(dataset: Any) -> DatasetInfo | None:
|
||||
if not dataset:
|
||||
return None
|
||||
@@ -357,8 +372,8 @@ def serialize_dataset_object(dataset: Any) -> DatasetInfo | None:
|
||||
offset=getattr(dataset, "offset", None),
|
||||
cache_timeout=getattr(dataset, "cache_timeout", None),
|
||||
params=params,
|
||||
template_params=getattr(dataset, "template_params", None),
|
||||
extra=getattr(dataset, "extra", None),
|
||||
template_params=_parse_json_field(dataset, "template_params"),
|
||||
extra=_parse_json_field(dataset, "extra"),
|
||||
columns=columns,
|
||||
metrics=metrics,
|
||||
is_favorite=getattr(dataset, "is_favorite", None),
|
||||
|
||||
@@ -181,8 +181,11 @@ class ModelListCore(BaseCore, Generic[L]):
|
||||
total_pages = (total_count + page_size - 1) // page_size if page_size > 0 else 0
|
||||
from superset.mcp_service.system.schemas import PaginationInfo
|
||||
|
||||
# Report 1-based page in response to match the 1-based input convention
|
||||
# used by all list tool wrappers (list_charts, list_datasets, etc.)
|
||||
page_1based = page + 1
|
||||
pagination_info = PaginationInfo(
|
||||
page=page,
|
||||
page=page_1based,
|
||||
page_size=page_size,
|
||||
total_count=total_count,
|
||||
total_pages=total_pages,
|
||||
@@ -202,7 +205,7 @@ class ModelListCore(BaseCore, Generic[L]):
|
||||
self.list_field_name: item_objs,
|
||||
"count": len(item_objs),
|
||||
"total_count": total_count,
|
||||
"page": page,
|
||||
"page": page_1based,
|
||||
"page_size": page_size,
|
||||
"total_pages": total_pages,
|
||||
"has_previous": page > 0,
|
||||
|
||||
@@ -19,12 +19,14 @@
|
||||
|
||||
from typing import Any
|
||||
|
||||
from pydantic import AliasChoices, BaseModel, Field, field_validator
|
||||
from pydantic import AliasChoices, BaseModel, ConfigDict, Field, field_validator
|
||||
|
||||
|
||||
class ExecuteSqlRequest(BaseModel):
|
||||
"""Request schema for executing SQL queries."""
|
||||
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
database_id: int = Field(
|
||||
..., description="Database connection ID to execute query against"
|
||||
)
|
||||
@@ -36,9 +38,13 @@ class ExecuteSqlRequest(BaseModel):
|
||||
None, description="Schema to use for query execution", alias="schema"
|
||||
)
|
||||
catalog: str | None = Field(None, description="Catalog name for query execution")
|
||||
limit: int = Field(
|
||||
default=1000,
|
||||
description="Maximum number of rows to return",
|
||||
limit: int | None = Field(
|
||||
default=None,
|
||||
description=(
|
||||
"Maximum number of rows to return. "
|
||||
"If not specified, respects the LIMIT in your SQL query. "
|
||||
"If specified, overrides any SQL LIMIT clause."
|
||||
),
|
||||
ge=1,
|
||||
le=10000,
|
||||
)
|
||||
@@ -118,6 +124,8 @@ class ExecuteSqlResponse(BaseModel):
|
||||
class OpenSqlLabRequest(BaseModel):
|
||||
"""Request schema for opening SQL Lab with context."""
|
||||
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
database_connection_id: int = Field(
|
||||
...,
|
||||
description="Database connection ID to use in SQL Lab",
|
||||
@@ -140,6 +148,8 @@ class OpenSqlLabRequest(BaseModel):
|
||||
class SqlLabResponse(BaseModel):
|
||||
"""Response schema for SQL Lab URL generation."""
|
||||
|
||||
model_config = ConfigDict(populate_by_name=True)
|
||||
|
||||
url: str = Field(..., description="URL to open SQL Lab with context")
|
||||
database_id: int = Field(..., description="Database ID used")
|
||||
schema_name: str | None = Field(None, description="Schema selected", alias="schema")
|
||||
|
||||
@@ -94,7 +94,7 @@ def open_sql_lab_with_context(
|
||||
context_comment += f"\nSELECT * FROM {table_reference} LIMIT 100;"
|
||||
params["sql"] = context_comment
|
||||
|
||||
# Construct SQL Lab URL
|
||||
# Construct SQL Lab URL with full base URL
|
||||
query_string = urlencode(params)
|
||||
url = f"{get_superset_base_url()}/sqllab?{query_string}"
|
||||
|
||||
|
||||
@@ -71,8 +71,8 @@ def calculate_dashboard_breakdown(
|
||||
dashboards_with_charts = (
|
||||
db.session.query(Dashboard).join(Dashboard.slices).distinct().count()
|
||||
)
|
||||
dashboards_without_charts = (
|
||||
base_counts.get("total_dashboards", 0) - dashboards_with_charts
|
||||
dashboards_without_charts = max(
|
||||
0, base_counts.get("total_dashboards", 0) - dashboards_with_charts
|
||||
)
|
||||
|
||||
return DashboardBreakdown(
|
||||
|
||||
@@ -20,6 +20,7 @@
|
||||
import datetime
|
||||
import logging
|
||||
import platform
|
||||
import time
|
||||
|
||||
from flask import current_app
|
||||
from superset_core.mcp.decorators import tool
|
||||
@@ -30,6 +31,8 @@ from superset.utils.version import get_version_metadata
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
_start_time = time.monotonic()
|
||||
|
||||
|
||||
@tool(tags=["core"])
|
||||
async def health_check() -> HealthCheckResponse:
|
||||
@@ -77,6 +80,7 @@ async def health_check() -> HealthCheckResponse:
|
||||
version=version,
|
||||
python_version=platform.python_version(),
|
||||
platform=platform.system(),
|
||||
uptime_seconds=round(time.monotonic() - _start_time, 1),
|
||||
)
|
||||
|
||||
logger.info("Health check completed successfully")
|
||||
|
||||
@@ -64,6 +64,21 @@ def _mock_chart(id: int = 1, slice_name: str = "Test Chart") -> Mock:
|
||||
chart.id = id
|
||||
chart.slice_name = slice_name
|
||||
chart.uuid = f"chart-uuid-{id}"
|
||||
chart.tags = []
|
||||
chart.owners = []
|
||||
chart.viz_type = "table"
|
||||
chart.datasource_name = None
|
||||
chart.datasource_type = None
|
||||
chart.description = None
|
||||
chart.cache_timeout = None
|
||||
chart.changed_by = None
|
||||
chart.changed_by_name = None
|
||||
chart.changed_on = None
|
||||
chart.changed_on_humanized = None
|
||||
chart.created_by = None
|
||||
chart.created_by_name = None
|
||||
chart.created_on = None
|
||||
chart.created_on_humanized = None
|
||||
return chart
|
||||
|
||||
|
||||
@@ -390,7 +405,7 @@ class TestAddChartToExistingDashboard:
|
||||
):
|
||||
"""Test adding a chart to an existing dashboard."""
|
||||
mock_dashboard = _mock_dashboard(id=1, title="Existing Dashboard")
|
||||
mock_dashboard.slices = [Mock(id=10), Mock(id=20)]
|
||||
mock_dashboard.slices = [_mock_chart(id=10), _mock_chart(id=20)]
|
||||
mock_dashboard.position_json = json.dumps(
|
||||
{
|
||||
"ROOT_ID": {
|
||||
@@ -430,7 +445,11 @@ class TestAddChartToExistingDashboard:
|
||||
mock_db_session.get.return_value = mock_chart
|
||||
|
||||
updated_dashboard = _mock_dashboard(id=1, title="Existing Dashboard")
|
||||
updated_dashboard.slices = [Mock(id=10), Mock(id=20), Mock(id=30)]
|
||||
updated_dashboard.slices = [
|
||||
_mock_chart(id=10),
|
||||
_mock_chart(id=20),
|
||||
_mock_chart(id=30),
|
||||
]
|
||||
mock_update_command.return_value.run.return_value = updated_dashboard
|
||||
|
||||
request = {"dashboard_id": 1, "chart_id": 30}
|
||||
@@ -505,7 +524,7 @@ class TestAddChartToExistingDashboard:
|
||||
):
|
||||
"""Test error when chart is already in dashboard."""
|
||||
mock_dashboard = _mock_dashboard()
|
||||
mock_dashboard.slices = [Mock(id=5)]
|
||||
mock_dashboard.slices = [_mock_chart(id=5)]
|
||||
mock_find_dashboard.return_value = mock_dashboard
|
||||
mock_db_session.get.return_value = _mock_chart(id=5)
|
||||
request = {"dashboard_id": 1, "chart_id": 5}
|
||||
@@ -537,7 +556,7 @@ class TestAddChartToExistingDashboard:
|
||||
mock_db_session.get.return_value = mock_chart
|
||||
|
||||
updated_dashboard = _mock_dashboard(id=2)
|
||||
updated_dashboard.slices = [Mock(id=15)]
|
||||
updated_dashboard.slices = [_mock_chart(id=15)]
|
||||
mock_update_command.return_value.run.return_value = updated_dashboard
|
||||
|
||||
request = {"dashboard_id": 2, "chart_id": 15}
|
||||
@@ -585,7 +604,7 @@ class TestAddChartToExistingDashboard:
|
||||
):
|
||||
"""Test adding chart to a dashboard that uses tabs."""
|
||||
mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
|
||||
mock_dashboard.slices = [Mock(id=10)]
|
||||
mock_dashboard.slices = [_mock_chart(id=10)]
|
||||
mock_dashboard.position_json = json.dumps(
|
||||
{
|
||||
"ROOT_ID": {
|
||||
@@ -646,7 +665,7 @@ class TestAddChartToExistingDashboard:
|
||||
mock_db_session.get.return_value = mock_chart
|
||||
|
||||
updated_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
|
||||
updated_dashboard.slices = [Mock(id=10), Mock(id=25)]
|
||||
updated_dashboard.slices = [_mock_chart(id=10), _mock_chart(id=25)]
|
||||
mock_update_command.return_value.run.return_value = updated_dashboard
|
||||
|
||||
request = {"dashboard_id": 3, "chart_id": 25}
|
||||
@@ -685,7 +704,7 @@ class TestAddChartToExistingDashboard:
|
||||
):
|
||||
"""Test adding chart to a specific tab using target_tab name."""
|
||||
mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
|
||||
mock_dashboard.slices = [Mock(id=10)]
|
||||
mock_dashboard.slices = [_mock_chart(id=10)]
|
||||
mock_dashboard.position_json = json.dumps(
|
||||
{
|
||||
"ROOT_ID": {
|
||||
@@ -746,7 +765,7 @@ class TestAddChartToExistingDashboard:
|
||||
mock_db_session.get.return_value = mock_chart
|
||||
|
||||
updated_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
|
||||
updated_dashboard.slices = [Mock(id=10), Mock(id=30)]
|
||||
updated_dashboard.slices = [_mock_chart(id=10), _mock_chart(id=30)]
|
||||
mock_update_command.return_value.run.return_value = updated_dashboard
|
||||
|
||||
request = {"dashboard_id": 3, "chart_id": 30, "target_tab": "Customers"}
|
||||
@@ -786,7 +805,7 @@ class TestAddChartToExistingDashboard:
|
||||
):
|
||||
"""Test adding chart to dashboard that has nanoid-style ROW IDs."""
|
||||
mock_dashboard = _mock_dashboard(id=4, title="Nanoid Dashboard")
|
||||
mock_dashboard.slices = [Mock(id=10)]
|
||||
mock_dashboard.slices = [_mock_chart(id=10)]
|
||||
mock_dashboard.position_json = json.dumps(
|
||||
{
|
||||
"ROOT_ID": {
|
||||
@@ -821,7 +840,7 @@ class TestAddChartToExistingDashboard:
|
||||
mock_db_session.get.return_value = mock_chart
|
||||
|
||||
updated_dashboard = _mock_dashboard(id=4, title="Nanoid Dashboard")
|
||||
updated_dashboard.slices = [Mock(id=10), Mock(id=50)]
|
||||
updated_dashboard.slices = [_mock_chart(id=10), _mock_chart(id=50)]
|
||||
mock_update_command.return_value.run.return_value = updated_dashboard
|
||||
|
||||
request = {"dashboard_id": 4, "chart_id": 50}
|
||||
|
||||
@@ -1319,7 +1319,7 @@ class TestDatasetDefaultColumnFiltering:
|
||||
dataset_item = data["datasets"][0]
|
||||
|
||||
# Verify ONLY default columns are present in the response item
|
||||
expected_keys = {"id", "table_name", "schema_name", "changed_on_humanized"}
|
||||
expected_keys = {"id", "table_name", "schema", "changed_on_humanized"}
|
||||
actual_keys = set(dataset_item.keys())
|
||||
|
||||
# The response should only contain the default columns, NOT all columns
|
||||
|
||||
@@ -702,6 +702,41 @@ class TestExecuteSql:
|
||||
with pytest.raises(ToolError, match="less than or equal to 10000"):
|
||||
await client.call_tool("execute_sql", {"request": request})
|
||||
|
||||
@patch("superset.security_manager")
|
||||
@patch("superset.db")
|
||||
@pytest.mark.asyncio
|
||||
async def test_execute_sql_no_limit_respects_sql(
|
||||
self, mock_db, mock_security_manager, mcp_server
|
||||
):
|
||||
"""Test that omitting limit lets the SQL LIMIT clause be respected."""
|
||||
mock_database = _mock_database()
|
||||
mock_database.execute.return_value = _create_select_result(
|
||||
rows=[{"id": i} for i in range(5)],
|
||||
columns=["id"],
|
||||
original_sql="SELECT id FROM users LIMIT 5",
|
||||
)
|
||||
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
|
||||
mock_database
|
||||
)
|
||||
mock_security_manager.can_access_database.return_value = True
|
||||
|
||||
# No 'limit' key — should default to None (no override)
|
||||
request = {
|
||||
"database_id": 1,
|
||||
"sql": "SELECT id FROM users LIMIT 5",
|
||||
}
|
||||
|
||||
async with Client(mcp_server) as client:
|
||||
result = await client.call_tool("execute_sql", {"request": request})
|
||||
|
||||
data = result.structured_content
|
||||
assert data["success"] is True
|
||||
|
||||
# Verify limit=None was passed to QueryOptions (no override)
|
||||
call_args = mock_database.execute.call_args
|
||||
options = call_args[0][1]
|
||||
assert options.limit is None
|
||||
|
||||
@patch("superset.security_manager")
|
||||
@patch("superset.db")
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -366,6 +366,6 @@ def test_chart_filter_existing_columns_still_work():
|
||||
|
||||
def test_dashboard_filter_existing_columns_still_work():
|
||||
"""Test that pre-existing dashboard filter columns are not broken."""
|
||||
for col in ("dashboard_title", "published", "favorite"):
|
||||
for col in ("dashboard_title", "published", "created_by_fk"):
|
||||
f = DashboardFilter(col=col, opr="eq", value="test")
|
||||
assert f.col == col
|
||||
|
||||
@@ -95,10 +95,11 @@ def test_model_list_tool_basic():
|
||||
assert result.count == 2
|
||||
assert result.total_count == 2
|
||||
assert isinstance(result.items[0], DummyOutputSchema)
|
||||
assert result.page == 1
|
||||
# run_tool receives 0-based page; response reports 1-based (page+1)
|
||||
assert result.page == 2
|
||||
assert result.page_size == 2
|
||||
assert result.total_pages == 1
|
||||
# For page=1, ModelListCore sets has_previous=True
|
||||
# For page=1 (0-based), ModelListCore sets has_previous=True
|
||||
assert result.has_previous is True
|
||||
assert result.has_next is False
|
||||
assert result.columns_requested == ["id", "name"]
|
||||
|
||||
Reference in New Issue
Block a user