bito-code-review[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3314807078
########## superset/mcp_service/chart/registry.py: ########## @@ -0,0 +1,256 @@ +# 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: + _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: Review Comment: <!-- Bito Reply --> The suggestion in the PR comment is valid and appropriate. It addresses a potential issue with the current exception handling in the code. The suggestion replaces a broad `except Exception:` catch with a more specific exception type, which improves code reliability and maintainability. This change ensures that only the intended exceptions are caught, preventing unintended behavior from unhandled errors. **superset/mcp_service/chart/registry.py** ``` try: return bool(config.enabled_func(chart_type)) except Exception as e: # Handle specific exceptions only pass ``` ########## superset/mcp_service/chart/plugins/mixed_timeseries.py: ########## @@ -0,0 +1,170 @@ +# 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. + +"""Mixed timeseries chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _mixed_timeseries_what, + _summarize_filters, + map_mixed_timeseries_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, MixedTimeseriesChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class MixedTimeseriesChartPlugin(BaseChartPlugin): + """Plugin for mixed_timeseries chart type.""" + + chart_type = "mixed_timeseries" + display_name = "Mixed Timeseries" + native_viz_types = { + "mixed_timeseries": "Mixed Timeseries Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "x" not in config and "x_axis" not in config: + missing_fields.append("'x' (X-axis temporal column)") + if "y" not in config and "metrics" not in config: + missing_fields.append("'y' (primary Y-axis metrics)") + if "y_secondary" not in config and "metrics_b" not in config: + missing_fields.append("'y_secondary' (secondary Y-axis metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_mixed_timeseries_fields", + message=( + f"Mixed timeseries chart missing required fields: " + f"{', '.join(missing_fields)}" + ), + details=( + "Mixed timeseries charts require an x-axis, primary metrics, " + "and secondary metrics" + ), + suggestions=[ + "Add 'x' field: {'name': 'date_column'}", + "Add 'y' field: [{'name': 'revenue', 'aggregate': 'SUM'}]", + "Add 'y_secondary': [{'name': 'orders', 'aggregate': 'COUNT'}]", + "Optional: 'primary_kind' and 'secondary_kind' for chart types", + ], Review Comment: <!-- Bito Reply --> The PR comment suggests that user-facing strings in error messages and schema_error_hint should be wrapped with a translation function (e.g., t()) to support multi-language environments. This is a valid suggestion as it aligns with the rule for internationalization (i18n) in the project. -- 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]
