Three pre_validate methods were checking only canonical field names, but
the Pydantic schemas accept validation aliases. For example
PieChartConfig.dimension accepts "groupby" as an alias, so sending
{"groupby": ...} would pass the schema but be incorrectly rejected by
pre_validate.
Four normalize_column_refs implementations did not guard against
sql_expression metrics (name=None), causing AttributeError when
_get_canonical_column_name(None, ...) was called. XYChartPlugin already
handled this correctly; the fix brings the other plugins in line.
Pre-validate alias fixes:
- TableChartPlugin: accept columns/all_columns/groupby (AliasChoices)
- PieChartPlugin: accept dimension/groupby (AliasChoices)
- PivotTableChartPlugin: accept rows/groupby/dimension (AliasChoices)
sql_expression normalization guards:
- BigNumberChartPlugin: skip metric normalization when sql_expression set
- PieChartPlugin: same for metric field
- MixedTimeseriesChartPlugin: skip in _norm_list helper
- PivotTableChartPlugin: skip in _norm_col_list helper
create_app() called configure_mcp_chart_registry() unconditionally, which
imported mcp_config and its top-level fastmcp dependency, breaking plain
Superset installs without the [fastmcp] extra. Move the registry configure
call entirely into flask_singleton.py (both the standalone and reused-app
paths), making it the single configure site and resolving the dual-site
review finding.
Earlier style commits reformatted 24 unrelated files with a newer local
ruff than the pinned 0.9.7, removed the PT004 ignore from
pyproject.toml, and the initial branch commit accidentally deleted
tests/unit_tests/utils/test_split.py. None of these belong to the chart
plugin registry change; restore them to master to keep the PR scoped.
If two plugins claim the same viz_type, display_name_for_viz_type()
silently resolves to the iteration-order winner. Surface a warning at
register() time so plugin authors catch the shadowing immediately.
- schema_validator.py: add circular-import comment to both local registry
imports (H1); extract valid_types before the conditional so all_types()
is called once instead of in each error branch (N1)
- plugin.py: expand BaseChartPlugin docstring to list all default method
behaviours including schema_error_hint (N3); add comment warning that
native_viz_types is a class-level shared dict — subclasses must override
as a class attribute, not mutate in place (M1)
- registry.py: expand _reset_for_testing() docstring with explicit warning
that direct global assignment is not reverted by pytest monkeypatch —
callers must restore state in teardown (M2)
- mcp_config.py: replace Dict/Optional from typing with dict/X|None modern
syntax; remove now-unused Optional and Dict imports (N2)
- initialization/__init__.py: add docstring to configure_mcp_chart_registry()
explaining the known two-call pattern in MCP-standalone startup and why
the stale-config window between the two calls is benign in practice (H2)
Move bare `import superset.mcp_service.chart.plugins` before the `from`
imports per isort conventions; CI was failing with I001 (unsorted-imports).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Presence checks like `"y" not in config` pass silently when `y=[]` is submitted,
deferring to Pydantic's min_length error instead of the friendlier ChartGenerationError.
Switch to falsy checks (`not config.get("y")`) to catch both missing keys and empty
lists in the same early guard — matching the pattern already applied to pivot_table.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- big_number.py pre_validate: add sql_expression branch; return
MISSING_SQL_METRIC_LABEL when label is absent/non-string, so the
existing unit tests (and LLM callers) get a clear actionable error.
- xy.py normalize_column_refs: skip entries with sql_expression set
(name is None for these metrics); previously crashed with
AttributeError: 'NoneType'.lower() in _get_canonical_column_name.
- test_big_number_chart.py: replace three calls to deleted
SchemaValidator._pre_validate_big_number_config with
plugin.pre_validate() via get_registry().
- test_runtime_validator.py: replace call to deleted
RuntimeValidator._validate_cardinality with XYChartPlugin.get_runtime_warnings;
patch FormatTypeValidator to isolate cardinality guard.
- test_update_chart.py: set mock_create_preview.return_value to a
3-tuple so the update_chart unpack doesn't crash; change RuntimeError
to ValueError which is in NORMALIZATION_EXCEPTIONS.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- schema_validator: collapse is_enabled()+get() double lookup into a
single get() call so operator-supplied enabled_func is invoked once
- update_chart: use guarded chart_datasource_id local var instead of
re-accessing chart.datasource_id after the None check
- chart_utils: propagate post_map_validate() details+suggestions into
the raised ValueError so callers log actionable context
- schemas: clarify chart_type_display_name description — prefer over
viz_type when present, fall back to viz_type when null
- schemas: add or-empty-string fallback to dedup key labels to satisfy
mypy (dict is typed dict[tuple[bool, str], str])
- plugins/xy: guard config.x.name is not None before cardinality check
- runtime/__init__, plugins/xy, registry: add noqa BLE001 to intentional
broad exception catches with inline rationale comments
- tests: add TestUpdateChartColumnNormalization covering normalization
called with guarded ID, graceful exception handling, and skip-when-null
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- pivot_table.pre_validate: `not config.get("rows/metrics")` catches both
missing keys and empty lists, matching PivotTableChartConfig min_length=1
- chart_utils.map_config_to_form_data: omit `(chart_type=None)` suffix from
ValueError when chart_type is None to avoid misleading error messages
schemas.py:
- Add @field_validator('temporal_column') on BigNumberChartConfig — the regex removal
in d883b622 left this field with only min/max_length guards; ColumnRef.name and
FilterConfig.column already used sanitize_user_input (check_sql_keywords=True) and
sanitize_user_input respectively, but BigNumberChartConfig.temporal_column was missed.
PR #39915 (relaxing the original regex) was closed; this PR covers the same intent
by relying on sanitize_user_input validators instead.
- serialize_chart_object: split the broad except Exception (BLE001) into ImportError
(for import-failure path) + Exception (for third-party plugin errors) so the scope
of each catch is explicit.
registry.py:
- Add _reset_for_testing() that resets _REGISTRY, _plugins_loaded, _plugins_load_failed,
and _filter_config — gives tests a single clean-slate function instead of four
separate monkeypatches.
- Move _RegistryProxy instantiation to module level (_PROXY); get_registry() returns
the singleton instead of allocating a new object on every call.
file modes:
- Remove executable bits (100755 → 100644) from 9 files: plugin.py, all 7 plugin
files, registry.py, and initialization/__init__.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- register() now holds _plugins_lock when writing to _REGISTRY, preventing
concurrent write races if plugins are registered outside the bootstrap path
- _ensure_plugins_loaded() now sets _plugins_load_failed=True on ImportError
so subsequent lookups return None immediately instead of retrying the import
on every call
- _isolated_registry fixture in test_registry.py resets _plugins_load_failed
- Two new tests cover the circuit-breaker skip path and the failure-flag path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
validate_and_compile already runs Tier 1 (validate_against_dataset)
with the same fuzzy-match suggestions in CompileResult.error_obj.
The explicit pre-call fetched dataset context a second time via a
separate DB query, producing duplicate work on every update request.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A saved metric and a regular column with the same input name resolve
to different display labels after normalization (saved metrics use the
dataset's actual casing). Using a plain string key incorrectly flags
them as duplicates; keying on (saved_metric, label) avoids the false
collision.
Fixes test_xy_saved_metric_uses_metric_casing.
Add _get_canonical_metric_name() to DatasetValidator that searches only
available_metrics, preventing a column with matching case-insensitive name
from shadowing a saved metric's canonical casing.
Update all 7 chart plugins (xy, table, pie, big_number, handlebars,
mixed_timeseries, pivot_table) to branch on saved_metric flag: saved
metrics now go through _get_canonical_metric_name while regular column
refs continue to use _get_canonical_column_name.
Fix pre_validate alias handling in xy and mixed_timeseries plugins to
accept Pydantic AliasChoices keys (metrics/x_axis/metrics_b) so payloads
using canonical Superset field names are not incorrectly rejected.
Add TestGetCanonicalMetricName, TestSavedMetricNormalizationCorrectness,
and TestPreValidateAliasHandling test classes covering the collision case
and alias acceptance.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces a dynamic filter layer in the chart type registry so operators can
disable individual plugins (e.g. `handlebars`) without a code deploy:
- `MCP_DISABLED_CHART_PLUGINS: frozenset[str]` — static deny-list in mcp_config.py
- `MCP_CHART_PLUGIN_ENABLED_FUNC: Callable[[str], bool] | None` — dynamic hook
for Harness/Split/per-user targeting; takes precedence over the deny-list
- Both keys are propagated through `get_mcp_config()` defaults
registry.py changes:
- `_PluginFilterConfig` frozen dataclass replaces two bare globals so
configure() replaces them atomically (no torn reads under concurrency)
- `configure(disabled, enabled_func)` — called at app init; accepts any
iterable for `disabled`; validates `enabled_func` is callable
- `_is_plugin_enabled()` — reads config once, fails closed on callable exception
- `get()` / `all_types()` / `is_enabled()` apply the filter at lookup time;
`is_registered()` and `display_name_for_viz_type()` intentionally bypass it
so callers can distinguish "unknown" vs "disabled" and existing charts still
resolve display names for disabled viz types
schema_validator.py: two-step pre-check — `is_registered()` for unknown types,
`is_enabled()` for disabled ones, with distinct `DISABLED_CHART_TYPE` error code.
Wiring:
- `SupersetAppInitializer.configure_mcp_chart_registry()` called after
`configure_feature_flags()` in `init_app()`
- `flask_singleton.py` re-calls `registry.configure()` after the MCP config
overlay so MCP-specific overrides in `superset_config.py` take effect in
standalone MCP mode
Tests: 28 cases in test_registry_filters.py covering deny-list, callable hook,
fail-closed on exception, all_types() filtering, display_name bypass, atomic
reconfigure, and configure() with list/tuple/frozenset inputs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Split an 89-char comment line and an over-limit condition in update_chart.py
to satisfy the ruff E501 rule. Also applied ruff format.
Two TestUpdateChartValidationGate tests expected CHART_VALIDATION_FAILED but
received CHART_DATASET_NOT_FOUND because _validate_update_against_dataset calls
DatasetValidator.validate_against_dataset before validate_and_compile, and the
existing mocks provided a Mock() object for chart.datasource whose .id attribute
is an auto-generated MagicMock (not a real int). Added a patch for
DatasetValidator.validate_against_dataset returning (True, None) so the
column-validation tier is bypassed and the test reaches the mocked
validate_and_compile response as intended.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
P1.1 registry.py: move _plugins_loaded=True to after successful import so a
failed load doesn't permanently poison the registry.
P1.3 schemas.py: remove overly restrictive ColumnRef.name / FilterClause.column
/ BigNumberChartConfig.temporal_column regex that blocked valid column names
containing parentheses, slashes, and other SQL-common characters.
P2.3 (DRY): eliminate _CHART_TYPE_ERROR_HINTS second-registry in
schema_validator.py by adding schema_error_hint() to ChartTypePlugin protocol,
BaseChartPlugin default, and all 7 plugin classes. SchemaValidator now delegates
to the plugin registry instead of maintaining a parallel dict.
P3.3 test_registry.py: add full registry unit-test coverage (register, get,
all_types, is_registered, display_name_for_viz_type, proxy methods, duplicate
warning, empty chart_type validation, insertion-order guarantee).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
update_chart was only running SchemaValidator + Tier-2 compile check,
silently skipping DatasetValidator's column-existence + fuzzy-match
and column-name normalisation layers that generate_chart runs.
A typo like {name: "reveneu"} would save the broken chart and only
surface as a render-time failure in the browser.
Now matches generate_chart pipeline:
- Layer 2: DatasetValidator.validate_against_dataset() — column
existence check with fuzzy-match "did you mean?" suggestions returned
to the LLM before any DB write occurs
- Layer 4: DatasetValidator.normalize_column_names() — case
normalisation so "order_date" resolves to "OrderDate" if that is the
canonical dataset name
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_ensure_plugins_loaded() used an unprotected boolean flag, making it
unsafe under concurrent first-call scenarios (e.g. gunicorn multi-thread
workers). Double-checked locking with threading.Lock eliminates the race.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Split long string literal in schema_validator.py line 202 (E501, 94 > 88 chars)
- Apply ruff format auto-fixes to big_number.py, handlebars.py, and test_get_chart_data.py
All per-method local imports in the 7 chart plugins were moved to module-level.
None of them create circular imports: schemas.py, chart_utils.py, and
dataset_validator.py are safe to import at plugin load time because those
modules guard their own registry lookups with local imports.
- big_number: add map_big_number_config, _big_number_chart_what,
_summarize_filters, DatasetValidator to top-level imports
- pie: add map_pie_config, _pie_chart_what, _summarize_filters, PieChartConfig,
DatasetValidator to top-level imports
- xy: add map_xy_config, _xy_chart_what/context, XYChartConfig, DatasetValidator,
FormatTypeValidator, CardinalityValidator to top-level imports
- table: add map_table_config, _table_chart_what, _summarize_filters,
TableChartConfig, DatasetValidator to top-level imports
- pivot_table: add map_pivot_table_config, _pivot_table_what, _summarize_filters,
PivotTableChartConfig, DatasetValidator to top-level imports
- mixed_timeseries: add map_mixed_timeseries_config, _mixed_timeseries_what,
_summarize_filters, MixedTimeseriesChartConfig, DatasetValidator to top-level
- handlebars: add map_handlebars_config, _handlebars_chart_what, _summarize_filters,
HandlebarsChartConfig, DatasetValidator to top-level imports
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove redundant local imports from BigNumberChartPlugin.post_map_validate()
now that BigNumberChartConfig and is_column_truly_temporal are at top level
- Add explanatory comments on the two remaining local get_registry imports in
chart_utils.py and dataset_validator.py (circular import prevention)
- Fix schema_validator.py and generate_chart.py docstring: XY 'x' field is
optional (defaults to dataset primary datetime column), not required
- Propagate cardinality suggestions alongside warnings in XYChartPlugin
- Clarify app.py instructions: chart_type_display_name is null for viz_types
outside the 7 generate_chart-supported types
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On top of the dead-code elimination in the previous commit:
- Add lazy _ensure_plugins_loaded() bootstrap to ChartTypeRegistry so the
registry is populated even without importing app.py (fixes isolated test runs)
- Delegate _RegistryProxy methods to module-level functions so bootstrap runs
- Guard register() against empty chart_type strings
- Add generate_name + resolve_viz_type to ChartTypePlugin Protocol and
BaseChartPlugin; delegate generate_chart_name/_resolve_viz_type in
chart_utils to the plugin registry
- Add _with_context static helper to BaseChartPlugin (shared by all plugins)
- Fix stale 'five methods' → 'eight methods' docstring in plugin.py
- Add TypeVar _C to normalize_column_names so mypy infers correct return type
- Fix broken tests: update _pre_validate_big_number_config → _pre_validate_chart_type,
remove deleted TestNormalizeXYConfig/TestNormalizeTableConfig classes,
update runtime validator tests for removed _validate_format_compatibility /
_validate_cardinality methods, add x is not None narrowing guards
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
H1: Delete 7 dead _pre_validate_* static methods from SchemaValidator
— exact duplicates of plugin pre_validate() methods, never called
after _pre_validate_chart_type() was updated to delegate to plugin.
H2: Inline DatasetValidator._normalize_xy_config/_normalize_table_config
into XYChartPlugin/TableChartPlugin.normalize_column_refs() and delete
both DatasetValidator helper methods. The 5 other plugins already
called _get_canonical_column_name directly; XY and Table now match.
H3: Add generate_name()/resolve_viz_type() to ChartTypePlugin protocol
and BaseChartPlugin, implement in all 7 plugins. Replace the 7-arm
isinstance chain in generate_chart_name() and the 7-arm elif chain
in _resolve_viz_type() with single-line registry dispatch.
H4: Add a sync comment above _CHART_TYPE_ERROR_HINTS to document that
it must stay in sync with the plugin registry.
M4: Move logger=logging.getLogger(__name__) from inside
XYChartPlugin.get_runtime_warnings() to module scope.
Each ChartTypePlugin now declares:
- display_name: human-readable label for the chart_type discriminator
(e.g. "Line / Bar / Area / Scatter Chart", "Pivot Table")
- native_viz_types: dict mapping every Superset-internal viz_type the
plugin produces to a user-friendly name
(e.g. {"echarts_timeseries_line": "Line Chart", "echarts_area": "Area Chart"})
The registry gains display_name_for_viz_type(viz_type) which searches
all plugins' native_viz_types maps, replacing the need for a separate
viz_type_display_names.json or viz_type_names.py module.
ChartInfo gains a chart_type_display_name field populated via the registry,
so list_charts / get_chart_info return human-readable chart type names.
The MCP system instructions now reference display names rather than
internal viz_type identifiers.
Replaces four scattered dispatch locations (schema_validator, dataset_validator,
chart_utils, runtime validator) with a central ChartTypePlugin registry. Each of
the 7 supported chart types (xy, table, pie, pivot_table, mixed_timeseries,
handlebars, big_number) now owns its pre-validation, column extraction, form_data
mapping, post-map validation, column normalization, and runtime warnings in a
single plugin class.
Key changes:
- Add ChartTypePlugin protocol and BaseChartPlugin base class (plugin.py)
- Add ChartTypeRegistry with register/get/all_types helpers (registry.py)
- Add 7 chart type plugins under chart/plugins/ with full coverage
- Fix 5-type column validation gap: pie, pivot_table, mixed_timeseries, handlebars,
and big_number now participate in dataset column validation (previously silently skipped)
- Move BigNumber trendline temporal check to BigNumberChartPlugin.post_map_validate()
- Add get_runtime_warnings() to plugin protocol; XYChartPlugin implements
format/cardinality checks, removing isinstance(config, XYChartConfig) from RuntimeValidator
- Fix stale generate_chart.py docstring listing only 'xy' and 'table' chart types
- Add missing pie, pivot_table, mixed_timeseries handlers to _enhance_validation_error;
refactor into a data-driven lookup table to stay within complexity limits
- Fix empty details fallback in Pydantic error handler
