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 74eb298eb68 fix(mcp): preserve chart form data on update (#43679)
74eb298eb68 is described below
commit 74eb298eb6860386828c093b5fd4d3ec732b388c
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Wed Sep 9 17:43:04 2026 -0400
fix(mcp): preserve chart form data on update (#43679)
Co-authored-by: Claude Opus 5 <[email protected]>
---
superset/mcp_service/chart/chart_utils.py | 73 ++++-
superset/mcp_service/chart/tool/update_chart.py | 167 +++++++++++-
.../mcp_service/chart/tool/test_update_chart.py | 303 ++++++++++++++++++++-
.../chart/tool/test_update_chart_preview.py | 287 +++++++++++++++++++
4 files changed, 821 insertions(+), 9 deletions(-)
diff --git a/superset/mcp_service/chart/chart_utils.py
b/superset/mcp_service/chart/chart_utils.py
index d8ad21141a5..bc3de192924 100644
--- a/superset/mcp_service/chart/chart_utils.py
+++ b/superset/mcp_service/chart/chart_utils.py
@@ -689,9 +689,20 @@ def merge_chart_form_data( # noqa: C901
if not isinstance(config, GaugeChartConfig):
if dataset_rebind:
return dict(new_form_data)
+ fields_set = config.model_fields_set
+ if "filters" not in fields_set:
+ preserve_previous_adhoc_filters(new_form_data, existing_form_data)
merged = {**existing_form_data, **new_form_data}
- if getattr(config, "filters", None) == []:
- merged.pop("adhoc_filters", None)
+ # An explicitly empty collection clears the control rather than
+ # falling through to the inherited value.
+ for config_field, form_data_field in (
+ ("filters", "adhoc_filters"),
+ ("group_by", "groupby"),
+ ("group_by_secondary", "groupby_b"),
+ ("sort_by", "order_by_cols"),
+ ):
+ if config_field in fields_set and getattr(config, config_field,
None) == []:
+ merged.pop(form_data_field, None)
return merged
fields_set = config.model_fields_set
@@ -2011,3 +2022,61 @@ def analyze_chart_semantics(viz_type: str | None,
config: Any) -> ChartSemantics
anomalies=[], # Would need actual data analysis to populate
statistical_summary={}, # Would need actual data analysis to populate
)
+
+
+def preserve_previous_adhoc_filters(
+ new_form_data: dict[str, Any], previous_form_data: dict[str, Any]
+) -> None:
+ """Preserve saved filters without dropping mapper-generated bindings.
+
+ Saved predicates the caller did not mention survive the update, while the
+ bindings generated for the new config are appended when they are not
+ already represented. A stale temporal binding is dropped when the config
+ rebinds the time filter to a different subject.
+ """
+ previous_filters = previous_form_data.get("adhoc_filters")
+ if not isinstance(previous_filters, list) or not previous_filters:
+ return
+
+ generated_filters = new_form_data.get("adhoc_filters", [])
+ previous_binding =
previous_form_data.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
+ new_binding = new_form_data.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
+ merged_filters = [
+ filter_
+ for filter_ in previous_filters
+ if not (
+ previous_binding
+ and previous_binding != new_binding
+ and isinstance(filter_, dict)
+ and filter_.get("operator") == FilterOperator.TEMPORAL_RANGE.value
+ and filter_.get("subject") == previous_binding
+ and filter_.get("comparator") == NO_TIME_RANGE
+ )
+ ]
+ for generated_filter in generated_filters:
+ if not isinstance(generated_filter, dict):
+ if generated_filter not in merged_filters:
+ merged_filters.append(generated_filter)
+ continue
+
+ # A saved temporal predicate on the same subject wins over the
+ # generated default, so only the comparator-insensitive match is
+ # treated as already represented for TEMPORAL_RANGE.
+ is_same_filter = any(
+ isinstance(previous_filter, dict)
+ and previous_filter.get("clause") == generated_filter.get("clause")
+ and previous_filter.get("expressionType")
+ == generated_filter.get("expressionType")
+ and previous_filter.get("subject") ==
generated_filter.get("subject")
+ and previous_filter.get("operator") ==
generated_filter.get("operator")
+ and (
+ generated_filter.get("operator") ==
FilterOperator.TEMPORAL_RANGE.value
+ or previous_filter.get("comparator")
+ == generated_filter.get("comparator")
+ )
+ for previous_filter in merged_filters
+ )
+ if not is_same_filter:
+ merged_filters.append(generated_filter)
+
+ new_form_data["adhoc_filters"] = merged_filters
diff --git a/superset/mcp_service/chart/tool/update_chart.py
b/superset/mcp_service/chart/tool/update_chart.py
index 0bc76fb6ee6..b011895f120 100644
--- a/superset/mcp_service/chart/tool/update_chart.py
+++ b/superset/mcp_service/chart/tool/update_chart.py
@@ -49,6 +49,7 @@ from superset.mcp_service.chart.schemas import (
AccessibilityMetadata,
ChartConfig,
ColumnRef,
+ GaugeChartConfig,
GenerateChartResponse,
PerformanceMetadata,
TableChartConfig,
@@ -211,6 +212,151 @@ def _merge_replacement_config(
)
+def _valid_dataset_reference(
+ value: Any,
+ columns: set[str],
+ metrics: set[str],
+ *,
+ allow_metric: bool = False,
+) -> bool:
+ """Return whether a form_data reference resolves against a dataset."""
+ if not isinstance(value, str):
+ return True
+ normalized = value.casefold()
+ return normalized in columns or (allow_metric and normalized in metrics)
+
+
+def _inherited_metrics_match_dataset(
+ existing_form_data: dict[str, Any],
+ columns: set[str],
+ metrics: set[str],
+) -> bool:
+ for metric in existing_form_data.get("metrics") or []:
+ if isinstance(metric, str) and not _valid_dataset_reference(
+ metric, columns, metrics, allow_metric=True
+ ):
+ return False
+ if isinstance(metric, dict):
+ column = metric.get("column")
+ if isinstance(column, dict) and not _valid_dataset_reference(
+ column.get("column_name"), columns, metrics
+ ):
+ return False
+ return True
+
+
+def _inherited_sort_matches_dataset(
+ order_by_cols: Any, columns: set[str], metrics: set[str]
+) -> bool:
+ for order_by in order_by_cols or []:
+ try:
+ column = json.loads(order_by)[0]
+ except (TypeError, ValueError, IndexError):
+ return False
+ if not _valid_dataset_reference(column, columns, metrics,
allow_metric=True):
+ return False
+ return True
+
+
+def _inherited_filters_match_dataset(
+ filters: Any, columns: set[str], metrics: set[str]
+) -> bool:
+ for filter_ in filters or []:
+ if not isinstance(filter_, dict):
+ return False
+ if filter_.get("expressionType") not in (None, "SIMPLE"):
+ return False
+ subject = filter_.get("subject") or filter_.get("col")
+ allow_metric = str(filter_.get("clause", "WHERE")).upper() == "HAVING"
+ if not _valid_dataset_reference(
+ subject, columns, metrics, allow_metric=allow_metric
+ ):
+ return False
+ return True
+
+
+#: form_data keys carrying query roles, mapped to the config field that
+#: sets them explicitly. An explicit field is the caller's stated intent,
+#: so it is never treated as inherited state.
+_INHERITED_QUERY_ROLE_FIELDS = {
+ "groupby": "group_by",
+ "groupby_b": "group_by_secondary",
+ "all_columns": None,
+ "columns": None,
+ "x_axis": None,
+ "granularity_sqla": None,
+ "metrics": None,
+ "order_by_cols": "sort_by",
+ "adhoc_filters": "filters",
+}
+
+_INHERITED_COLUMN_LIST_KEYS = frozenset(
+ {"groupby", "groupby_b", "all_columns", "columns"}
+)
+_INHERITED_COLUMN_SCALAR_KEYS = frozenset({"x_axis", "granularity_sqla"})
+
+
+def _inherited_state_invalid_keys(
+ existing_form_data: dict[str, Any],
+ new_form_data: dict[str, Any],
+ parsed_config: ChartConfig,
+ dataset_id: int,
+) -> set[str]:
+ """Return inherited query fields that are invalid for a new dataset.
+
+ A dataset rebind only has to discard the state that cannot resolve
+ against the replacement dataset; everything else stays valid and is
+ preserved so the update does not silently reset the chart.
+ """
+ fields_set = parsed_config.model_fields_set
+ inherited_keys = {
+ key
+ for key, config_field in _INHERITED_QUERY_ROLE_FIELDS.items()
+ if key not in new_form_data
+ and config_field not in fields_set
+ and existing_form_data.get(key)
+ }
+ if not inherited_keys:
+ return set()
+
+ from superset.daos.dataset import DatasetDAO
+ from superset.mcp_service.chart.validation.dataset_validator import (
+ build_dataset_context_from_orm,
+ )
+
+ context = build_dataset_context_from_orm(DatasetDAO.find_by_id(dataset_id))
+ if context is None:
+ # The replacement dataset cannot be inspected, so no inherited
+ # reference can be shown to be safe.
+ return inherited_keys
+ columns = {column["name"].casefold() for column in
context.available_columns}
+ metrics = {metric["name"].casefold() for metric in
context.available_metrics}
+
+ invalid_keys: set[str] = set()
+ for key in inherited_keys & _INHERITED_COLUMN_LIST_KEYS:
+ values = existing_form_data.get(key)
+ if isinstance(values, list) and not all(
+ _valid_dataset_reference(value, columns, metrics) for value in
values
+ ):
+ invalid_keys.add(key)
+ for key in inherited_keys & _INHERITED_COLUMN_SCALAR_KEYS:
+ if not _valid_dataset_reference(existing_form_data.get(key), columns,
metrics):
+ invalid_keys.add(key)
+ if "metrics" in inherited_keys and not _inherited_metrics_match_dataset(
+ existing_form_data, columns, metrics
+ ):
+ invalid_keys.add("metrics")
+ if "order_by_cols" in inherited_keys and not
_inherited_sort_matches_dataset(
+ existing_form_data.get("order_by_cols"), columns, metrics
+ ):
+ invalid_keys.add("order_by_cols")
+ if "adhoc_filters" in inherited_keys and not
_inherited_filters_match_dataset(
+ existing_form_data.get("adhoc_filters"), columns, metrics
+ ):
+ invalid_keys.add("adhoc_filters")
+ return invalid_keys
+
+
def _build_replacement_form_data(
existing_form_data: dict[str, Any],
parsed_config: ChartConfig,
@@ -222,13 +368,32 @@ def _build_replacement_form_data(
parsed_config, dataset_id=effective_dataset_id
)
new_form_data.pop("_mcp_warnings", None)
+ dataset_rebind = replacement_dataset_id is not None
+ if replacement_dataset_id is not None and not isinstance(
+ parsed_config, GaugeChartConfig
+ ):
+ # Drop only the inherited state the replacement dataset cannot
+ # resolve, then merge as a same-dataset update. Gauge keeps the
+ # stricter presentation-only rebind handled downstream.
+ invalid_keys = _inherited_state_invalid_keys(
+ existing_form_data,
+ new_form_data,
+ parsed_config,
+ replacement_dataset_id,
+ )
+ existing_form_data = {
+ key: value
+ for key, value in existing_form_data.items()
+ if key not in invalid_keys
+ }
+ dataset_rebind = False
merge_table_column_config(existing_form_data, new_form_data)
merge_interactive_pivot_ui_config(existing_form_data, new_form_data)
merged = _merge_replacement_config(
existing_form_data,
new_form_data,
parsed_config,
- dataset_rebind=replacement_dataset_id is not None,
+ dataset_rebind=dataset_rebind,
)
if replacement_dataset_id is not None:
merged["datasource"] = f"{replacement_dataset_id}__table"
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
index 254066711f7..7ea2aafa9c5 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
@@ -37,6 +37,7 @@ from superset.mcp_service.chart.schemas import (
GaugeChartConfig,
GenerateChartResponse,
LegendConfig,
+ MixedTimeseriesChartConfig,
TableChartConfig,
UpdateChartRequest,
XYChartConfig,
@@ -44,6 +45,7 @@ from superset.mcp_service.chart.schemas import (
from superset.mcp_service.chart.tool.update_chart import (
_build_preview_form_data,
_build_update_payload,
+ _inherited_state_invalid_keys,
)
from superset.utils import json
@@ -739,6 +741,288 @@ class TestBuildUpdatePayload:
# query_context must be cleared so get_chart_data uses updated params
assert result["query_context"] is None
+ def test_config_update_preserves_unrelated_mixed_timeseries_settings(self):
+ """Save payload retains settings outside the simplified config
schema."""
+ config = MixedTimeseriesChartConfig(
+ x=ColumnRef(name="ds"),
+ y=[ColumnRef(name="new_primary", aggregate="SUM")],
+ y_secondary=[ColumnRef(name="new_secondary", aggregate="SUM")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Year-over-year metrics",
+ params=json.dumps(
+ {
+ "viz_type": "mixed_timeseries",
+ "time_compare": ["1 year ago"],
+ "comparison_type_b": "percentage",
+ "y_axis_format": ",.2f",
+ }
+ ),
+ )
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ saved_form_data = json.loads(result["params"])
+ assert saved_form_data["time_compare"] == ["1 year ago"]
+ assert saved_form_data["comparison_type_b"] == "percentage"
+ assert saved_form_data["y_axis_format"] == ",.2f"
+
+ @patch(
+ "superset.mcp_service.chart.chart_utils.is_column_truly_temporal",
+ return_value=True,
+ )
+ def test_temporal_update_preserves_omitted_non_temporal_filters(
+ self, unused_temporal_mock
+ ) -> None:
+ """Generated time bindings do not replace omitted saved predicates."""
+ country_filter = {
+ "clause": "WHERE",
+ "comparator": "US",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "country",
+ }
+ temporal_filter = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ config = XYChartConfig(
+ x=ColumnRef(name="ds"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Revenue",
+ params=json.dumps(
+ {
+ "viz_type": "echarts_timeseries_line",
+ "adhoc_filters": [country_filter, temporal_filter],
+ "_mcp_dashboard_time_filter_subject": "ds",
+ }
+ ),
+ )
+
+ saved = _build_update_payload(request, chart, parsed_config=config)
+ preview = _build_preview_form_data(request, chart,
parsed_config=config)
+
+ assert isinstance(saved, dict)
+ assert isinstance(preview, dict)
+ saved_filters = json.loads(saved["params"])["adhoc_filters"]
+ assert saved_filters == [country_filter, temporal_filter]
+ assert preview["adhoc_filters"] == saved_filters
+
+ def test_explicit_empty_group_by_clears_save_and_preview(self) -> None:
+ config = XYChartConfig.model_validate(
+ {
+ "x": {"name": "ds"},
+ "y": [{"name": "revenue", "aggregate": "SUM"}],
+ "groupby": [],
+ }
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Revenue by region",
+ params=json.dumps(
+ {
+ "viz_type": "echarts_timeseries_line",
+ "groupby": ["region"],
+ }
+ ),
+ )
+
+ saved = _build_update_payload(request, chart, parsed_config=config)
+ preview = _build_preview_form_data(request, chart,
parsed_config=config)
+
+ assert isinstance(saved, dict)
+ assert isinstance(preview, dict)
+ assert "groupby" not in json.loads(saved["params"])
+ assert "groupby" not in preview
+
+ def test_explicit_empty_sort_by_clears_save_and_preview(self) -> None:
+ config = TableChartConfig.model_validate(
+ {
+ "columns": [{"name": "region"}],
+ "order_by_cols": [],
+ }
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Regions",
+ params=json.dumps(
+ {
+ "viz_type": "table",
+ "order_by_cols": ['["region", false]'],
+ }
+ ),
+ )
+
+ saved = _build_update_payload(request, chart, parsed_config=config)
+ preview = _build_preview_form_data(request, chart,
parsed_config=config)
+
+ assert isinstance(saved, dict)
+ assert isinstance(preview, dict)
+ assert "order_by_cols" not in json.loads(saved["params"])
+ assert "order_by_cols" not in preview
+
+ @patch.object(update_chart_module, "_inherited_state_invalid_keys")
+ def test_dataset_rebind_only_preserves_compatible_state(
+ self, mock_invalid_keys
+ ) -> None:
+ config = TableChartConfig(columns=[ColumnRef(name="revenue")])
+ request = UpdateChartRequest(identifier=1, config=config, dataset_id=9)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Revenue",
+ params=json.dumps(
+ {
+ "viz_type": "table",
+ "groupby": ["removed_column"],
+ "custom_flag": True,
+ }
+ ),
+ )
+
+ mock_invalid_keys.return_value = {"groupby"}
+ incompatible = _build_update_payload(request, chart,
parsed_config=config)
+ mock_invalid_keys.return_value = set()
+ compatible = _build_update_payload(request, chart,
parsed_config=config)
+
+ assert isinstance(incompatible, dict)
+ assert isinstance(compatible, dict)
+ incompatible_params = json.loads(incompatible["params"])
+ compatible_params = json.loads(compatible["params"])
+ assert "groupby" not in incompatible_params
+ assert incompatible_params["custom_flag"] is True
+ assert compatible_params["groupby"] == ["removed_column"]
+ assert compatible_params["custom_flag"] is True
+ assert incompatible_params["datasource"] == "9__table"
+ assert compatible_params["datasource"] == "9__table"
+
+ @patch(
+ "superset.mcp_service.chart.validation.dataset_validator."
+ "build_dataset_context_from_orm"
+ )
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id")
+ def test_dataset_rebind_explicit_clear_preserves_other_state(
+ self, mock_find_dataset, mock_build_context
+ ) -> None:
+ mock_find_dataset.return_value = Mock()
+ mock_build_context.return_value = Mock(
+ available_columns=[{"name": "revenue"}],
+ available_metrics=[],
+ )
+ config = XYChartConfig.model_validate(
+ {
+ "x": {"name": "revenue"},
+ "y": [{"name": "revenue", "aggregate": "SUM"}],
+ "groupby": [],
+ }
+ )
+ request = UpdateChartRequest(identifier=1, config=config, dataset_id=9)
+ chart = Mock(
+ datasource_id=7,
+ slice_name="Revenue",
+ params=json.dumps(
+ {
+ "viz_type": "echarts_timeseries_line",
+ "groupby": ["removed_column"],
+ "custom_flag": True,
+ }
+ ),
+ )
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ params = json.loads(result["params"])
+ assert "groupby" not in params
+ assert params["custom_flag"] is True
+
+ @patch(
+ "superset.mcp_service.chart.validation.dataset_validator."
+ "build_dataset_context_from_orm"
+ )
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id")
+ def test_dataset_rebind_compatibility_uses_inherited_references(
+ self, mock_find_dataset, mock_build_context
+ ) -> None:
+ mock_find_dataset.return_value = Mock()
+ mock_build_context.return_value = Mock(
+ available_columns=[{"name": "revenue"}, {"name": "region"}],
+ available_metrics=[],
+ )
+ config = TableChartConfig(columns=[ColumnRef(name="revenue")])
+ new_form_data = {"viz_type": "table", "all_columns": ["revenue"]}
+
+ assert not _inherited_state_invalid_keys(
+ {
+ "viz_type": "table",
+ "groupby": ["region"],
+ "adhoc_filters": [
+ {
+ "expressionType": "SIMPLE",
+ "subject": "region",
+ "operator": "==",
+ "comparator": "US",
+ }
+ ],
+ },
+ new_form_data,
+ config,
+ 9,
+ )
+ assert _inherited_state_invalid_keys(
+ {
+ "viz_type": "table",
+ "groupby": ["removed_column"],
+ },
+ new_form_data,
+ config,
+ 9,
+ ) == {"groupby"}
+
+ def test_config_update_does_not_merge_settings_from_another_viz_type(self):
+ """Changing visualization types drops stale query-defining settings."""
+ config = XYChartConfig(
+ x=ColumnRef(name="ds"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ )
+ request = UpdateChartRequest(identifier=1, config=config)
+ chart = Mock(
+ id=1,
+ datasource_id=7,
+ slice_name="Raw records",
+ params=json.dumps(
+ {
+ "viz_type": "table",
+ "query_mode": "raw",
+ "all_columns": ["ds", "revenue"],
+ }
+ ),
+ )
+
+ result = _build_update_payload(request, chart, parsed_config=config)
+
+ assert isinstance(result, dict)
+ saved_form_data = json.loads(result["params"])
+ assert saved_form_data["viz_type"] == "echarts_timeseries_line"
+ assert "query_mode" not in saved_form_data
+ assert "all_columns" not in saved_form_data
+
def test_add_columns_preserves_existing_columns_and_metrics(self):
"""An additive update does not require reconstructing the table."""
request = UpdateChartRequest(
@@ -1160,7 +1444,9 @@ class TestUpdateChartPreviewFirst:
mock_chart.slice_name = "Existing Chart"
mock_chart.viz_type = "table"
mock_chart.uuid = "abc-123"
- mock_chart.params = '{"viz_type": "table", "datasource": "10__table"}'
+ mock_chart.params = (
+ '{"viz_type": "table", "datasource": "10__table", "custom_flag":
true}'
+ )
mock_find_by_id.return_value = mock_chart
mock_check_access.return_value = DatasetValidationResult(
@@ -1196,6 +1482,7 @@ class TestUpdateChartPreviewFirst:
# Ensure the chart was NOT persisted
mock_update_cmd_cls.assert_not_called()
mock_create_preview.assert_called_once()
+ assert mock_create_preview.call_args.args[1]["custom_flag"] is True
@patch.object(update_chart_module, "_create_preview_url",
new_callable=Mock)
@patch(
@@ -1252,13 +1539,15 @@ class TestBuildPreviewFormData:
chart.id = 42
chart.datasource_id = 7
chart.slice_name = "Existing"
- chart.params = '{"viz_type": "line", "custom_flag": true}'
+ chart.params = '{"viz_type": "table", "custom_flag": true}'
result = _build_preview_form_data(request, chart, parsed_config=config)
assert isinstance(result, dict)
- # Cross-viz updates do not inherit stale controls from the prior
plugin.
- assert "custom_flag" not in result
+ # A same-viz update keeps controls the config does not mention.
+ # Cross-viz drops are covered by
+ # test_config_update_does_not_merge_settings_from_another_viz_type.
+ assert result["custom_flag"] is True
# New config overrides existing keys
assert result["viz_type"] == "table"
# slice_id and datasource are always stamped onto the preview
@@ -1560,7 +1849,7 @@ class TestUpdateChartSaveWithConfig:
mock_chart.slice_name = "Pre-save"
mock_chart.viz_type = "table"
mock_chart.uuid = "uuid-77"
- mock_chart.params = '{"viz_type": "table"}'
+ mock_chart.params = '{"viz_type": "table", "custom_flag": true}'
mock_find_by_id.return_value = mock_chart
mock_check_access.return_value = DatasetValidationResult(
@@ -1600,6 +1889,7 @@ class TestUpdateChartSaveWithConfig:
# Verify query_context is cleared so get_chart_data uses updated params
payload = mock_update_cmd_cls.call_args[0][1]
assert payload["query_context"] is None
+ assert json.loads(payload["params"])["custom_flag"] is True
# Verify form_data is returned in the response
form_data = result.structured_content["form_data"]
@@ -2214,7 +2504,7 @@ class TestBuildUpdatePayloadDatasetId:
columns=[ColumnRef(name="col1")],
)
request = UpdateChartRequest(identifier=1, config=config,
dataset_id=99)
- chart = Mock()
+ chart = Mock(params='{"viz_type":"table","datasource":"10__table"}')
chart.datasource_id = 10
chart.slice_name = "Old Name"
@@ -2225,6 +2515,7 @@ class TestBuildUpdatePayloadDatasetId:
assert result["datasource_type"] == "table"
assert "params" in result
assert "viz_type" in result
+ assert json.loads(result["params"])["datasource"] == "99__table"
def test_config_without_dataset_does_not_include_datasource(self):
"""When dataset_id is None, payload must NOT include datasource_id."""
diff --git
a/tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
b/tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
index cc04860eb82..e7306bb993a 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
@@ -21,6 +21,7 @@ Unit tests for update_chart_preview MCP tool
import importlib
from contextlib import nullcontext
+from typing import Any
from unittest.mock import Mock, patch
import pytest
@@ -28,8 +29,13 @@ from fastmcp import Client
from superset.extensions import feature_flag_manager
from superset.mcp_service.app import mcp
+from superset.mcp_service.chart.chart_utils import (
+ map_big_number_config,
+ preserve_previous_adhoc_filters,
+)
from superset.mcp_service.chart.schemas import (
AxisConfig,
+ BigNumberChartConfig,
ColumnRef,
FilterConfig,
GaugeChartConfig,
@@ -683,6 +689,287 @@ class TestUpdateChartPreview:
assert result is None
+ def test_preserves_generated_temporal_filter_with_cached_filters(self) ->
None:
+ """Cached filters are merged without replacing the temporal binding."""
+ new_form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ ]
+ }
+ previous_form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "North",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "region",
+ }
+ ]
+ }
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ previous_form_data,
+ )
+
+ assert [filter_["subject"] for filter_ in
new_form_data["adhoc_filters"]] == [
+ "region",
+ "ds",
+ ]
+
+ def test_cached_temporal_filter_takes_precedence_over_generated_default(
+ self,
+ ) -> None:
+ """A cached chart-specific time range is not duplicated or reset."""
+ new_form_data = {
+ "adhoc_filters": [
+ {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ ]
+ }
+ cached_temporal_filter = {
+ "clause": "WHERE",
+ "comparator": "Last month",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {"adhoc_filters": [cached_temporal_filter]},
+ )
+
+ assert new_form_data["adhoc_filters"] == [cached_temporal_filter]
+
+ def test_preserves_filters_with_distinct_comparators(self) -> None:
+ """Filters that differ only by value are not deduplicated."""
+ previous_filter = {
+ "clause": "WHERE",
+ "comparator": 2020,
+ "expressionType": "SIMPLE",
+ "operator": ">",
+ "subject": "year",
+ }
+ generated_filter = {**previous_filter, "comparator": 2021}
+ new_form_data = {"adhoc_filters": [generated_filter]}
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {"adhoc_filters": [previous_filter]},
+ )
+
+ assert new_form_data["adhoc_filters"] == [previous_filter,
generated_filter]
+
+ def test_replaces_cached_temporal_filter_when_column_changes(self) -> None:
+ """A newly selected temporal column replaces the cached binding."""
+ new_temporal_filter = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ }
+ region_filter = {
+ "clause": "WHERE",
+ "comparator": "North",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "region",
+ }
+ previous_temporal_filter = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ new_form_data: dict[str, Any] = {"adhoc_filters":
[new_temporal_filter]}
+ new_form_data["_mcp_dashboard_time_filter_subject"] = "created_at"
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {
+ "adhoc_filters": [region_filter, previous_temporal_filter],
+ "_mcp_dashboard_time_filter_subject": "ds",
+ },
+ )
+
+ assert new_form_data["adhoc_filters"] == [
+ region_filter,
+ new_temporal_filter,
+ ]
+
+ def test_replaces_temporal_xy_binding_when_subject_changes(self) -> None:
+ """A temporal XY binding does not survive rebinding to a new
subject."""
+ previous_binding = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "event_time",
+ }
+ new_binding = {
+ **previous_binding,
+ "subject": "created_at",
+ }
+ new_form_data = {
+ "adhoc_filters": [new_binding],
+ "_mcp_dashboard_time_filter_subject": "created_at",
+ }
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {
+ "adhoc_filters": [previous_binding],
+ "_mcp_dashboard_time_filter_subject": "event_time",
+ },
+ )
+
+ assert new_form_data["adhoc_filters"] == [new_binding]
+
+ def test_replaces_big_number_fallback_binding_when_subject_changes(self)
-> None:
+ """A Big Number fallback binding is replaced by a selected subject."""
+ dataset = Mock(
+ main_dttm_col=None,
+ columns=[Mock(column_name="order_date")],
+ )
+ config = BigNumberChartConfig(
+ chart_type="big_number",
+ metric=ColumnRef(name="revenue", aggregate="SUM"),
+ )
+ rebound_config = config.model_copy(update={"temporal_column":
"created_at"})
+
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.mcp_service.chart.chart_utils.is_column_truly_temporal",
+ return_value=True,
+ ),
+ ):
+ previous_form_data = map_big_number_config(config, dataset_id=42)
+ new_form_data = map_big_number_config(rebound_config,
dataset_id=42)
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ previous_form_data,
+ )
+
+ assert previous_form_data["_mcp_dashboard_time_filter_subject"] ==
"order_date"
+ assert new_form_data["_mcp_dashboard_time_filter_subject"] ==
"created_at"
+ assert [filter_["subject"] for filter_ in
new_form_data["adhoc_filters"]] == [
+ "created_at"
+ ]
+
+ def test_removes_cached_temporal_filter_without_new_binding(self) -> None:
+ """A mapping without a temporal subject drops the cached binding."""
+ region_filter = {
+ "clause": "WHERE",
+ "comparator": "North",
+ "expressionType": "SIMPLE",
+ "operator": "==",
+ "subject": "region",
+ }
+ previous_temporal_filter = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ new_form_data: dict[str, Any] = {}
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {
+ "adhoc_filters": [region_filter, previous_temporal_filter],
+ "_mcp_dashboard_time_filter_subject": "ds",
+ },
+ )
+
+ assert new_form_data["adhoc_filters"] == [region_filter]
+
+ def test_preserves_user_temporal_filter_on_generated_subject(self) -> None:
+ """A user-authored range on the binding subject is not generated
state."""
+ generated_binding = {
+ "clause": "WHERE",
+ "comparator": "No filter",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+ user_filter = {
+ "clause": "WHERE",
+ "comparator": "Last month",
+ "expressionType": "SIMPLE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ }
+
+ new_form_data: dict[str, Any] = {}
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {
+ "adhoc_filters": [generated_binding, user_filter],
+ "_mcp_dashboard_time_filter_subject": "ds",
+ },
+ )
+
+ assert new_form_data["adhoc_filters"] == [user_filter]
+
+ def test_rebinding_preserves_unrelated_cached_temporal_filter(self) ->
None:
+ """Only the generated binding is replaced; user filters retain
provenance."""
+ previous_binding = {
+ "expressionType": "SIMPLE",
+ "clause": "WHERE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "ds",
+ "comparator": "No filter",
+ }
+ unrelated_filter = {
+ "expressionType": "SIMPLE",
+ "clause": "WHERE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "processed_at",
+ "comparator": "Last year",
+ }
+ new_binding = {
+ "expressionType": "SIMPLE",
+ "clause": "WHERE",
+ "operator": "TEMPORAL_RANGE",
+ "subject": "created_at",
+ "comparator": "No filter",
+ }
+ new_form_data = {
+ "adhoc_filters": [new_binding],
+ "_mcp_dashboard_time_filter_subject": "created_at",
+ }
+
+ preserve_previous_adhoc_filters(
+ new_form_data,
+ {
+ "adhoc_filters": [previous_binding, unrelated_filter],
+ "_mcp_dashboard_time_filter_subject": "ds",
+ },
+ )
+
+ assert new_form_data["adhoc_filters"] == [unrelated_filter,
new_binding]
+
@patch.object(update_chart_preview_module, "validate_and_compile")
@patch.object(update_chart_preview_module, "has_dataset_access",
return_value=True)
@patch("superset.daos.dataset.DatasetDAO.find_by_id")
