This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new e6d088d6b1b fix(mcp): surface rejected filter columns in get_chart_sql
(#43680)
e6d088d6b1b is described below
commit e6d088d6b1bf460dcb0653eb539f606893b4283a
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Tue Sep 8 18:05:37 2026 -0400
fix(mcp): surface rejected filter columns in get_chart_sql (#43680)
---
superset/common/query_actions.py | 78 +++++++++++++---
superset/mcp_service/chart/chart_helpers.py | 70 ++++++++++++++
superset/mcp_service/chart/tool/get_chart_data.py | 64 +------------
superset/mcp_service/chart/tool/get_chart_sql.py | 19 +++-
superset/models/helpers.py | 9 +-
.../charts/commands/data/test_get_data_command.py | 15 ++-
tests/unit_tests/common/test_query_actions.py | 97 ++++++++++++++++++++
.../mcp_service/chart/tool/test_get_chart_data.py | 39 ++++++--
.../mcp_service/chart/tool/test_get_chart_sql.py | 102 +++++++++++++++++++++
9 files changed, 404 insertions(+), 89 deletions(-)
diff --git a/superset/common/query_actions.py b/superset/common/query_actions.py
index 7668ebfe0c9..1591e92338c 100644
--- a/superset/common/query_actions.py
+++ b/superset/common/query_actions.py
@@ -78,6 +78,41 @@ def _get_timegrains(
return {"data": grains}
+def _filter_status(
+ datasource: Explorable,
+ query_obj: QueryObject,
+ applied_filter_columns: list[Any],
+ rejected_filter_columns: list[Any],
+) -> dict[str, Any]:
+ """Describe which of the query's filters reached the generated SQL.
+
+ Used by both SQL-only and data-bearing results so the public filter status
+ and the datasource-only rejection carrier cannot diverge.
+ """
+ applied_time_columns, rejected_time_columns = get_time_filter_status(
+ datasource, query_obj.applied_time_extras
+ )
+ return {
+ "applied_filters": [
+ {"column": get_column_name(col)} for col in applied_filter_columns
+ ]
+ + applied_time_columns,
+ "rejected_filters": [
+ {
+ "reason": ExtraFiltersReasonType.COL_NOT_IN_DATASOURCE,
+ "column": get_column_name(col),
+ }
+ for col in rejected_filter_columns
+ ]
+ + rejected_time_columns,
+ # Keep the datasource rejection origin available to consumers that
+ # must distinguish it from temporal pseudo-filter status above.
+ "rejected_filter_columns": [
+ get_column_name(col) for col in rejected_filter_columns
+ ],
+ }
+
+
def _get_query(
query_context: QueryContext,
query_obj: QueryObject,
@@ -86,7 +121,26 @@ def _get_query(
datasource = _get_datasource(query_context, query_obj)
result = {"language": datasource.query_language}
try:
- result["query"] = datasource.get_query_str(query_obj.to_dict())
+ # Prefer the extended form so the rejected/applied filter columns the
+ # datasource computed while building the query are not discarded: a
+ # filter silently dropped during query construction is otherwise
+ # invisible to anyone requesting only the SQL. Datasources that do not
+ # implement it (e.g. semantic layers) keep the plain string form.
+ if get_query_str_extended := getattr(
+ datasource, "get_query_str_extended", None
+ ):
+ extended = get_query_str_extended(query_obj.to_dict())
+ result["query"] = extended.full_sql
+ result.update(
+ _filter_status(
+ datasource,
+ query_obj,
+ extended.applied_filter_columns,
+ extended.rejected_filter_columns,
+ )
+ )
+ else:
+ result["query"] = datasource.get_query_str(query_obj.to_dict())
except QueryObjectValidationError as err:
# Validation errors (missing required fields, invalid config)
# No SQL was generated
@@ -185,24 +239,18 @@ def _materialize_full_payload(
)
del payload["df"]
- applied_time_columns, rejected_time_columns = get_time_filter_status(
- datasource, query_obj.applied_time_extras
- )
-
applied_filter_columns = payload.get("applied_filter_columns", [])
rejected_filter_columns = payload.get("rejected_filter_columns", [])
del payload["applied_filter_columns"]
del payload["rejected_filter_columns"]
- payload["applied_filters"] = [
- {"column": get_column_name(col)} for col in applied_filter_columns
- ] + applied_time_columns
- payload["rejected_filters"] = [
- {
- "reason": ExtraFiltersReasonType.COL_NOT_IN_DATASOURCE,
- "column": get_column_name(col),
- }
- for col in rejected_filter_columns
- ] + rejected_time_columns
+ payload.update(
+ _filter_status(
+ datasource,
+ query_obj,
+ applied_filter_columns,
+ rejected_filter_columns,
+ )
+ )
if result_type == ChartDataResultType.RESULTS and status !=
QueryStatus.FAILED:
return {
diff --git a/superset/mcp_service/chart/chart_helpers.py
b/superset/mcp_service/chart/chart_helpers.py
index 95da1318576..f76e3426f24 100644
--- a/superset/mcp_service/chart/chart_helpers.py
+++ b/superset/mcp_service/chart/chart_helpers.py
@@ -30,6 +30,7 @@ from typing import Any, TYPE_CHECKING
from urllib.parse import parse_qs, urlparse
from superset.constants import EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS
+from superset.utils.core import ExtraFiltersReasonType
if TYPE_CHECKING:
from superset.mcp_service.chart.schemas import AppliedDashboardFilter
@@ -62,6 +63,75 @@ class ChartNotOnDashboardError(ValueError):
"""Raised when a chart is not part of the given dashboard's slices."""
+def requested_filter_columns(extra_form_data: dict[str, Any] | None) ->
set[str]:
+ """Return simple column names explicitly requested through extra form
data."""
+ if not extra_form_data:
+ return set()
+
+ columns: set[str] = set()
+ for filter_ in extra_form_data.get("filters") or []:
+ if isinstance(filter_, dict) and isinstance(column :=
filter_.get("col"), str):
+ columns.add(column)
+ for filter_ in extra_form_data.get("adhoc_filters") or []:
+ if (
+ isinstance(filter_, dict)
+ and filter_.get("expressionType") == "SIMPLE"
+ and isinstance(column := filter_.get("subject"), str)
+ ):
+ columns.add(column)
+ return columns
+
+
+def rejected_columns_in_query(query: Any) -> set[str]:
+ """Return the rejected filter column names reported by one query payload.
+
+ Query construction reports dropped filters as ``rejected_filters`` entries
+ (``{"reason": ..., "column": ...}``), the shape every consumer of a
+ chart-data or query payload sees. The raw ``rejected_filter_columns`` list
+ is still accepted for payloads captured before that conversion.
+ """
+ if not isinstance(query, dict):
+ return set()
+
+ # QUERY results retain the datasource-only list so temporal pseudo-filter
+ # rejections cannot be mistaken for ordinary filters with the same name.
+ # Prefer it whenever present, including when it is empty.
+ if "rejected_filter_columns" in query:
+ return {
+ column
+ for column in query.get("rejected_filter_columns") or []
+ if isinstance(column, str)
+ }
+
+ columns = {
+ column
+ for entry in query.get("rejected_filters") or []
+ if isinstance(entry, dict)
+ and entry.get("reason") != ExtraFiltersReasonType.NO_TEMPORAL_COLUMN
+ and isinstance(column := entry.get("column"), str)
+ }
+ return columns
+
+
+def rejected_requested_filter_columns(
+ result: Any, extra_form_data: dict[str, Any] | None
+) -> list[str]:
+ """Find request filters rejected by datasource query construction.
+
+ Only columns the caller asked for are reported, so a stale filter stored in
+ an older chart configuration cannot fail the request.
+ """
+ if not isinstance(result, dict):
+ return []
+ requested = requested_filter_columns(extra_form_data)
+ rejected = {
+ column
+ for query in result.get("queries", [])
+ for column in rejected_columns_in_query(query)
+ }
+ return sorted(requested & rejected)
+
+
def find_chart_by_identifier(
identifier: int | str,
query_options: list[Any] | None = None,
diff --git a/superset/mcp_service/chart/tool/get_chart_data.py
b/superset/mcp_service/chart/tool/get_chart_data.py
index 1016ea01b9c..77f4904c8c8 100644
--- a/superset/mcp_service/chart/tool/get_chart_data.py
+++ b/superset/mcp_service/chart/tool/get_chart_data.py
@@ -43,6 +43,7 @@ from superset.mcp_service.chart.chart_helpers import (
find_chart_by_identifier,
get_cached_form_data,
merge_extra_form_data_filters_into_query,
+ rejected_requested_filter_columns,
)
from superset.mcp_service.chart.chart_utils import validate_chart_dataset
from superset.mcp_service.chart.schemas import (
@@ -63,65 +64,6 @@ from superset.utils.core import GenericDataType
logger = logging.getLogger(__name__)
-def _requested_filter_columns(extra_form_data: dict[str, Any] | None) ->
set[str]:
- """Return simple column names explicitly requested through extra form
data."""
- if not extra_form_data:
- return set()
-
- columns: set[str] = set()
- for filter_ in extra_form_data.get("filters", []):
- if isinstance(filter_, dict) and isinstance(column :=
filter_.get("col"), str):
- columns.add(column)
- for filter_ in extra_form_data.get("adhoc_filters", []):
- if (
- isinstance(filter_, dict)
- and filter_.get("expressionType") == "SIMPLE"
- and isinstance(column := filter_.get("subject"), str)
- ):
- columns.add(column)
- return columns
-
-
-def _rejected_columns_in_query(query: Any) -> set[str]:
- """Return the rejected filter column names reported by one query payload.
-
- ``_materialize_full_payload`` converts the datasource's raw
- ``rejected_filter_columns`` list into the ``rejected_filters`` entries
- (``{"reason": ..., "column": ...}``) that every consumer of a chart-data
- payload sees, so that is the primary shape to read. The raw key is still
- accepted for payloads captured before that conversion.
- """
- if not isinstance(query, dict):
- return set()
-
- columns = {
- column
- for entry in query.get("rejected_filters", [])
- if isinstance(entry, dict) and isinstance(column :=
entry.get("column"), str)
- }
- columns.update(
- column
- for column in query.get("rejected_filter_columns", [])
- if isinstance(column, str)
- )
- return columns
-
-
-def _rejected_requested_filter_columns(
- result: Any, extra_form_data: dict[str, Any] | None
-) -> list[str]:
- """Find request filters rejected by datasource query construction."""
- if not isinstance(result, dict):
- return []
- requested = _requested_filter_columns(extra_form_data)
- rejected = {
- column
- for query in result.get("queries", [])
- for column in _rejected_columns_in_query(query)
- }
- return sorted(requested & rejected)
-
-
_GENERIC_TYPE_MAP: dict[int, str] = {
GenericDataType.NUMERIC: "numeric",
GenericDataType.STRING: "string",
@@ -747,7 +689,7 @@ async def get_chart_data( # noqa: C901
command.validate()
result = command.run()
- if rejected := _rejected_requested_filter_columns(
+ if rejected := rejected_requested_filter_columns(
result, request.extra_form_data
):
rejected_columns = ", ".join(rejected)
@@ -1109,7 +1051,7 @@ async def _query_from_form_data( # noqa: C901
command.validate()
result = command.run()
- if rejected := _rejected_requested_filter_columns(
+ if rejected := rejected_requested_filter_columns(
result, request.extra_form_data
):
rejected_columns = ", ".join(rejected)
diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py
b/superset/mcp_service/chart/tool/get_chart_sql.py
index 3914e2672ce..33a7276ba95 100644
--- a/superset/mcp_service/chart/tool/get_chart_sql.py
+++ b/superset/mcp_service/chart/tool/get_chart_sql.py
@@ -38,6 +38,7 @@ from superset.mcp_service.chart.chart_helpers import (
build_query_context_from_form_data,
extract_x_axis_col,
merge_extra_form_data_filters_into_query,
+ rejected_requested_filter_columns,
resolve_form_data_datasource,
resolve_groupby,
resolve_metrics,
@@ -238,7 +239,11 @@ def _sql_from_saved_query_context(
result = command.run()
return _extract_sql_from_result(
- result, chart.id, chart.slice_name, chart.datasource_name
+ result,
+ chart.id,
+ chart.slice_name,
+ chart.datasource_name,
+ extra_form_data=extra_form_data,
)
except SupersetSecurityException:
raise # Let access denials propagate for consistent error handling
@@ -326,6 +331,7 @@ def _sql_from_form_data(
chart_id=getattr(chart, "id", None),
chart_name=getattr(chart, "slice_name", None),
datasource_name=_resolve_datasource_name(form_data, chart),
+ extra_form_data=extra_form_data,
)
@@ -334,6 +340,7 @@ def _extract_sql_from_result(
chart_id: int | None,
chart_name: str | None,
datasource_name: str | None,
+ extra_form_data: dict[str, Any] | None = None,
) -> ChartSql | ChartError:
"""Extract SQL query string(s) from the ChartDataCommand result.
@@ -350,6 +357,16 @@ def _extract_sql_from_result(
error_type="EmptyQuery",
)
+ # A filter naming a column the dataset does not have is dropped during
query
+ # construction. Returning the resulting unfiltered SQL as a success would
+ # misrepresent it as the SQL for the filters that were asked for.
+ if rejected := rejected_requested_filter_columns(result, extra_form_data):
+ rejected_columns = ", ".join(rejected)
+ return ChartError(
+ error=f"Unknown dataset column(s) in filters: {rejected_columns}",
+ error_type="ValidationError",
+ )
+
sql_parts: list[str] = []
errors: list[str] = []
language = "sql"
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index 8d2cae5ce36..06f8274025f 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -1745,6 +1745,11 @@ class QueryStringExtended(NamedTuple):
sql: str
sql_shifted_temporal_labels: set[str]
+ @property
+ def full_sql(self) -> str:
+ """The prequeries and the main query as one displayable statement."""
+ return ";\n\n".join([*self.prequeries, self.sql]) + ";"
+
class SqlaQuery(NamedTuple):
applied_template_filters: list[str]
@@ -3864,9 +3869,7 @@ class ExploreMixin: # pylint:
disable=too-many-public-methods
return values
def get_query_str(self, query_obj: QueryObjectDict) -> str:
- query_str_ext = self.get_query_str_extended(query_obj)
- all_queries = query_str_ext.prequeries + [query_str_ext.sql]
- return ";\n\n".join(all_queries) + ";"
+ return self.get_query_str_extended(query_obj).full_sql
def _get_series_orderby(
self,
diff --git a/tests/unit_tests/charts/commands/data/test_get_data_command.py
b/tests/unit_tests/charts/commands/data/test_get_data_command.py
index c4d7c9281a1..e4691653df1 100644
--- a/tests/unit_tests/charts/commands/data/test_get_data_command.py
+++ b/tests/unit_tests/charts/commands/data/test_get_data_command.py
@@ -291,7 +291,10 @@ def test_get_query_catches_parsing_error() -> None:
with patch("superset.common.query_actions._get_datasource") as mock_get_ds:
mock_datasource = Mock()
mock_datasource.query_language = "sql"
- mock_datasource.get_query_str.side_effect = parse_error
+ # SQL is compiled through get_query_str_extended so the datasource's
+ # applied/rejected filter columns survive into the payload; the parse
+ # error surfaces from there.
+ mock_datasource.get_query_str_extended.side_effect = parse_error
mock_get_ds.return_value = mock_datasource
# GREEN: Exception is caught, values returned (new behavior after fix)
@@ -330,7 +333,10 @@ def
test_get_query_handles_parsing_error_with_missing_sql_key() -> None:
with patch("superset.common.query_actions._get_datasource") as mock_get_ds:
mock_datasource = Mock()
mock_datasource.query_language = "sql"
- mock_datasource.get_query_str.side_effect = parse_error
+ # SQL is compiled through get_query_str_extended so the datasource's
+ # applied/rejected filter columns survive into the payload; the parse
+ # error surfaces from there.
+ mock_datasource.get_query_str_extended.side_effect = parse_error
mock_get_ds.return_value = mock_datasource
result = _get_query(mock_query_context, mock_query_obj, False)
@@ -367,7 +373,10 @@ def
test_get_query_handles_parsing_error_with_null_sql_value() -> None:
with patch("superset.common.query_actions._get_datasource") as mock_get_ds:
mock_datasource = Mock()
mock_datasource.query_language = "sql"
- mock_datasource.get_query_str.side_effect = parse_error
+ # SQL is compiled through get_query_str_extended so the datasource's
+ # applied/rejected filter columns survive into the payload; the parse
+ # error surfaces from there.
+ mock_datasource.get_query_str_extended.side_effect = parse_error
mock_get_ds.return_value = mock_datasource
result = _get_query(mock_query_context, mock_query_obj, False)
diff --git a/tests/unit_tests/common/test_query_actions.py
b/tests/unit_tests/common/test_query_actions.py
index 0cb68b7b1f2..996f427482b 100644
--- a/tests/unit_tests/common/test_query_actions.py
+++ b/tests/unit_tests/common/test_query_actions.py
@@ -25,6 +25,7 @@ from superset.common.chart_data_timing import (
QueryAcquisitionResult,
QueryAcquisitionTiming,
)
+from superset.common.db_query_status import QueryStatus
from superset.common.query_actions import (
_prepare_drill_detail_query,
_prepare_samples_query,
@@ -281,3 +282,99 @@ def
test_legacy_result_wrapper_delegates_to_timed_resolver() -> None:
query_obj,
False,
)
+
+
+def test_get_query_surfaces_rejected_filter_columns() -> None:
+ """The QUERY result type must report filters dropped while building the
SQL.
+
+ A filter naming a column the dataset does not have is silently discarded
+ during query construction. Without this, a caller asking only for the SQL
+ (e.g. the MCP get_chart_sql tool, or "View query") receives the unfiltered
+ statement with no indication that a filter was dropped.
+ """
+ from superset.common.query_actions import _get_query
+ from superset.models.helpers import QueryStringExtended
+
+ extended = QueryStringExtended(
+ applied_template_filters=[],
+ applied_filter_columns=["country"],
+ rejected_filter_columns=["does_not_exist"],
+ labels_expected=[],
+ prequeries=[],
+ sql="SELECT country FROM sales",
+ sql_shifted_temporal_labels=set(),
+ )
+
+ mock_query_obj = MagicMock()
+ mock_query_obj.to_dict.return_value = {}
+ mock_query_obj.applied_time_extras = {}
+
+ with (
+ patch.object(query_actions, "_get_datasource") as mock_get_ds,
+ patch.object(query_actions, "get_time_filter_status",
return_value=([], [])),
+ ):
+ datasource = MagicMock()
+ datasource.query_language = "sql"
+ datasource.get_query_str_extended.return_value = extended
+ mock_get_ds.return_value = datasource
+
+ result = _get_query(MagicMock(), mock_query_obj, False)
+
+ assert result["query"] == "SELECT country FROM sales;"
+ assert result["applied_filters"] == [{"column": "country"}]
+ assert [entry["column"] for entry in result["rejected_filters"]] == [
+ "does_not_exist"
+ ]
+ assert result["rejected_filter_columns"] == ["does_not_exist"]
+
+
+def test_materialized_payload_preserves_datasource_rejection_origin() -> None:
+ """Data results retain provenance alongside combined temporal status."""
+ query_context = MagicMock()
+ query_context.result_type = ChartDataResultType.FULL
+ query_obj = MagicMock()
+ query_obj.result_type = ChartDataResultType.FULL
+ query_obj.applied_time_extras = {}
+ payload = {
+ "df": None,
+ "status": QueryStatus.FAILED,
+ "applied_filter_columns": [],
+ "rejected_filter_columns": ["__time_col"],
+ }
+
+ with (
+ patch.object(query_actions, "_get_datasource",
return_value=MagicMock()),
+ patch.object(
+ query_actions,
+ "get_time_filter_status",
+ return_value=(
+ [],
+ [{"reason": "not_in_datasource", "column": "__time_col"}],
+ ),
+ ),
+ ):
+ result = query_actions._materialize_full_payload(
+ query_context, query_obj, payload
+ )
+
+ assert result["rejected_filter_columns"] == ["__time_col"]
+ assert len(result["rejected_filters"]) == 2
+
+
+def test_get_query_falls_back_when_datasource_has_no_extended_form() -> None:
+ """Datasources without get_query_str_extended keep the plain string
path."""
+ from superset.common.query_actions import _get_query
+
+ mock_query_obj = MagicMock()
+ mock_query_obj.to_dict.return_value = {}
+
+ with patch.object(query_actions, "_get_datasource") as mock_get_ds:
+ datasource = MagicMock(spec=["query_language", "get_query_str"])
+ datasource.query_language = "sql"
+ datasource.get_query_str.return_value = "SELECT 1;"
+ mock_get_ds.return_value = datasource
+
+ result = _get_query(MagicMock(), mock_query_obj, False)
+
+ assert result["query"] == "SELECT 1;"
+ assert "rejected_filters" not in result
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
index 789d53409f5..4bb74aaff0d 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
@@ -27,6 +27,10 @@ from unittest.mock import MagicMock
import pytest
+from superset.mcp_service.chart.chart_helpers import (
+ rejected_requested_filter_columns,
+ requested_filter_columns,
+)
from superset.mcp_service.chart.schemas import (
ChartData,
ChartError,
@@ -41,15 +45,13 @@ from superset.mcp_service.chart.tool.get_chart_data import (
_MAX_RECOMMENDATIONS,
_query_from_form_data,
_recommend_visualizations,
- _rejected_requested_filter_columns,
- _requested_filter_columns,
)
from superset.utils import json
from superset.utils.core import ExtraFiltersReasonType, GenericDataType
def test_requested_filter_columns_supports_both_payload_shapes() -> None:
- assert _requested_filter_columns(
+ assert requested_filter_columns(
{
"filters": [{"col": "country", "op": "==", "val": "USA"}],
"adhoc_filters": [
@@ -65,6 +67,10 @@ def
test_requested_filter_columns_supports_both_payload_shapes() -> None:
) == {"country", "city"}
+def test_requested_filter_columns_accepts_null_lists() -> None:
+ assert requested_filter_columns({"filters": None, "adhoc_filters": None})
== set()
+
+
def test_rejected_requested_filter_columns_ignores_saved_chart_filters() ->
None:
result = {
"queries": [
@@ -72,7 +78,7 @@ def
test_rejected_requested_filter_columns_ignores_saved_chart_filters() -> None
]
}
- assert _rejected_requested_filter_columns(
+ assert rejected_requested_filter_columns(
result,
{
"adhoc_filters": [
@@ -109,7 +115,7 @@ def
test_rejected_requested_filter_columns_reads_materialized_payload() -> None:
]
}
- assert _rejected_requested_filter_columns(
+ assert rejected_requested_filter_columns(
result,
{"filters": [{"col": "does_not_exist", "op": "==", "val": "x"}]},
) == ["does_not_exist"]
@@ -129,7 +135,7 @@ def
test_rejected_requested_filter_columns_ignores_rejected_time_filters() -> No
}
assert (
- _rejected_requested_filter_columns(
+ rejected_requested_filter_columns(
result,
{"filters": [{"col": "country", "op": "==", "val": "USA"}]},
)
@@ -137,6 +143,27 @@ def
test_rejected_requested_filter_columns_ignores_rejected_time_filters() -> No
)
+def test_rejected_requested_filter_columns_prefers_datasource_rejections() ->
None:
+ result = {
+ "queries": [
+ {
+ "rejected_filter_columns": [],
+ "rejected_filters": [
+ {"reason": "not_in_datasource", "column": "__time_col"}
+ ],
+ }
+ ]
+ }
+
+ assert (
+ rejected_requested_filter_columns(
+ result,
+ {"filters": [{"col": "__time_col", "op": "==", "val": "value"}]},
+ )
+ == []
+ )
+
+
def _collect_groupby_extras(
form_data: dict[str, Any],
groupby_columns: list[str],
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
index ed7b8fd7ccd..50bcfcf9272 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
@@ -1552,3 +1552,105 @@ class TestGetChartSqlTool:
)
assert result is None
+
+
+class TestRejectedFilterColumnsAreSurfaced:
+ """A filter naming a column the dataset does not have is dropped during
+ query construction. get_chart_sql must not return the resulting unfiltered
+ SQL as a success, which would misrepresent it as the SQL for the requested
+ filters."""
+
+ BAD_FILTER = {
+ "clause": "WHERE",
+ "expressionType": "SIMPLE",
+ "subject": "does_not_exist",
+ "operator": "==",
+ "comparator": "value",
+ }
+
+ def _result(self, rejected_columns):
+ return {
+ "queries": [
+ {
+ "query": "SELECT country, count(*) FROM sales GROUP BY
country",
+ "language": "sql",
+ "rejected_filters": [
+ {
+ "reason": "COL_NOT_IN_DATASOURCE",
+ "column": column,
+ }
+ for column in rejected_columns
+ ],
+ }
+ ]
+ }
+
+ def test_rejected_request_filter_returns_validation_error(self):
+ from superset.mcp_service.chart.tool.get_chart_sql import (
+ _extract_sql_from_result,
+ )
+
+ result = _extract_sql_from_result(
+ self._result(["does_not_exist"]),
+ chart_id=1,
+ chart_name="Sales",
+ datasource_name="sales",
+ extra_form_data={"adhoc_filters": [self.BAD_FILTER]},
+ )
+
+ assert isinstance(result, ChartError)
+ assert result.error_type == "ValidationError"
+ assert "does_not_exist" in result.error
+
+ def test_rejected_filter_not_requested_by_caller_is_ignored(self):
+ """A stale filter saved on the chart must not fail the request."""
+ from superset.mcp_service.chart.tool.get_chart_sql import (
+ _extract_sql_from_result,
+ )
+
+ result = _extract_sql_from_result(
+ self._result(["stale_saved_filter"]),
+ chart_id=1,
+ chart_name="Sales",
+ datasource_name="sales",
+ extra_form_data={"filters": [{"col": "country", "op": "==", "val":
"US"}]},
+ )
+
+ assert isinstance(result, ChartSql)
+ assert result.sql.startswith("SELECT country")
+
+ def test_temporal_rejection_does_not_match_same_named_request_filter(self):
+ """Temporal pseudo-filters and datasource columns have separate
origins."""
+ from superset.mcp_service.chart.tool.get_chart_sql import (
+ _extract_sql_from_result,
+ )
+
+ result = self._result(["__time_col"])
+ result["queries"][0]["rejected_filter_columns"] = []
+
+ extracted = _extract_sql_from_result(
+ result,
+ chart_id=1,
+ chart_name="Sales",
+ datasource_name="sales",
+ extra_form_data={
+ "filters": [{"col": "__time_col", "op": "==", "val": "value"}]
+ },
+ )
+
+ assert isinstance(extracted, ChartSql)
+
+ def test_no_rejections_returns_sql(self):
+ from superset.mcp_service.chart.tool.get_chart_sql import (
+ _extract_sql_from_result,
+ )
+
+ result = _extract_sql_from_result(
+ self._result([]),
+ chart_id=1,
+ chart_name="Sales",
+ datasource_name="sales",
+ extra_form_data={"adhoc_filters": [self.BAD_FILTER]},
+ )
+
+ assert isinstance(result, ChartSql)
