Skip to content

fix(mcp): reject unknown fields in nested chart-config models too (#42626) - #42732

Open
rusackas wants to merge 2 commits into
masterfrom
tdd/issue-42626-mcp-nested-config-unknown-fields
Open

fix(mcp): reject unknown fields in nested chart-config models too (#42626)#42732
rusackas wants to merge 2 commits into
masterfrom
tdd/issue-42626-mcp-nested-config-unknown-fields

Conversation

@rusackas

@rusackas rusackas commented Aug 3, 2026

Copy link
Copy Markdown
Member

SUMMARY

Fixes #42626: only the 10 top-level MCP chart config models (PieChartConfig, XYChartConfig, etc.) inherited UnknownFieldCheckMixin, which rejects unknown fields with a "did you mean?" suggestion. The nested sub-models they're composed of — AxisConfig, LegendConfig, CurrencyFormat, FilterConfig, SortByConfig, ColumnRef — were plain BaseModel, so pydantic's default extra="ignore" silently dropped a typo'd field one level down instead of raising.

This was worse than a plain rejection for an MCP client (especially an LLM driving one): update_chart would return success: true with no warnings, so the caller had no signal the setting it just sent was silently dropped. The reporter's own repro showed three successive update_chart calls each returning success while changing nothing.

Fix. Give every nested config model the same UnknownFieldCheckMixin (and matching extra="ignore" config) the top-level models already use, since the mixin itself has no top-level-specific assumptions — it just inspects model_fields on whatever class it's mixed into.

A separate, pre-existing wrinkle this surfaced, left as a follow-up (not fixed here): XYChartConfig.x declares x_axis as one of its validation_alias choices, but the model also has its own field literally named x_axis: AxisConfig. That's a real alias/field-name collision — pydantic resolves it in favor of the actual field name, so a column dict sent through the x_axis key was always routed into x_axis: AxisConfig, not into x: ColumnRef as the alias implies. Before this fix that silently no-op'd (extra fields ignored); after this fix it would raise "Unknown field" for any ColumnRef-shaped payload sent that way. The one existing test that happened to rely on this (test_known_aliases_not_flagged_as_unknown) has been updated to use the unambiguous x_column alias instead, but the underlying alias collision on XYChartConfig itself is untouched — worth its own follow-up if it's confusing clients in practice.

TESTING INSTRUCTIONS

pytest tests/unit_tests/mcp_service/chart/test_chart_schemas.py
pytest tests/unit_tests/mcp_service/ --ignore=tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py

test_unknown_field_nested_one_level_down_is_rejected now passes (green). Full mcp_service unit suite (3132 tests, excluding the e2e ASGI-transport smoke file, which is unrelated and flaky under heavy parallel load) passes with no regressions.

ADDITIONAL INFORMATION

Only the 10 top-level chart config models (PieChartConfig, XYChartConfig,
etc.) inherit UnknownFieldCheckMixin. Nested sub-models they're composed
of -- AxisConfig, LegendConfig, CurrencyFormat, FilterConfig, SortByConfig,
ColumnRef -- are plain BaseModel, so pydantic's default extra="ignore"
silently drops a typo'd field one level down instead of raising the same
"did you mean?" error a top-level typo gets.

This is worse than a plain rejection for an MCP client (especially an LLM
driving one): update_chart returns success:true with no warnings, so the
caller has no signal the setting it just sent was silently dropped.

Currently red: AxisConfig.model_validate() with an unknown field succeeds
today instead of raising, documenting the gap until the nested models pick
up UnknownFieldCheckMixin too.
@bito-code-review

bito-code-review Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #1e07cb

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 57ef18b..57ef18b
    • tests/unit_tests/mcp_service/chart/test_chart_schemas.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 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

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.01%. Comparing base (25ab961) to head (a7ba320).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42732      +/-   ##
==========================================
- Coverage   65.57%   65.01%   -0.56%     
==========================================
  Files        2818     2819       +1     
  Lines      160038   160168     +130     
  Branches    36557    36569      +12     
==========================================
- Hits       104942   104135     -807     
- Misses      53051    53984     +933     
- Partials     2045     2049       +4     
Flag Coverage Δ
hive 38.10% <100.00%> (+0.01%) ⬆️
mysql 57.92% <100.00%> (+0.05%) ⬆️
postgres 57.96% <100.00%> (+0.05%) ⬆️
presto 40.02% <100.00%> (+0.06%) ⬆️
python 58.18% <100.00%> (-1.11%) ⬇️
sqlite 57.59% <100.00%> (+0.05%) ⬆️
unit ?

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.

…2626)

Only the 10 top-level chart config models inherited UnknownFieldCheckMixin;
nested sub-models (ColumnRef, AxisConfig, LegendConfig, CurrencyFormat,
FilterConfig, SortByConfig) were plain BaseModel, so a typo'd field one
level down was silently dropped by pydantic's default extra="ignore"
instead of raising the same "did you mean?" error a top-level typo gets.
Give every nested config the same mixin and extra="ignore" config the
top-level models already use.

This also surfaces a separate, pre-existing wrinkle: XYChartConfig.x has
x_axis as a validation_alias, which collides with the model's own
x_axis: AxisConfig field of the same name -- pydantic resolves that
collision in favor of the real field name, so routing a column through
the x_axis alias silently landed in (and, before this fix, was silently
swallowed by) AxisConfig instead of populating x. That's a separate bug
in the alias design, left as a known follow-up rather than fixed here;
the one existing test that exercised it was updated to use the
unambiguous x_column alias instead.
@pull-request-size pull-request-size Bot added size/M and removed size/S labels Aug 4, 2026
@rusackas rusackas changed the title test(mcp): pin nested chart-config models reject unknown fields (#42626) fix(mcp): reject unknown fields in nested chart-config models too (#42626) Aug 4, 2026
@netlify

netlify Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit a7ba320
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a713596e4664e0008ae97b8
😎 Deploy Preview https://deploy-preview-42732--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

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

bito-code-review Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #680585

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 57ef18b..a7ba320
    • superset/mcp_service/chart/schemas.py
    • tests/unit_tests/mcp_service/chart/test_chart_schemas.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 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

AI Code Review powered by Bito Logo

@rusackas
rusackas requested a review from sadpandajoe August 4, 2026 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP: nested chart-config models silently accept unknown fields — update_chart reports success while discarding them

2 participants