This is an automated email from the ASF dual-hosted git repository.
rusackas 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 10356c30edb fix(mcp): stop partial-update tools from advertising null
defaults (#44573)
10356c30edb is described below
commit 10356c30edb5bdfbe80199b3a99bb9a616a15130
Author: Nguyen Dang Trung Tien <[email protected]>
AuthorDate: Sat Oct 3 12:16:12 2026 +0700
fix(mcp): stop partial-update tools from advertising null defaults (#44573)
Co-authored-by: Claude Opus 5 <[email protected]>
---
superset/mcp_service/chart/schemas.py | 15 +--
superset/mcp_service/dashboard/schemas.py | 3 +-
superset/mcp_service/dataset/schemas.py | 7 +-
superset/mcp_service/utils/schema_utils.py | 27 +++++-
.../mcp_service/test_mcp_tool_registration.py | 106 +++++++++++++++++++++
5 files changed, 147 insertions(+), 11 deletions(-)
diff --git a/superset/mcp_service/chart/schemas.py
b/superset/mcp_service/chart/schemas.py
index 9b14c9ab8c2..c411b481912 100644
--- a/superset/mcp_service/chart/schemas.py
+++ b/superset/mcp_service/chart/schemas.py
@@ -78,6 +78,7 @@ from superset.mcp_service.utils.sanitization import (
sanitize_user_input,
sanitize_user_input_with_changes,
)
+from superset.mcp_service.utils.schema_utils import OmittedMeansUnchanged
from superset.mcp_service.utils.serialization import (
JsonSafeMapping,
JsonSafeRows,
@@ -799,7 +800,7 @@ class UnknownFieldCheckMixin(BaseModel):
return _check_unknown_fields(data, cls)
-class BaseChartConfig(UnknownFieldCheckMixin):
+class BaseChartConfig(UnknownFieldCheckMixin, OmittedMeansUnchanged):
"""Fields shared by every MCP chart configuration."""
temporal_column: str | None = Field(
@@ -826,7 +827,7 @@ class BaseChartConfig(UnknownFieldCheckMixin):
)
-class ColumnRef(UnknownFieldCheckMixin):
+class ColumnRef(UnknownFieldCheckMixin, OmittedMeansUnchanged):
model_config = ConfigDict(extra="ignore", populate_by_name=True)
name: str | None = Field(
@@ -952,7 +953,7 @@ class ColumnRef(UnknownFieldCheckMixin):
)
-class AxisConfig(UnknownFieldCheckMixin):
+class AxisConfig(UnknownFieldCheckMixin, OmittedMeansUnchanged):
model_config = ConfigDict(extra="ignore")
title: str | None = Field(None, max_length=200)
@@ -990,7 +991,7 @@ class CurrencyFormat(UnknownFieldCheckMixin):
LEGEND_POSITION_LITERAL = Literal["top", "bottom", "left", "right"]
-class FilterConfig(UnknownFieldCheckMixin):
+class FilterConfig(UnknownFieldCheckMixin, OmittedMeansUnchanged):
model_config = ConfigDict(extra="ignore", populate_by_name=True)
column: str = Field(
@@ -2359,7 +2360,7 @@ class BigNumberChartConfig(BaseChartConfig):
return self
-class TableColumnConfig(UnknownFieldCheckMixin):
+class TableColumnConfig(UnknownFieldCheckMixin, OmittedMeansUnchanged):
"""Display formatting supported by the MCP table-chart schema."""
model_config = ConfigDict(
@@ -4043,7 +4044,9 @@ class
GenerateExploreLinkRequest(ChartRequestNormalizerMixin, FormDataCacheContr
)
-class UpdateChartRequest(ChartRequestNormalizerMixin, QueryCacheControl):
+class UpdateChartRequest(
+ ChartRequestNormalizerMixin, OmittedMeansUnchanged, QueryCacheControl
+):
model_config = ConfigDict(populate_by_name=True)
identifier: int | str = Field(
diff --git a/superset/mcp_service/dashboard/schemas.py
b/superset/mcp_service/dashboard/schemas.py
index 6b5d0d419a0..e6d25586db9 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -122,6 +122,7 @@ from superset.mcp_service.utils.sanitization import (
sanitize_user_input,
sanitize_user_input_with_changes,
)
+from superset.mcp_service.utils.schema_utils import OmittedMeansUnchanged
from superset.mcp_service.utils.serialization import JsonSafeRows,
OptionalRowCount
from superset.mcp_service.utils.url_utils import get_superset_base_url
from superset.utils.core import DatasourceType
@@ -826,7 +827,7 @@ class GenerateDashboardRequest(BaseModel):
)
-class UpdateDashboardRequest(BaseModel):
+class UpdateDashboardRequest(OmittedMeansUnchanged):
"""Request schema for updating an existing dashboard's layout/theme/style.
All fields are optional; only the fields explicitly passed are applied.
diff --git a/superset/mcp_service/dataset/schemas.py
b/superset/mcp_service/dataset/schemas.py
index b0a9c999dea..12ec0bdcdb0 100644
--- a/superset/mcp_service/dataset/schemas.py
+++ b/superset/mcp_service/dataset/schemas.py
@@ -60,6 +60,7 @@ from superset.mcp_service.system.schemas import (
TagInfo,
)
from superset.mcp_service.utils.response_utils import humanize_timestamp
+from superset.mcp_service.utils.schema_utils import OmittedMeansUnchanged
from superset.mcp_service.utils.serialization import (
JsonSafeRows,
OptionalRowCount,
@@ -635,7 +636,7 @@ UPDATABLE_METRIC_FIELDS: frozenset[str] = frozenset(
)
-class MetricCurrency(BaseModel):
+class MetricCurrency(OmittedMeansUnchanged):
"""Currency formatting configuration for a metric."""
symbol: str | None = Field(
@@ -648,7 +649,7 @@ class MetricCurrency(BaseModel):
)
-class DatasetMetricProperties(BaseModel):
+class DatasetMetricProperties(OmittedMeansUnchanged):
"""Dataset identifier and writable saved-metric properties."""
model_config = ConfigDict(populate_by_name=True)
@@ -946,7 +947,7 @@ UPDATABLE_DATASET_FIELDS: frozenset[str] = frozenset(
)
-class UpdateDatasetRequest(BaseModel):
+class UpdateDatasetRequest(OmittedMeansUnchanged):
"""Request schema for update_dataset."""
model_config = ConfigDict(populate_by_name=True)
diff --git a/superset/mcp_service/utils/schema_utils.py
b/superset/mcp_service/utils/schema_utils.py
index 31f2e9c9e3c..e7c5955d632 100644
--- a/superset/mcp_service/utils/schema_utils.py
+++ b/superset/mcp_service/utils/schema_utils.py
@@ -27,13 +27,38 @@ from __future__ import annotations
import logging
from typing import Any, Callable, List, Type, TypeVar
-from pydantic import BaseModel, ValidationError
+from pydantic import BaseModel, GetJsonSchemaHandler, ValidationError
logger = logging.getLogger(__name__)
T = TypeVar("T")
+class OmittedMeansUnchanged(BaseModel):
+ """Base for models that tell an omitted field from an explicit ``null``.
+
+ These models read ``model_fields_set``, so leaving a field out is not the
+ same as passing ``null``. Pydantic advertises ``"default": null`` for every
+ optional field, and a client that materialises those defaults then sends
+ nulls the caller never named, which the model reads as deliberate input.
+
+ Dropping the advertised default keeps the fields optional without handing
+ clients a value to fill in. Nothing else changes: an omitted field is still
+ unset, and an explicit ``null`` still means whatever the tool already made
+ it mean.
+ """
+
+ @classmethod
+ def __get_pydantic_json_schema__(
+ cls, core_schema: Any, handler: GetJsonSchemaHandler
+ ) -> dict[str, Any]:
+ schema = handler(core_schema)
+ for field in schema.get("properties", {}).values():
+ if "default" in field and field["default"] is None:
+ del field["default"]
+ return schema
+
+
class JSONParseError(ValueError):
"""Raised when JSON parsing fails with helpful context."""
diff --git a/tests/unit_tests/mcp_service/test_mcp_tool_registration.py
b/tests/unit_tests/mcp_service/test_mcp_tool_registration.py
index 516e8a04220..42856765b08 100644
--- a/tests/unit_tests/mcp_service/test_mcp_tool_registration.py
+++ b/tests/unit_tests/mcp_service/test_mcp_tool_registration.py
@@ -26,6 +26,7 @@ import pytest
from superset.mcp_service.app import get_default_instructions,
init_fastmcp_server, mcp
from superset.mcp_service.utils.response_size_utils import
COMMITTED_WRITE_SPECS
+from superset.mcp_service.utils.schema_utils import OmittedMeansUnchanged
# Patch target for the feature_flag_manager imported inside
_apply_config_guards
_FFM_PATH = "superset.extensions.feature_flag_manager"
@@ -263,6 +264,111 @@ def _run(coro):
return asyncio.run(coro)
+# Tools whose request model tells "field omitted" from "field set to null":
+# omitting leaves the stored value alone, an explicit null is deliberate input.
+# Their optional fields must not advertise a default, or a client that
+# materialises defaults sends nulls for everything the caller never named.
+OMITTED_MEANS_UNCHANGED_TOOLS = (
+ "update_chart",
+ "update_dashboard",
+ "update_dataset",
+ "update_dataset_metric",
+)
+
+
+def _request_model_schema(tool: Any) -> dict[str, Any]:
+ """Return the JSON Schema of a tool's ``request`` argument."""
+ schema = tool.parameters or {}
+ request = schema.get("properties", {}).get("request", {})
+ for candidate in (request, *request.get("allOf", [])):
+ if reference := candidate.get("$ref"):
+ name = reference.rpartition("/")[2]
+ for container in ("$defs", "definitions"):
+ if name in schema.get(container, {}):
+ return schema[container][name]
+ raise AssertionError(
+ f"{tool.name}: cannot resolve request schema reference
{reference!r}"
+ )
+ if "properties" in request:
+ return request
+ raise AssertionError(
+ f"{tool.name}: unrecognised request schema shape {sorted(request)}; "
+ "the null-default check below would silently pass"
+ )
+
+
+def _null_defaults(schema: Any, path: str = "") -> list[str]:
+ """Return every field at or under ``schema`` that defaults to null.
+
+ The walk goes all the way down on purpose. A tool's advertised parameters
+ arrive fully inlined, while ``model_json_schema()`` puts nested models in
+ ``$defs``, and a null default is a hazard wherever it sits: the fields of
+ a nested model are merged into stored state the same way.
+ """
+ hits: list[str] = []
+ if isinstance(schema, dict):
+ for name, spec in (schema.get("properties") or {}).items():
+ if isinstance(spec, dict) and "default" in spec and
spec["default"] is None:
+ hits.append(f"{path}/{name}")
+ for key, value in schema.items():
+ hits.extend(_null_defaults(value, f"{path}/{key}"))
+ elif isinstance(schema, list):
+ for index, value in enumerate(schema):
+ hits.extend(_null_defaults(value, f"{path}/{index}"))
+ return sorted(hits)
+
+
+def _omitted_means_unchanged_models() -> list[type[OmittedMeansUnchanged]]:
+ """Return every model built on ``OmittedMeansUnchanged``."""
+ models: list[type[OmittedMeansUnchanged]] = []
+ pending: list[type[OmittedMeansUnchanged]] = [OmittedMeansUnchanged]
+ while pending:
+ for subclass in pending.pop().__subclasses__():
+ if subclass not in models:
+ models.append(subclass)
+ pending.append(subclass)
+ return models
+
+
+def test_partial_update_tools_advertise_no_null_default() -> None:
+ """No optional field of a partial-update tool offers null as its
default."""
+ registered = {tool.name: tool for tool in _run(mcp.list_tools())}
+ advertised = {}
+ for name in OMITTED_MEANS_UNCHANGED_TOOLS:
+ tool: Any = registered.get(name)
+ if tool is None:
+ raise AssertionError(
+ f"{name} is not registered, so its schema cannot be checked"
+ )
+ if offenders := _null_defaults(_request_model_schema(tool)):
+ advertised[name] = offenders
+
+ assert not advertised, (
+ "Partial-update tools must not advertise null defaults, or a client "
+ f"filling them clears values the caller never named: {advertised}"
+ )
+
+
+def test_omitted_means_unchanged_models_advertise_no_null_default() -> None:
+ """Every model on the base keeps null out of its advertised defaults.
+
+ ``update_chart`` reads ``model_fields_set`` on the nested chart config
+ rather than on the request, so the config models carry the base too. This
+ covers them, and any model that joins them later.
+ """
+ models = _omitted_means_unchanged_models()
+ assert models, "OmittedMeansUnchanged has no subclasses; is the import
stale?"
+
+ offenders = {
+ model.__name__: null_defaults
+ for model in models
+ if (null_defaults := _null_defaults(model.model_json_schema()))
+ }
+ assert not offenders, (
+ f"Models on OmittedMeansUnchanged must not advertise null defaults:
{offenders}"
+ )
+
+
def test_mcp_app_imports_successfully():
"""Test that the MCP app can be imported without errors."""
assert mcp is not None
