tien238lnd commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4089225863
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -790,7 +791,7 @@ def sanitize_dashboard_title(cls, v: str | None) -> str |
None:
)
-class UpdateDashboardRequest(BaseModel):
+class UpdateDashboardRequest(OmittedMeansUnchanged):
Review Comment:
You are right, and it changes what this PR claims rather than what it does.
`update_dashboard` applies every field behind `if request.<field> is not None`,
so an explicit null is ignored there, not honoured — there is no clearing
semantics to preserve.
I reworded the base-class docstring so it no longer promises uniform
clearing: an omitted field is still unset, and an explicit null keeps whatever
meaning the tool already gave it. `update_dashboard` keeps the base anyway,
since the advertised default is a value none of its branches would honour. The
model that actually reads `model_fields_set` on the chart side is
`BaseChartConfig`, one level below `UpdateChartRequest`, so it now carries the
base too.
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +249,46 @@ 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, passing null clears it. Their
+# optional fields must not advertise a default, or a client that materialises
+# defaults turns every call into a clear of everything it did not mention.
+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", {})
+ reference = request.get("$ref", "")
+ if reference.startswith("#/$defs/"):
+ return schema.get("$defs", {}).get(reference.split("/")[-1], {})
+ return request
Review Comment:
Good catch. The helper now follows a `$ref` out of `allOf` as well, looks in
both `$defs` and `definitions`, and calls `pytest.fail` when the reference
cannot be resolved or the request schema has no `properties`, so an
unrecognised shape reds the test instead of passing silently.
There is also a second test that does not depend on the schema shape at all:
`test_omitted_means_unchanged_models_advertise_no_null_default` walks every
subclass of `OmittedMeansUnchanged` and checks `model_json_schema()` directly,
including models added later.
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +249,46 @@ 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, passing null clears it. Their
+# optional fields must not advertise a default, or a client that materialises
+# defaults turns every call into a clear of everything it did not mention.
+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", {})
+ reference = request.get("$ref", "")
+ if reference.startswith("#/$defs/"):
+ return schema.get("$defs", {}).get(reference.split("/")[-1], {})
+ return request
+
+
+def test_partial_update_tools_advertise_no_null_default():
+ """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:
Review Comment:
Done: `registered.get(name)` with a `pytest.fail` that names the tool, so a
disabled or renamed tool reports absence instead of a bare `KeyError`.
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -25,15 +25,40 @@
from __future__ import annotations
import logging
-from typing import Any, Callable, List, Type, TypeVar
+from typing import Any, Callable, Dict, List, Type, TypeVar
Review Comment:
Dropped: the annotation is `dict[str, Any]` and `Dict` is gone from the
imports — it was the only use left in the module.
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -249,6 +249,46 @@ 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, passing null clears it. Their
+# optional fields must not advertise a default, or a client that materialises
+# defaults turns every call into a clear of everything it did not mention.
+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", {})
+ reference = request.get("$ref", "")
+ if reference.startswith("#/$defs/"):
+ return schema.get("$defs", {}).get(reference.split("/")[-1], {})
+ return request
+
+
+def test_partial_update_tools_advertise_no_null_default():
Review Comment:
Added `-> None` on both new tests.
--
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]
