gabotorresruiz commented on code in PR #44573:
URL: https://github.com/apache/superset/pull/44573#discussion_r4096792745
##########
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:
Following Amin's note above about how deep these two checks look, I measured
which of the leftovers actually change stored state, in case it helps scope the
fix. Driving `_build_replacement_form_data` at `93bfb4b` with the same update
twice, once naming only the fields and once with every advertised default
spelled out as `null`:
- `TableColumnConfig` is the only one that clobbers, exactly as Amin
measured. A stored `{"Region": {"columnWidth": 200, "d3NumberFormat": ",.2f"}}`
comes back with both keys `null`, because `merge_table_column_config` merges
per label at `chart_utils.py:575`.
- `ColumnRef`, `AxisConfig` and `FilterConfig` are 266 of the remaining 269
and are harmless: both calls produce identical `form_data`.
- `MetricCurrency` only adds a `"symbolPosition": null` key, and `updates()`
replaces `currency` wholesale, so nothing is lost.
I ran the fix too. With the base on `TableColumnConfig`, `MetricCurrency`,
`ColumnRef`, `AxisConfig` and `FilterConfig`, `update_chart`,
`update_dashboard` and `update_dataset_metric` all reach zero null defaults at
every depth, `TableColumnConfig` keeps its `additionalProperties: False`, and
nothing else changes across the 72 tools. `TableColumnConfig` and
`MetricCurrency` alone leave `update_chart` at 266, so the assertion only
reaches zero if the three harmless ones come along.
One thing to know if you make `_null_defaults` recurse: `tool.parameters`
arrives fully inlined (no `$defs`, `request` carries `properties` directly), so
the `$ref` and `allOf` branch in `_request_model_schema` never runs on the real
shape, while `model_json_schema()` in the second test does use `$defs`. A
single walk covers both:
```python
def _null_defaults(schema: Any, path: str = "") -> list[str]:
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)
```
##########
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:
Not a blocker, more a "let the description match what ships" note. Giving
`BaseChartConfig` the base reaches past the three tools in the title: comparing
`93bfb4b` against `c14ac29`, `generate_chart` goes from 366 advertised null
defaults to 255, `generate_explore_link` from 365 to 254 and
`update_chart_preview` from 385 to 265. It also changes a tool response and not
only the manifest, since `get_chart_type_schema` returns `json_schema()` for
all 15 chart types at `get_chart_type_schema.py:279`, and every one of those
responses loses its null defaults too.
I checked all of it and none of it worries me: once the `default: null`
removals are normalised away the schemas are identical to the base everywhere,
and `outputSchema` does not move. Could the summary mention the extra three
tools and `get_chart_type_schema` though? Someone reading the title would not
expect `generate_chart`'s advertised schema to change.
--
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]
