Copilot commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4081509019
##########
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:
The `$ref` dereference logic only handles the `#/$defs/...` form. If the
schema generator emits other common ref forms (e.g., `#/definitions/...`, or a
`request` schema wrapped in `allOf` with a `$ref`), this helper will silently
return an unresolved structure and the test may miss `default: null` offenders.
Consider extending the helper to resolve additional ref patterns (and/or
`allOf`-wrapped refs) or explicitly fail when the request schema shape is not
recognized.
##########
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:
This module already uses modern built-in generics elsewhere (e.g.,
`dict[str, Any]` in tests), and `from __future__ import annotations` is
enabled. For consistency, prefer `dict[str, Any]` over `Dict[str, Any]` and
drop `Dict` from the imports.
##########
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:
Indexing `registered[name]` will raise a raw `KeyError` if a tool is
renamed/removed, which makes the test failure less actionable. Consider
asserting presence first (e.g., `assert name in registered, ...`) so failures
clearly indicate registration issues vs. schema issues.
##########
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
-from pydantic import BaseModel, ValidationError
+from pydantic import BaseModel, GetJsonSchemaHandler, ValidationError
logger = logging.getLogger(__name__)
T = TypeVar("T")
+class OmittedMeansUnchanged(BaseModel):
+ """Base for update requests where an omitted field leaves the value alone.
+
+ These requests tell "not provided" from an explicit ``null`` through
+ ``model_fields_set``: the first leaves the stored value alone, the second
+ clears it. Pydantic advertises ``"default": null`` for every optional
+ field, and a client that materialises those defaults sends nulls the
+ caller never asked for, which the model then reads as deliberate clears.
+
+ Dropping the advertised default keeps the fields optional without handing
+ clients a value to fill in. Validation is unchanged: omitting a field
+ still leaves it unset, and passing ``null`` still clears.
+ """
+
+ @classmethod
+ def __get_pydantic_json_schema__(
+ cls, core_schema: Any, handler: GetJsonSchemaHandler
+ ) -> Dict[str, Any]:
Review Comment:
This module already uses modern built-in generics elsewhere (e.g.,
`dict[str, Any]` in tests), and `from __future__ import annotations` is
enabled. For consistency, prefer `dict[str, Any]` over `Dict[str, Any]` and
drop `Dict` from the imports.
--
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]
