diff --git a/superset/daos/chart.py b/superset/daos/chart.py index 44d0057eea8..352b9b9cb68 100644 --- a/superset/daos/chart.py +++ b/superset/daos/chart.py @@ -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"], } diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py index 3e8b7c8d82f..d0d6a027a94 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -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() diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py index 30476c01eb6..6f4bbc51320 100644 --- a/superset/daos/dataset.py +++ b/superset/daos/dataset.py @@ -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) diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index b1e5be880c1..bdaae915703 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -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( diff --git a/superset/mcp_service/chart/preview_utils.py b/superset/mcp_service/chart/preview_utils.py index c28681a9da0..5b930797b06 100644 --- a/superset/mcp_service/chart/preview_utils.py +++ b/superset/mcp_service/chart/preview_utils.py @@ -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) diff --git a/superset/mcp_service/chart/schemas.py b/superset/mcp_service/chart/schemas.py index 155d455f372..4cc2bffa07f 100644 --- a/superset/mcp_service/chart/schemas.py +++ b/superset/mcp_service/chart/schemas.py @@ -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" ), diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index 7cec6e9e71f..76ce79b5212 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -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: diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py index b6edb257f08..c6f9123c5cc 100644 --- a/superset/mcp_service/chart/tool/get_chart_data.py +++ b/superset/mcp_service/chart/tool/get_chart_data.py @@ -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), diff --git a/superset/mcp_service/chart/tool/get_chart_preview.py b/superset/mcp_service/chart/tool/get_chart_preview.py index 5fea5c461e6..d95e9500661 100644 --- a/superset/mcp_service/chart/tool/get_chart_preview.py +++ b/superset/mcp_service/chart/tool/get_chart_preview.py @@ -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 diff --git a/superset/mcp_service/chart/tool/update_chart.py b/superset/mcp_service/chart/tool/update_chart.py index 7abc1ef9da3..d69e09693a9 100644 --- a/superset/mcp_service/chart/tool/update_chart.py +++ b/superset/mcp_service/chart/tool/update_chart.py @@ -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 diff --git a/superset/mcp_service/chart/tool/update_chart_preview.py b/superset/mcp_service/chart/tool/update_chart_preview.py index 117a92a6331..f3242b14091 100644 --- a/superset/mcp_service/chart/tool/update_chart_preview.py +++ b/superset/mcp_service/chart/tool/update_chart_preview.py @@ -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) diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index e09b3871ce6..48aa72a952c 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -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", []) ] diff --git a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py index 19f17c95a5e..2629aded0dc 100644 --- a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py +++ b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py @@ -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 = ( diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py index 61285c1a874..4bbc4450502 100644 --- a/superset/mcp_service/dashboard/tool/generate_dashboard.py +++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py @@ -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}/" diff --git a/superset/mcp_service/dataset/schemas.py b/superset/mcp_service/dataset/schemas.py index 14dd329e8f7..1fc5d67e112 100644 --- a/superset/mcp_service/dataset/schemas.py +++ b/superset/mcp_service/dataset/schemas.py @@ -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), diff --git a/superset/mcp_service/mcp_core.py b/superset/mcp_service/mcp_core.py index 3ac0f008ca4..0b1a817d26d 100644 --- a/superset/mcp_service/mcp_core.py +++ b/superset/mcp_service/mcp_core.py @@ -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, diff --git a/superset/mcp_service/sql_lab/schemas.py b/superset/mcp_service/sql_lab/schemas.py index 36f76980704..436e9330733 100644 --- a/superset/mcp_service/sql_lab/schemas.py +++ b/superset/mcp_service/sql_lab/schemas.py @@ -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") diff --git a/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py b/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py index 15a1919ffbf..d326cf26244 100644 --- a/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py +++ b/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py @@ -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}" diff --git a/superset/mcp_service/system/system_utils.py b/superset/mcp_service/system/system_utils.py index b9a4b8759f3..75fe81af5f8 100644 --- a/superset/mcp_service/system/system_utils.py +++ b/superset/mcp_service/system/system_utils.py @@ -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( diff --git a/superset/mcp_service/system/tool/health_check.py b/superset/mcp_service/system/tool/health_check.py index f5393d7cfb7..7faf3f18ee7 100644 --- a/superset/mcp_service/system/tool/health_check.py +++ b/superset/mcp_service/system/tool/health_check.py @@ -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") diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py index 912bb74fc08..b382f1f4ab2 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py @@ -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} diff --git a/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py b/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py index 6ea8342be13..48742973863 100644 --- a/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py +++ b/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py @@ -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 diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py index c9d01def8f9..3576acd3d37 100644 --- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py +++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py @@ -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 diff --git a/tests/unit_tests/mcp_service/system/tool/test_get_current_user.py b/tests/unit_tests/mcp_service/system/tool/test_get_current_user.py index e8fb24674e2..01ac357f3eb 100644 --- a/tests/unit_tests/mcp_service/system/tool/test_get_current_user.py +++ b/tests/unit_tests/mcp_service/system/tool/test_get_current_user.py @@ -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 diff --git a/tests/unit_tests/mcp_service/system/tool/test_mcp_core.py b/tests/unit_tests/mcp_service/system/tool/test_mcp_core.py index aefdd8c8433..945b5fea084 100644 --- a/tests/unit_tests/mcp_service/system/tool/test_mcp_core.py +++ b/tests/unit_tests/mcp_service/system/tool/test_mcp_core.py @@ -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"]