Skip to content

fix(mcp): handle all chart types in column normalization and reject adhoc_filters#39283

Draft
sadpandajoe wants to merge 2 commits intomasterfrom
sc-103356-mcp-adhoc-filters-fix
Draft

fix(mcp): handle all chart types in column normalization and reject adhoc_filters#39283
sadpandajoe wants to merge 2 commits intomasterfrom
sc-103356-mcp-adhoc-filters-fix

Conversation

@sadpandajoe
Copy link
Copy Markdown
Member

@sadpandajoe sadpandajoe commented Apr 10, 2026

SUMMARY

Two fixes for MCP generate_chart returning an opaque validation_system_error.

Fix 1: Column normalization for all chart types

normalize_column_names() previously fell through to TableChartConfig.model_validate() for non-XY/Table chart types (pie, pivot_table, mixed_timeseries, handlebars, big_number), which raised a Pydantic ValidationError due to discriminator mismatch. That exception wasn't caught by the narrow except-tuple in ValidationPipeline._normalize_column_names and surfaced to users as an opaque validation_system_error.

Now each chart type is reconstructed with its own model class, and the except clause uses broad Exception to catch any unexpected errors during normalization.

Fix 2: Reject adhoc_filters with a clear error

When adhoc_filters (Superset's internal format) was passed in config, it was silently dropped by Pydantic's extra="ignore", giving the caller no indication that filters were not applied. Now schema_validator detects it early and returns a clear UNSUPPORTED_ADHOC_FILTERS error with guidance to use the simplified filters field instead.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend-only change in MCP service validation.

TESTING INSTRUCTIONS

  1. Run the new unit tests:

    pytest tests/unit_tests/mcp_service/chart/test_new_chart_types.py::TestAdhocFiltersDetection -v
    pytest tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py::TestNormalizeNonXYTableChartTypes -v
  2. Verify that existing tests still pass:

    pytest tests/unit_tests/mcp_service/chart/ -v
  3. Manual verification via MCP:

    • Call generate_chart with a pie chart config → should no longer return validation_system_error
    • Call generate_chart with adhoc_filters in config → should return clear UNSUPPORTED_ADHOC_FILTERS error with suggestions

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

…dhoc_filters

Two fixes for sc-103356:

1. normalize_column_names() previously fell through to
   TableChartConfig.model_validate() for non-XY/Table chart types
   (pie, pivot_table, mixed_timeseries, etc.), which raised a Pydantic
   ValidationError due to discriminator mismatch. That exception wasn't
   caught by the narrow except-tuple in the pipeline, surfacing as an
   opaque validation_system_error. Now each chart type is reconstructed
   with its own model, and the except clause catches all exceptions.

2. When adhoc_filters is passed in config, it was silently dropped by
   Pydantic's extra="ignore". Now schema_validator detects it early and
   returns a clear error with guidance to use the 'filters' field instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.41%. Comparing base (de40b58) to head (a5d556a).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
.../mcp_service/chart/validation/dataset_validator.py 0.00% 13 Missing ⚠️
superset/mcp_service/chart/validation/pipeline.py 0.00% 2 Missing ⚠️
...t/mcp_service/chart/validation/schema_validator.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #39283      +/-   ##
==========================================
- Coverage   64.42%   64.41%   -0.01%     
==========================================
  Files        2553     2553              
  Lines      132588   132602      +14     
  Branches    30758    30765       +7     
==========================================
  Hits        85416    85416              
- Misses      45686    45700      +14     
  Partials     1486     1486              
Flag Coverage Δ
hive 39.95% <0.00%> (-0.01%) ⬇️
mysql 60.58% <0.00%> (-0.02%) ⬇️
postgres 60.67% <0.00%> (-0.02%) ⬇️
presto 41.74% <0.00%> (-0.01%) ⬇️
python 62.25% <0.00%> (-0.02%) ⬇️
sqlite 60.30% <0.00%> (-0.02%) ⬇️
unit 100.00% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 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.

…add missing tests

- Remove unused NORMALIZATION_EXCEPTIONS constant from dataset_validator.py
- Replace broad `except Exception` with explicit exception tuple including
  PydanticValidationError in pipeline.py normalization handler
- Add smoke tests for HandlebarsChartConfig and BigNumberChartConfig
  normalization
- Add handlebars and big_number to adhoc_filters detection parametrize list

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant