tien238lnd opened a new pull request, #44573: URL: https://github.com/apache/superset/pull/44573
### SUMMARY `update_chart`, `update_dashboard` and `update_dataset_metric` are partial updates: their request models read `model_fields_set`, so a field left out keeps the stored value while a field set to `null` clears it. Pydantic advertises `"default": null` for every optional field of those models, and a client that materialises schema defaults then sends `null` for fields the caller never named. They land in `model_fields_set`, and the call clears everything it did not mention. It is reproducible with no write at all, because the validators run before the tool body. Calling `update_dataset_metric` with only `dataset_id` and `metric` comes back as `metric_name cannot be empty or null`, a check that only fires when `"metric_name" in model_fields_set`. The same shape hits `update_chart` and `update_dashboard`, where the effect is a silent reset of whatever the caller left out, and fields that reject `null` end up behaving as required on every call. The server is not the one adding them: through `fastmcp.Client` a tool receives exactly what the caller passes, which is what `test_update_chart_*` and friends already rely on. Claude Code, as an MCP client, does materialise the defaults. ### Approaches considered 1. **Stop advertising the defaults** (this PR). The fields stay optional in the schema, but it no longer offers a value to fill in. Nothing changes for callers that already omit fields, and `null` keeps meaning "clear this". 2. **Give clearing its own opt-in**, e.g. a `clear: ["description"]` list, and treat `null` as "no change". This reads well, but it changes the request contract of three shipped tools and leaves every client that currently passes `null` to clear a field silently doing nothing instead. 3. **Document the behaviour** in each tool description and rely on clients not filling defaults. That leaves the tools correct only for clients that read and obey prose, and the failure stays silent when they do not. 1 is the smallest change that removes the cause rather than describing it, and it is the only one of the three that needs nothing from the client. A shared `OmittedMeansUnchanged` base drops the `"default": null` entry from the generated schema, and `test_partial_update_tools_advertise_no_null_default` keeps new fields and new tools from reintroducing it. ### TESTING INSTRUCTIONS `pytest tests/unit_tests/mcp_service/chart tests/unit_tests/mcp_service/dashboard tests/unit_tests/mcp_service/dataset tests/unit_tests/mcp_service/utils tests/unit_tests/mcp_service/test_mcp_tool_registration.py` Manually, from a client that fills schema defaults: - Before this change, `update_dataset_metric` with only `dataset_id` and `metric` is rejected with `metric_name cannot be empty or null`; after it, the call is accepted and changes nothing. - Update one field of a chart or dashboard and confirm the other optional fields keep their stored values. - Passing an explicit `null` still clears the field. ### ADDITIONAL INFORMATION - [ ] Has associated issue: 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
