aminghadersohi commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3501654614
########## superset/mcp_service/chart/plugins/pivot_table.py: ########## @@ -0,0 +1,164 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Pivot table chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _pivot_table_what, + _summarize_filters, + map_pivot_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PivotTableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PivotTableChartPlugin(BaseChartPlugin): + """Plugin for pivot_table chart type.""" + + chart_type = "pivot_table" + display_name = "Pivot Table" + native_viz_types: ClassVar[Mapping[str, str]] = { + "pivot_table_v2": "Pivot Table", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): + missing_fields.append("'rows' (row grouping columns)") + if not config.get("metrics"): + missing_fields.append("'metrics' (aggregation metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pivot_fields", + message=( + f"Pivot table missing required fields: {', '.join(missing_fields)}" + ), + details="Pivot tables require row groupings and metrics", + suggestions=[ + "Add 'rows' field: [{'name': 'category'}]", + "Add 'metrics' field: [{'name': 'sales', 'aggregate': 'SUM'}]", + "Optional 'columns' for cross-tabulation: [{'name': 'region'}]", + ], + error_code="MISSING_PIVOT_FIELDS", + ) + + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/plugins/table.py: ########## @@ -0,0 +1,136 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Table chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _summarize_filters, + _table_chart_what, + map_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, TableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class TableChartPlugin(BaseChartPlugin): + """Plugin for table chart type.""" + + chart_type = "table" + display_name = "Table" Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -353,29 +353,44 @@ def map_config_to_form_data( | BigNumberChartConfig, dataset_id: int | str | None = None, ) -> Dict[str, Any]: - """Map chart config to Superset form_data.""" - if isinstance(config, TableChartConfig): - return map_table_config(config) - elif isinstance(config, XYChartConfig): - return map_xy_config(config, dataset_id=dataset_id) - elif isinstance(config, PieChartConfig): - return map_pie_config(config) - elif isinstance(config, PivotTableChartConfig): - return map_pivot_table_config(config) - elif isinstance(config, MixedTimeseriesChartConfig): - return map_mixed_timeseries_config(config, dataset_id=dataset_id) - elif isinstance(config, HandlebarsChartConfig): - return map_handlebars_config(config) - elif isinstance(config, BigNumberChartConfig): - if config.show_trendline and config.temporal_column: - if not is_column_truly_temporal(config.temporal_column, dataset_id): - raise ValueError( - f"Big Number trendline requires a temporal SQL column; " - f"'{config.temporal_column}' is not temporal." - ) - return map_big_number_config(config) - else: - raise ValueError(f"Unsupported config type: {type(config)}") + """Map chart config to Superset form_data via the plugin registry. + + The previous if/elif chain across all 7 chart types has been replaced by a + single registry lookup. Cross-field constraints (e.g. BigNumber trendline + temporal check) are now owned by each plugin's post_map_validate() method + rather than being baked into this dispatcher. + """ + # Local import: plugins call map_*_config from their to_form_data() methods, + # so chart_utils is loaded before plugins finish registering. A top-level + # import of registry here would trigger plugin loading mid-import = cycle. + from superset.mcp_service.chart.registry import get_registry + + chart_type = getattr(config, "chart_type", None) + plugin = get_registry().get(chart_type) if chart_type else None Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -353,29 +353,44 @@ def map_config_to_form_data( | BigNumberChartConfig, dataset_id: int | str | None = None, ) -> Dict[str, Any]: - """Map chart config to Superset form_data.""" - if isinstance(config, TableChartConfig): - return map_table_config(config) - elif isinstance(config, XYChartConfig): - return map_xy_config(config, dataset_id=dataset_id) - elif isinstance(config, PieChartConfig): - return map_pie_config(config) - elif isinstance(config, PivotTableChartConfig): - return map_pivot_table_config(config) - elif isinstance(config, MixedTimeseriesChartConfig): - return map_mixed_timeseries_config(config, dataset_id=dataset_id) - elif isinstance(config, HandlebarsChartConfig): - return map_handlebars_config(config) - elif isinstance(config, BigNumberChartConfig): - if config.show_trendline and config.temporal_column: - if not is_column_truly_temporal(config.temporal_column, dataset_id): - raise ValueError( - f"Big Number trendline requires a temporal SQL column; " - f"'{config.temporal_column}' is not temporal." - ) - return map_big_number_config(config) - else: - raise ValueError(f"Unsupported config type: {type(config)}") + """Map chart config to Superset form_data via the plugin registry. + + The previous if/elif chain across all 7 chart types has been replaced by a + single registry lookup. Cross-field constraints (e.g. BigNumber trendline + temporal check) are now owned by each plugin's post_map_validate() method + rather than being baked into this dispatcher. + """ + # Local import: plugins call map_*_config from their to_form_data() methods, + # so chart_utils is loaded before plugins finish registering. A top-level + # import of registry here would trigger plugin loading mid-import = cycle. + from superset.mcp_service.chart.registry import get_registry + + chart_type = getattr(config, "chart_type", None) + plugin = get_registry().get(chart_type) if chart_type else None + + if plugin is None: + if chart_type is None: + raise ValueError(f"Unsupported config type: {type(config)}") + raise ValueError( + f"Unsupported config type: {type(config)} (chart_type={chart_type!r})" + ) + + form_data = plugin.to_form_data(config, dataset_id=dataset_id) Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1279,87 +1294,32 @@ def _big_number_chart_what(config: BigNumberChartConfig) -> str: def generate_chart_name( - config: TableChartConfig - | XYChartConfig - | PieChartConfig - | PivotTableChartConfig - | MixedTimeseriesChartConfig - | HandlebarsChartConfig - | BigNumberChartConfig, + config: Any, dataset_name: str | None = None, ) -> str: """Generate a descriptive chart name following a standard format. - Format conventions (by chart type): - Aggregated (bar/scatter with group_by): [Metric] by [Dimension] - Time-series (line/area, no group_by): [Metric] Over Time - Table (no aggregates): [Dataset] Records - Table (with aggregates): [Metric] Summary - Pie: [Dimension] by [Metric] - Pivot Table: Pivot Table – [Row1, Row2] - Mixed Timeseries: [Primary] + [Secondary] - An en-dash followed by context (filters / time grain) is appended + Delegates to each plugin's ``generate_name()`` method. + See each plugin's ``generate_name`` for chart-type-specific format conventions. + An en-dash followed by context (filters / time grain) is appended by the plugin when such information is available. """ - if isinstance(config, TableChartConfig): - what = _table_chart_what(config, dataset_name) - context = _summarize_filters(config.filters) - elif isinstance(config, XYChartConfig): - what = _xy_chart_what(config) - context = _xy_chart_context(config) - elif isinstance(config, PieChartConfig): - what = _pie_chart_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, PivotTableChartConfig): - what = _pivot_table_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, MixedTimeseriesChartConfig): - what = _mixed_timeseries_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, HandlebarsChartConfig): - what = _handlebars_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - elif isinstance(config, BigNumberChartConfig): - what = _big_number_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - else: - return "Chart" + from superset.mcp_service.chart.registry import get_registry - name = what - if context: - name = f"{what} \u2013 {context}" - return _truncate(name) + plugin = get_registry().get(getattr(config, "chart_type", "")) Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1279,87 +1294,32 @@ def _big_number_chart_what(config: BigNumberChartConfig) -> str: def generate_chart_name( - config: TableChartConfig - | XYChartConfig - | PieChartConfig - | PivotTableChartConfig - | MixedTimeseriesChartConfig - | HandlebarsChartConfig - | BigNumberChartConfig, + config: Any, dataset_name: str | None = None, ) -> str: """Generate a descriptive chart name following a standard format. - Format conventions (by chart type): - Aggregated (bar/scatter with group_by): [Metric] by [Dimension] - Time-series (line/area, no group_by): [Metric] Over Time - Table (no aggregates): [Dataset] Records - Table (with aggregates): [Metric] Summary - Pie: [Dimension] by [Metric] - Pivot Table: Pivot Table – [Row1, Row2] - Mixed Timeseries: [Primary] + [Secondary] - An en-dash followed by context (filters / time grain) is appended + Delegates to each plugin's ``generate_name()`` method. + See each plugin's ``generate_name`` for chart-type-specific format conventions. + An en-dash followed by context (filters / time grain) is appended by the plugin when such information is available. """ - if isinstance(config, TableChartConfig): - what = _table_chart_what(config, dataset_name) - context = _summarize_filters(config.filters) - elif isinstance(config, XYChartConfig): - what = _xy_chart_what(config) - context = _xy_chart_context(config) - elif isinstance(config, PieChartConfig): - what = _pie_chart_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, PivotTableChartConfig): - what = _pivot_table_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, MixedTimeseriesChartConfig): - what = _mixed_timeseries_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, HandlebarsChartConfig): - what = _handlebars_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - elif isinstance(config, BigNumberChartConfig): - what = _big_number_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - else: - return "Chart" + from superset.mcp_service.chart.registry import get_registry - name = what - if context: - name = f"{what} \u2013 {context}" - return _truncate(name) + plugin = get_registry().get(getattr(config, "chart_type", "")) + if plugin is None: + return "Chart" + return _truncate(plugin.generate_name(config, dataset_name)) def _resolve_viz_type(config: Any) -> str: """Resolve the Superset viz_type from a chart config object.""" - chart_type = getattr(config, "chart_type", "unknown") - if chart_type == "xy": - kind = getattr(config, "kind", "line") - viz_type_map = { - "line": "echarts_timeseries_line", - "bar": "echarts_timeseries_bar", - "area": "echarts_area", - "scatter": "echarts_timeseries_scatter", - } - return viz_type_map.get(kind, "echarts_timeseries_line") - elif chart_type == "table": - return getattr(config, "viz_type", "table") - elif chart_type == "pie": - return "pie" - elif chart_type == "pivot_table": - return "pivot_table_v2" - elif chart_type == "mixed_timeseries": - return "mixed_timeseries" - elif chart_type == "handlebars": - return "handlebars" - elif chart_type == "big_number": - show_trendline = getattr(config, "show_trendline", False) - temporal_column = getattr(config, "temporal_column", None) - return ( - "big_number" if show_trendline and temporal_column else "big_number_total" - ) - return "unknown" + from superset.mcp_service.chart.registry import get_registry + + plugin = get_registry().get(getattr(config, "chart_type", "")) Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/mcp_service/chart/registry.py: ########## @@ -0,0 +1,279 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +ChartTypeRegistry — central registry mapping chart_type strings to plugins. + +Replaces the four previously-scattered dispatch locations: + - schema_validator.py: chart_type_validators dict + - dataset_validator.py: isinstance branches in _extract_column_references() + - chart_utils.py: if/elif chain in map_config_to_form_data() + - dataset_validator.py: isinstance branches in normalize_column_names() + +Usage:: + + from superset.mcp_service.chart.registry import get_registry + + plugin = get_registry().get("xy") + if plugin is None: + raise ValueError("Unknown chart type: xy") + form_data = plugin.to_form_data(config, dataset_id) +""" + +from __future__ import annotations + +import logging +import sys +import threading +from collections.abc import Callable, Iterable +from dataclasses import dataclass, field +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from superset.mcp_service.chart.plugin import ChartTypePlugin + +logger = logging.getLogger(__name__) + +_REGISTRY: dict[str, "ChartTypePlugin"] = {} +_plugins_loaded = False +_plugins_load_failed = False +_plugins_lock = threading.RLock() + +# --------------------------------------------------------------------------- +# Plugin filter — replaced atomically by configure() at app startup. +# Default: all registered plugins visible (no disabled set, no callable). +# --------------------------------------------------------------------------- + +PluginEnabledFunc = Callable[[str], bool] + + +@dataclass(frozen=True) +class _PluginFilterConfig: + disabled_plugins: frozenset[str] = field(default_factory=frozenset) + enabled_func: PluginEnabledFunc | None = None + + +_filter_config: _PluginFilterConfig = _PluginFilterConfig() + + +def _ensure_plugins_loaded() -> None: + """Lazily import the plugins package to populate _REGISTRY. + + Called before every registry lookup so the registry is always populated, + even when callers (tests, chart_utils, validators) import this module + directly without first importing app.py. + """ + global _plugins_loaded, _plugins_load_failed + if _plugins_loaded or _plugins_load_failed: + return + with _plugins_lock: + if not _plugins_loaded and not _plugins_load_failed: + registry_before_import = dict(_REGISTRY) + try: + import superset.mcp_service.chart.plugins # noqa: F401 + + _plugins_loaded = True + except Exception: # noqa: BLE001 — plugin import may raise anything + _REGISTRY.clear() + _REGISTRY.update(registry_before_import) + _plugins_load_failed = True + logger.exception( + "Failed to load built-in chart type plugins; " + "further lookups will return None" + ) + + +def configure( + disabled: Iterable[str] | None = None, + enabled_func: PluginEnabledFunc | None = None, +) -> None: + """Set runtime plugin filters. Called once during app initialization. + + Replaces the filter config atomically with a single assignment so concurrent + readers always observe a consistent (disabled_plugins, enabled_func) pair. + + Args: + disabled: chart_type strings to suppress. Accepts any iterable (set, + frozenset, list, tuple). Ignored when enabled_func is provided. + enabled_func: callable(chart_type) -> bool. When set, overrides + ``disabled``. Must be cheap and in-process — no network I/O per + call. On exception the registry fails *closed* (plugin hidden). + """ + global _filter_config + + if enabled_func is not None and not callable(enabled_func): + raise TypeError("enabled_func must be callable or None") + + new_config = _PluginFilterConfig( + disabled_plugins=frozenset(disabled or ()), + enabled_func=enabled_func, + ) + _filter_config = new_config + + if new_config.disabled_plugins: + logger.info( + "MCP chart plugins disabled: %s", sorted(new_config.disabled_plugins) + ) + if new_config.enabled_func is not None: + logger.info( + "MCP chart plugin dynamic filter configured: %r", new_config.enabled_func + ) + + +def _is_plugin_enabled(chart_type: str) -> bool: + """Return True if the plugin is currently enabled (not filtered out).""" + config = _filter_config # read once — atomic reference in CPython + if config.enabled_func is not None: + try: + return bool(config.enabled_func(chart_type)) + except Exception: # noqa: BLE001 — operator-supplied callable may raise anything + logger.warning( + "MCP_CHART_PLUGIN_ENABLED_FUNC raised for chart_type=%r; " + "failing closed (plugin hidden)", + chart_type, + exc_info=True, + ) + return False + return chart_type not in config.disabled_plugins + + +def register(plugin: "ChartTypePlugin") -> None: + """Register a chart type plugin in the global registry.""" + if not plugin.chart_type: + raise ValueError(f"{type(plugin).__name__} must define a non-empty chart_type") + with _plugins_lock: + if plugin.chart_type in _REGISTRY: + logger.warning( + "Overwriting existing plugin for chart_type=%r", plugin.chart_type + ) + for existing in _REGISTRY.values(): + if existing.chart_type == plugin.chart_type: + continue + colliding = plugin.native_viz_types.keys() & existing.native_viz_types + if colliding: + # display_name_for_viz_type() resolves to the first plugin in + # iteration order, making the later registration unreachable. + logger.warning( + "Plugin %r declares native_viz_types %s already claimed by " + "plugin %r; viz_type display-name lookups will resolve to " + "the earlier registration", + plugin.chart_type, + sorted(colliding), + existing.chart_type, + ) + _REGISTRY[plugin.chart_type] = plugin + logger.debug("Registered chart plugin: %r", plugin.chart_type) + + +def get(chart_type: str) -> "ChartTypePlugin | None": + """Return the plugin for chart_type, or None if unknown or disabled.""" + _ensure_plugins_loaded() + if chart_type not in _REGISTRY or not _is_plugin_enabled(chart_type): + return None + return _REGISTRY[chart_type] + + +def all_types() -> list[str]: + """Return enabled registered chart type strings in insertion order.""" + _ensure_plugins_loaded() + return [ct for ct in _REGISTRY if _is_plugin_enabled(ct)] + + +def is_registered(chart_type: str) -> bool: + """Return True if chart_type has a registered plugin, regardless of enabled state. + + Use this to distinguish an unknown chart type from a disabled one. + Use is_enabled() to check whether the plugin is currently available. + """ + _ensure_plugins_loaded() + return chart_type in _REGISTRY + + +def is_enabled(chart_type: str) -> bool: + """Return True if chart_type is registered AND currently enabled.""" + _ensure_plugins_loaded() + return chart_type in _REGISTRY and _is_plugin_enabled(chart_type) + + +def display_name_for_viz_type(viz_type: str) -> str | None: + """Return the user-facing display name for a Superset-internal viz_type. + + Searches every registered plugin's ``native_viz_types`` mapping. + Returns None if no plugin recognises the viz_type. + + Example:: + + display_name_for_viz_type("echarts_timeseries_line") # "Line Chart" + display_name_for_viz_type("pivot_table_v2") # "Pivot Table" + display_name_for_viz_type("unknown_type") # None + """ + _ensure_plugins_loaded() + for plugin in _REGISTRY.values(): + name = plugin.native_viz_types.get(viz_type) + if name is not None: + return name + return None + + +def _reset_for_testing() -> None: + """Reset all registry state to defaults. + + Only for use in tests that need a clean slate. Calling this in production + will discard all registered plugins and any runtime filter configuration. + + **Caller responsibility**: This function pops ``superset.mcp_service.chart.plugins`` + from ``sys.modules`` and directly assigns module globals (``_REGISTRY``, + ``_plugins_loaded``, etc.). Direct global assignment is NOT automatically + reverted by pytest's ``monkeypatch`` fixture. Callers must either use + ``monkeypatch.setattr`` for each global, or call ``_reset_for_testing()`` again + in teardown to restore the clean state. See ``test_registry.py`` for the + recommended ``monkeypatch.setattr`` isolation pattern. + """ + global _REGISTRY, _plugins_loaded, _plugins_load_failed, _filter_config + with _plugins_lock: + _REGISTRY = {} + _plugins_loaded = False + _plugins_load_failed = False + _filter_config = _PluginFilterConfig() + sys.modules.pop("superset.mcp_service.chart.plugins", None) + + +class _RegistryProxy: + """Thin proxy exposing registry functions as instance methods.""" + + def get(self, chart_type: str) -> "ChartTypePlugin | None": + return get(chart_type) + + def all_types(self) -> list[str]: + return all_types() + + def is_registered(self, chart_type: str) -> bool: + return is_registered(chart_type) + + def is_enabled(self, chart_type: str) -> bool: + return is_enabled(chart_type) + + def display_name_for_viz_type(self, viz_type: str) -> str | None: + return display_name_for_viz_type(viz_type) + + +_PROXY = _RegistryProxy() Review Comment: Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous. ########## superset/advanced_data_type/plugins/internet_address.py: ########## @@ -66,9 +66,9 @@ def cidr_func(req: AdvancedDataTypeRequest) -> AdvancedDataTypeResponse: else: resp["display_value"] = ", ".join( map( # noqa: C417 - lambda x: ( - f"{x['start']} - {x['end']}" if isinstance(x, dict) else str(x) - ), + lambda x: f"{x['start']} - {x['end']}" + if isinstance(x, dict) + else str(x), Review Comment: Declining — `superset/advanced_data_type/plugins/internet_address.py` is not modified by this PR; the suggestion targets pre-existing code in master. Out of scope. ########## superset/mcp_service/chart/plugin.py: ########## @@ -0,0 +1,262 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +ChartTypePlugin protocol and BaseChartPlugin base class. + +Each chart type owns its pre-validation, column extraction, form_data mapping, +and post-map validation in a single plugin class. This eliminates the previous +pattern of 4 separate dispatch points (schema_validator.py, dataset_validator.py, +chart_utils.py, pipeline.py) that had to be updated in sync whenever a new chart +type was added. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar, Protocol, runtime_checkable + +from superset.mcp_service.chart.schemas import ColumnRef +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +@runtime_checkable +class ChartTypePlugin(Protocol): + """ + Protocol that every chart-type plugin must satisfy. + + Implementing all eight methods in a single class guarantees that adding a + new chart type requires only one new file — the plugin — rather than edits + across multiple separate files. Review Comment: Good catch — fixed in `eec49674700`. The Protocol defines 9 methods (`pre_validate`, `extract_column_refs`, `to_form_data`, `post_map_validate`, `normalize_column_refs`, `get_runtime_warnings`, `generate_name`, `resolve_viz_type`, `schema_error_hint`). Changed 'eight' to 'nine'. -- 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]
