aminghadersohi commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4089348213
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -745,7 +746,7 @@ def check_unknown_fields(cls, data: Any) -> Any:
return _check_unknown_fields(data, cls)
-class BaseChartConfig(UnknownFieldCheckMixin):
+class BaseChartConfig(UnknownFieldCheckMixin, OmittedMeansUnchanged):
Review Comment:
`TableColumnConfig` (:2309) never took the base, yet `chart_utils.py:534`
dumps it `exclude_unset=True` into the per-key merge at `:575`. Measured at
head: with only `d3TimeFormat` named, a defaults-filling client nulls stored
`columnWidth`/`d3NumberFormat`; omitting keeps both. The base fixes it.
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +250,96 @@ 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_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]
+ pytest.fail(
+ f"{tool.name}: cannot resolve request schema reference
{reference!r}"
+ )
+ if "properties" in request:
+ return request
+ pytest.fail(
+ f"{tool.name}: unrecognised request schema shape {sorted(request)}; "
+ "the null-default check below would silently pass"
+ )
Review Comment:
`pre-commit (current)` fails here: the mypy hook env has no pytest, so
`pytest.fail` is `Any` not `NoReturn` and mypy reports `:264: Missing return
statement [return]`. Reproduced in an isolated env — this fence removes exactly
that error and adds none.
```suggestion
raise AssertionError(
f"{tool.name}: unrecognised request schema shape {sorted(request)}; "
"the null-default check below would silently pass"
)
```
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +250,96 @@ 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_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]
+ pytest.fail(
+ f"{tool.name}: cannot resolve request schema reference
{reference!r}"
+ )
+ if "properties" in request:
+ return request
+ pytest.fail(
+ f"{tool.name}: unrecognised request schema shape {sorted(request)}; "
+ "the null-default check below would silently pass"
+ )
+
+
+def _null_defaults(schema: dict[str, Any]) -> list[str]:
+ """Return the fields of one schema that advertise null as their default."""
+ return sorted(
+ field
+ for field, spec in schema.get("properties", {}).items()
Review Comment:
Both new tests read only the top-level `properties`, never `$defs`. At head
`update_chart` still publishes 269 null-default fields and
`update_dataset_metric` 2 (`MetricCurrency.symbol`/`symbolPosition`) with both
tests green — which is why the `TableColumnConfig` clobber is invisible here.
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -27,13 +27,38 @@
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():
Review Comment:
bito's CWE-409 note on this loop is wrong; please don't act on it. `del
field["default"]` mutates the inner per-field dict, so `properties` is never
resized. Verified: plain dict and defaultdict both iterate fine, while resizing
the outer mapping does raise. No change needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
