tien238lnd commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4101463558
##########
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:
Fair ask — the summary now names them: `generate_chart`,
`generate_explore_link` and `update_chart_preview` advertise fewer defaults as
a side effect of the shared config models, and `get_chart_type_schema` returns
those schemas so its responses change too. It also says what does not move,
which is the part your diff establishes.
##########
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()
+ if "default" in spec and spec["default"] is None
+ )
Review Comment:
This was exactly the right measurement to post, thank you — it turned "269
leftovers" into one model that matters and four that make the assertion
reachable. f147b2a44 puts the base on all five and uses your single walk, near
enough verbatim; it is the only version that handles both shapes, since a
tool's parameters arrive inlined while `model_json_schema()` uses `$defs`.
Local run at that head:
`tests/unit_tests/mcp_service/{chart,dashboard,dataset,utils}` plus
`test_mcp_tool_registration.py` give 3497 passed, 3 skipped, with only
`test_query_dataset_reexecutes_across_rollover` failing, which fails the same
way on the merge base here.
##########
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()
+ if "default" in spec and spec["default"] is None
+ )
+
+
+def _omitted_means_unchanged_models() -> list[type[OmittedMeansUnchanged]]:
+ """Return every model built on ``OmittedMeansUnchanged``."""
+ models: list[type[OmittedMeansUnchanged]] = []
+ pending = [OmittedMeansUnchanged]
Review Comment:
Annotated in f147b2a44: `pending` now carries its type.
##########
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()
+ if "default" in spec and spec["default"] is None
+ )
+
+
+def _omitted_means_unchanged_models() -> list[type[OmittedMeansUnchanged]]:
+ """Return every model built on ``OmittedMeansUnchanged``."""
+ models: list[type[OmittedMeansUnchanged]] = []
+ pending = [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 = registered.get(name)
Review Comment:
Annotated in f147b2a44: `tool` now carries its type.
--
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]
