fix(mcp): stop partial-update tools from advertising null defaults

[tien238lnd]

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. From such a client, 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. update_chart has the same shape one level down: it merges the nested chart config against the stored one by model_fields_set, so a null the caller never named overwrites a stored value instead of leaving it alone. update_dashboard turns out to be safe today, because it applies each field behind an is not None check and ignores explicit nulls entirely; it takes the base anyway so its schema stops offering a value no branch would honour.

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. Whether the defaults get filled in is up to the client, and clients differ. Calling update_dataset_metric with only dataset_id and metric from Claude's desktop app, over a remote MCP connector, reaches the tool as input_value={'dataset_id': ..., 'metric': ..., 'metric_name': None, ..., 'extra': None} and is rejected; the same call from the Claude Code CLI over stdio arrives with the two named fields only and gets through. One client filling them is enough to make the schema a hazard, and the server cannot tell the two apart: a null the caller meant and a null materialised from the advertised default look identical on the wire.

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. The four request models take it, and so do the models whose fields get merged into stored state: BaseChartConfig, where update_chart reads model_fields_set, and TableColumnConfig, MetricCurrency, ColumnRef, AxisConfig and FilterConfig one level below. Two tests keep it that way, both walking the schema all the way down: test_partial_update_tools_advertise_no_null_default checks what the tools advertise over MCP, and test_omitted_means_unchanged_models_advertise_no_null_default checks every model built on the base, including ones added later.

Because these models are shared, four more tools advertise fewer defaults as a side effect — generate_chart, generate_explore_link, update_chart_preview and create_dataset_metric, which inherits the metric properties base — and get_chart_type_schema returns the chart schemas, so its responses lose the null defaults too. Nothing else about those schemas moves: normalise the removals away and they are identical to the base, and no outputSchema changes.

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

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	087afc8
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abf6f9a7935b800086badec
😎 Deploy Preview	https://deploy-preview-44573--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bito-code-review]

The flagged issue is correct. While OmittedMeansUnchanged correctly removes default: null from the JSON schema to prevent clients from accidentally clearing fields, the actual update logic in the dashboard service likely relies on Pydantic's model_fields_set to distinguish between omitted fields and fields explicitly set to None. If the service logic only checks for non-null values (e.g., if value is not None:), it will ignore explicit null values intended to clear fields.

To resolve this, the update logic should be updated to check model_fields_set instead of just the field value. For example:

# Instead of: if request.description is not None:
# Use:
if "description" in request.model_fields_set:
    dashboard.description = request.description

This ensures that if description is in model_fields_set, the value (even if None) is applied to the model. Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?

superset/mcp_service/dashboard/schemas.py

if "description" in request.model_fields_set:
    dashboard.description = request.description

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.62%. Comparing base (c43770e) to head (087afc8).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44573      +/-   ##
==========================================
- Coverage   82.05%   81.62%   -0.44%     
==========================================
  Files        2991     2982       -9     
  Lines      184538   182990    -1548     
  Branches    42722    42089     -633     
==========================================
- Hits       151431   149369    -2062     
- Misses      30355    30873     +518     
+ Partials     2752     2748       -4     

Flag	Coverage Δ
hive	36.35% <100.00%> (+<0.01%)	⬆️
mysql	55.17% <100.00%> (+<0.01%)	⬆️
postgres	55.18% <100.00%> (?)
presto	38.23% <100.00%> (+<0.01%)	⬆️
python	86.08% <100.00%> (+0.01%)	⬆️
sqlite	54.90% <100.00%> (+<0.01%)	⬆️
unit	79.23% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 · 2

Open (4)

• The $ref dereference logic only handles the #/$defs/... form. If the schema generator emits… · New
• Indexing registered[name] will raise a raw KeyError if a tool is renamed/removed, which makes… · New
• This module already uses modern built-in generics elsewhere (e.g., dict[str, Any] in tests), and… · New
• This module already uses modern built-in generics elsewhere (e.g., dict[str, Any] in tests), and… · New

What changed in this PR

This PR prevents MCP partial-update tools from unintentionally clearing unspecified fields by removing "default": null from the JSON Schema of update request models where “omitted” and “explicit null” have different semantics.

Changes:

• Introduces an OmittedMeansUnchanged base model that strips default: null from generated JSON Schema.
• Applies the new base model to UpdateChartRequest, UpdateDashboardRequest, and UpdateDatasetMetricRequest.
• Adds a unit test to assert these partial-update tools no longer advertise null defaults for optional fields.

File	Description
tests/​unit_tests/​mcp_service/​test_mcp_tool_registration.py	Adds a regression test ensuring partial-update tools don’t advertise default: null.
superset/​mcp_service/​utils/​schema_utils.py	Adds OmittedMeansUnchanged schema customization to remove null defaults.
superset/​mcp_service/​dataset/​schemas.py	Switches update request model to inherit from OmittedMeansUnchanged.
superset/​mcp_service/​dashboard/​schemas.py	Switches update request model to inherit from OmittedMeansUnchanged.
superset/​mcp_service/​chart/​schemas.py	Updates chart update request model inheritance to include OmittedMeansUnchanged.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bito-code-review]

Code Review Agent Run #e7e06e

Actionable Suggestions - 2

• tests/unit_tests/mcp_service/test_mcp_tool_registration.py - 2

  • Missing return type annotation · Line 273-273
  • Unguarded tool lookup KeyError · Line 275-278

Additional Suggestions - 1

• superset/mcp_service/utils/schema_utils.py - 1

  • CWE-409: Mutation During Iteration · Line 56-58
    If `handler(core_schema)` returns a schema whose `properties` is a mutable mapping (e.g. a pydantic `defaultdict`-backed dict), `for field in schema.get("properties", {}).values()` mutates that mapping while iterating: deleting `field["default"]` mutates the per-field dict, and if any property dict is itself the values view's element being resized the loop can raise `RuntimeError: dictionary changed size during iteration`. Iterating `list(schema.get("properties", {}).values())` avoids it. Verified on pydantic 2.13.5 that the happy path works; this hardens against handler variants. ([CWE-409](https://cwe.mitre.org/data/definitions/409.html))

Review Details

• Files reviewed - 5 · Commit Range: 25e0679..25e0679

  • superset/mcp_service/chart/schemas.py
  • superset/mcp_service/dashboard/schemas.py
  • superset/mcp_service/dataset/schemas.py
  • superset/mcp_service/utils/schema_utils.py
  • tests/unit_tests/mcp_service/test_mcp_tool_registration.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[tien238lnd]

Pushed 93bfb4b and corrected the description, after testing the claim it rested on.

Which client fills the defaults. I had written that Claude Code does. That is wrong as stated: through the CLI over stdio, a call naming two arguments arrives with exactly those two, before and after this change. Through Claude's desktop app over a remote MCP connector, the same call reaches the tool as input_value={'dataset_id': ..., 'metric': ..., 'metric_name': None, ..., 'extra': None} and is rejected with metric_name cannot be empty or null. So clients differ, and a server cannot tell a null the caller meant from a null materialised out of the schema. The description now says that instead.

Where the null actually lands for charts. CodeAnt's point about update_dashboard was right, and chasing it found a real gap: update_chart reads model_fields_set on the nested chart config, not on the request, so UpdateChartRequest alone left the exposed fields untouched. BaseChartConfig now carries the base, which covers every config in the union. update_dashboard applies each field behind an is not None check and ignores explicit nulls, so it was never affected; it keeps the base so its schema stops offering a value no branch would honour, and the base docstring no longer claims null uniformly clears.

Tests. test_omitted_means_unchanged_models_advertise_no_null_default walks every subclass of the base, so a model that joins it later is covered without touching the test. The schema helper now resolves allOf-wrapped and definitions refs and fails loudly on a shape it does not recognise, rather than returning something that trivially passes; a missing tool reports absence instead of a bare KeyError.

Locally: tests/unit_tests/mcp_service is green except test_query_dataset_reexecutes_across_rollover, which fails the same way on master here. The red babel-extract is a messages.pot drift on master (one string added, one removed) — nothing in this branch touches a translated string.

[aminghadersohi]

pre-commit (current) is red on a real mypy error in the new test, and update_chart still silently clears stored per-column table formatting because TableColumnConfig never took the new base. Both reproduced locally.

[bito-code-review]

Code Review Agent Run #04c8d2

Actionable Suggestions - 2

• tests/unit_tests/mcp_service/test_mcp_tool_registration.py - 2

  • Untyped local pending · Line 297-297
  • Untyped local tool · Line 311-311

Review Details

• Files reviewed - 3 · Commit Range: 25e0679..93bfb4b

  • superset/mcp_service/chart/schemas.py
  • superset/mcp_service/utils/schema_utils.py
  • tests/unit_tests/mcp_service/test_mcp_tool_registration.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[gabotorresruiz]

Hey @tien238lnd, thanks for this one, and thanks for going back and correcting the description once you had actually tested which client materialises the defaults.

I checked out 93bfb4b and dumped the input schema of all 72 registered tools at the head and at the merge base (c14ac29), then diffed them with the default: null removals normalised away: 0 residual differences. Nothing moved in required, additionalProperties, descriptions, enum members or nullability, and the tool outputSchemas do not change at all, so the patch removes exactly what it claims. The server side is untouched too: at the head UpdateDatasetMetricRequest.model_validate({"dataset_id": 1, "metric": "m", "description": None}) still puts description in model_fields_set and updates() still returns {"description": None}, so an explicit null still clears and an omitted field is still unset. I also copied test_partial_update_tools_advertise_no_null_default onto the merge base with the import stubbed, and it fails there listing 25 fields across the three tools, so the test pins the fix rather than an invariant that held anyway. tests/unit_tests/mcp_service/{chart,dashboard,dataset,utils} plus test_mcp_tool_registration.py are green here: 3499 passed, 3 skipped.

Leaving this as a comment rather than a verdict, since Amin's request for changes is already open and pre-commit (current) is still red on the mypy Missing return statement at test_mcp_tool_registration.py:264 that his suggestion fixes. Two notes inline, neither blocking on its own.

[tien238lnd]

Pushed f147b2a, which addresses both points of @aminghadersohi's review and the two notes from @gabotorresruiz.

The substantive one was real and mine: TableColumnConfig is merged per column label with exclude_unset=True, so update_chart still wiped a stored columnWidth or d3NumberFormat when a client filled the defaults — and both of my checks stayed green because they read only the top-level properties. The check now walks the whole schema at every depth, which covers a tool's inlined parameters and the $defs of model_json_schema() alike, and it only reaches zero with TableColumnConfig, MetricCurrency, ColumnRef, AxisConfig and FilterConfig on the base as well. Reverting any one of the five reds it.

The red pre-commit was the mypy hook reading pytest.fail as Any, so the helper looked like it fell through; both exits are raise AssertionError now, which also narrows tool in the guard.

The summary names the three extra tools and get_chart_type_schema, whose advertised schemas change because the chart config models are shared. Bito's CWE-409 note on the loop is left alone — Amin is right that deleting a key from each inner field dict never resizes properties.

Locally: 3497 passed, 3 skipped across tests/unit_tests/mcp_service/{chart,dashboard,dataset,utils} and test_mcp_tool_registration.py; test_query_dataset_reexecutes_across_rollover fails the same way on the merge base here. mypy reports nothing in the two files I touched.

[aminghadersohi]

Our requested changes are addressed at f147b2a: no partial-update tool advertises a null default at any depth, and reverting any of the five new bases reds both tests. Separate follow-up: non-null defaults clobber the same way (a filled row_limit: 10000 overwrites a stored 500 on update_chart).

[bito-code-review]

Code Review Agent Run #4ca6a2

Actionable Suggestions - 0

Review Details

• Files reviewed - 3 · Commit Range: 93bfb4b..f147b2a

  • superset/mcp_service/chart/schemas.py
  • superset/mcp_service/dataset/schemas.py
  • tests/unit_tests/mcp_service/test_mcp_tool_registration.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[tien238lnd]

Merged master in at f65c748. The conflict was worth a note, since it moved where this fix belongs.

Upstream extracted a shared DatasetMetricProperties base and made UpdateDatasetMetricRequest and CreateDatasetMetricRequest derive from it, so the optional metric fields no longer live on the request model this PR had patched. The base class is now where OmittedMeansUnchanged sits:

class DatasetMetricProperties(OmittedMeansUnchanged):

update_dataset_metric is covered through inheritance, and create_dataset_metric picks it up too — harmless, because it overrides metric_name and expression as required fields, and a required field advertises no default to fill in. That does add create_dataset_metric to the set of tools whose advertised schema changes as a side effect of sharing these models, alongside generate_chart, generate_explore_link, update_chart_preview and get_chart_type_schema.

This is the same shape as the TableColumnConfig miss @aminghadersohi caught: the base has to sit on the model that actually holds the fields. Both invariant tests still pass at zero, and test_omitted_means_unchanged_models_advertise_no_null_default covers the new base without needing an edit.

Locally: 3082 passed, 3 skipped across tests/unit_tests/mcp_service/{dataset,chart,dashboard} plus the registration suite, with only test_query_dataset_reexecutes_across_rollover failing, which fails the same way on master here.

[gabotorresruiz]

Hey @tien238lnd, this is in good shape now. Approving, and nice work turning the whole thread around in one push.

Re-ran everything at f65c748 against the new merge base 35e5850. Amin's gap is closed at the only level this fix can act on: the advertised TableColumnConfig sub-schema inside update_chart no longer carries a default on columnWidth, d3NumberFormat or d3TimeFormat, while additionalProperties: false, the descriptions, the minimum/minLength/maxLength constraints and the {"type": "null"} branch all survive. All three partial-update tools now report zero null defaults at every depth, against 394, 11 and 11 at the base. Across all 74 registered tools, normalising the removals away leaves 0 residual difference in the input schemas and 0 in the output schemas, and the get_chart_type_schema responses come out identical.

No overcorrection, which was the thing I most wanted to check: updates() still returns {"description": None, "d3format": None} when those are passed as null and only {"verbose_name": "V"} when they are omitted, TableColumnConfig.model_validate({"columnWidth": None}) still puts column_width in model_fields_set and dumps {"columnWidth": None} under exclude_unset, and a deliberate column_config: {"Region": {"columnWidth": null}} still reaches form_data with the sibling d3NumberFormat and d3TimeFormat left on their stored values. I copied the new recursive check onto the merge base and it fails there with 416 offenders, so both tests pin the fix. I also swept the merged master for new exclude_unset and model_fields_set readers under superset/mcp_service/, and there are none beyond this diff, so create_dataset_metric and delete_dataset_metric arriving with the merge do not leave an uncovered partial update behind.

@aminghadersohi, your follow-up reproduces here: with row_limit omitted the merge keeps the stored 500, and with it filled from the advertised default it becomes 1000. For sizing, update_chart advertises 190 non-null defaults while update_dashboard and update_dataset_metric advertise none, so that one is update_chart only, and agreed it belongs in its own change since a non-null default carries real information and cannot simply be dropped.

CI is fully green, pre-commit (current) included. tests/unit_tests/mcp_service/{chart,dashboard,dataset,utils} plus test_mcp_tool_registration.py give 3553 passed, 3 skipped here; test_query_dataset_reexecutes_across_rollover passes under TZ=UTC, so that one is timezone sensitive rather than broken. One nit inline, take it or leave it.

[tien238lnd]

Pushed 087afc8 with the follow-up I promised once the other PR landed, plus @gabotorresruiz's nit.

#44337 merged on 29/09, so update_dataset is on master now, and its request model reads model_fields_set the same way the other three do. UpdateDatasetRequest takes OmittedMeansUnchanged and update_dataset joins OMITTED_MEANS_UNCHANGED_TOOLS. Reverting the base reds the check with six fields — cache_timeout, description, main_dttm_col, sql, sync_columns, table_name — so it pins the fix rather than passing by default. Master merged in cleanly at 77fb991 on the way.

The summary now names create_dataset_metric among the tools that advertise fewer defaults as a side effect, with the reason, and no longer calls the shared models "chart config models", which stopped being accurate when DatasetMetricProperties joined.

@aminghadersohi, this and TableColumnConfig are the two things your review asked for; your 25/09 note says the originals are addressed, so the only thing holding the PR is the standing CHANGES_REQUESTED from before that. Happy to split anything further out.

On the row_limit follow-up: agreed it belongs in its own change, and I will open it after this merges. A non-null default carries information, so the fix cannot be "drop the default" — it needs the tool to tell an advertised default from a caller's value, which is a different mechanism from this one.

One correction to what I have been reporting: the two test_query_dataset_reexecutes_across_rollover failures I kept calling pre-existing on master are timezone sensitive, as @gabotorresruiz found. They pass here under TZ=UTC and fail under my local ICT, so that is my machine, not master. Everything else: 358 passed in tests/unit_tests/mcp_service/dataset, 181 in the registration and inventory suites.

[aminghadersohi]

update_dataset now inherits OmittedMeansUnchanged and advertises no null defaults; reverting the base reds the registration test on all six fields. CI is green at 087afc8 and no findings remain open.

[bito-code-review]

Code Review Agent Run #7fe716

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: f147b2a..087afc8

  • superset/mcp_service/dataset/schemas.py
  • tests/unit_tests/mcp_service/test_mcp_tool_registration.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by