aminghadersohi commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3203063269
########## superset/mcp_service/chart/registry.py: ########## @@ -0,0 +1,112 @@ +# 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 +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from superset.mcp_service.chart.plugin import ChartTypePlugin + +logger = logging.getLogger(__name__) + +_REGISTRY: dict[str, "ChartTypePlugin"] = {} + + +def register(plugin: "ChartTypePlugin") -> None: + """Register a chart type plugin in the global registry.""" + if plugin.chart_type in _REGISTRY: + logger.warning( + "Overwriting existing plugin for chart_type=%r", plugin.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 a given chart_type, or None if not registered.""" + return _REGISTRY.get(chart_type) Review Comment: Addressed in c716a036 — `registry.py` now has `_ensure_plugins_loaded()` called on every lookup (`get()` and `display_name_for_viz_type()`). It lazily imports `superset.mcp_service.chart.plugins` on first access, so the registry is populated even in isolated unit tests that never import `app.py`. ########## superset/mcp_service/chart/validation/schema_validator.py: ########## @@ -147,19 +147,13 @@ def _pre_validate_chart_type( chart_type: str, config: Dict[str, Any], ) -> Tuple[bool, ChartGenerationError | None]: - """Validate chart type and dispatch to type-specific pre-validation.""" - chart_type_validators = { - "xy": SchemaValidator._pre_validate_xy_config, - "table": SchemaValidator._pre_validate_table_config, - "pie": SchemaValidator._pre_validate_pie_config, - "pivot_table": SchemaValidator._pre_validate_pivot_table_config, - "mixed_timeseries": SchemaValidator._pre_validate_mixed_timeseries_config, - "handlebars": SchemaValidator._pre_validate_handlebars_config, - "big_number": SchemaValidator._pre_validate_big_number_config, - } + """Validate chart type and dispatch to plugin pre-validation.""" + from superset.mcp_service.chart.registry import get_registry - if not isinstance(chart_type, str) or chart_type not in chart_type_validators: - valid_types = ", ".join(chart_type_validators.keys()) + registry = get_registry() + + if not isinstance(chart_type, str) or not registry.is_registered(chart_type): + valid_types = ", ".join(registry.all_types()) return False, ChartGenerationError( Review Comment: Handled by `_ensure_plugins_loaded()` (added in c716a036). The lazy bootstrap fires on every registry lookup, so `valid_types` is always populated before the check runs — no cold-start rejection possible. ########## superset/mcp_service/chart/validation/schema_validator.py: ########## @@ -178,352 +172,127 @@ def _pre_validate_chart_type( error_code="INVALID_CHART_TYPE", ) - return chart_type_validators[chart_type](config) - - @staticmethod - def _pre_validate_xy_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate XY chart configuration.""" - # x is optional — defaults to dataset's main_dttm_col in map_xy_config - if "y" not in config: - return False, ChartGenerationError( - error_type="missing_xy_fields", - message="XY chart missing required field: 'y' (Y-axis metrics)", - details="XY charts require Y-axis (metrics) specifications. " - "X-axis is optional and defaults to the dataset's primary " - "datetime column when omitted.", - suggestions=[ - "Add 'y' field: [{'name': 'metric_column', 'aggregate': 'SUM'}] " - "for Y-axis", - "Example: {'chart_type': 'xy', 'x': {'name': 'date'}, " - "'y': [{'name': 'sales', 'aggregate': 'SUM'}]}", - ], - error_code="MISSING_XY_FIELDS", - ) - - # Validate Y is a list - if not isinstance(config.get("y", []), list): + plugin = registry.get(chart_type) + if plugin is None: return False, ChartGenerationError( - error_type="invalid_y_format", - message="Y-axis must be a list of metrics", - details="The 'y' field must be an array of metric specifications", - suggestions=[ - "Wrap Y-axis metric in array: 'y': [{'name': 'column', " - "'aggregate': 'SUM'}]", - "Multiple metrics supported: 'y': [metric1, metric2, ...]", - ], - error_code="INVALID_Y_FORMAT", - ) - - return True, None - - @staticmethod - def _pre_validate_table_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate table chart configuration.""" - if "columns" not in config: - return False, ChartGenerationError( - error_type="missing_columns", - message="Table chart missing required field: columns", - details="Table charts require a 'columns' array to specify which " - "columns to display", - suggestions=[ - "Add 'columns' field with array of column specifications", - "Example: 'columns': [{'name': 'product'}, {'name': 'sales', " - "'aggregate': 'SUM'}]", - "Each column can have optional 'aggregate' for metrics", - ], - error_code="MISSING_COLUMNS", - ) - - if not isinstance(config.get("columns", []), list): - return False, ChartGenerationError( - error_type="invalid_columns_format", - message="Columns must be a list", - details="The 'columns' field must be an array of column specifications", - suggestions=[ - "Ensure columns is an array: 'columns': [...]", - "Each column should be an object with 'name' field", - ], - error_code="INVALID_COLUMNS_FORMAT", - ) - - return True, None - - @staticmethod - def _pre_validate_pie_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate pie chart configuration.""" - missing_fields = [] - - if "dimension" not in config: - missing_fields.append("'dimension' (category column for slices)") - if "metric" not in config: - missing_fields.append("'metric' (value metric for slice sizes)") - - if missing_fields: - return False, ChartGenerationError( - error_type="missing_pie_fields", - message=f"Pie chart missing required " - f"fields: {', '.join(missing_fields)}", - details="Pie charts require a dimension (categories) and a metric " - "(values)", - suggestions=[ - "Add 'dimension' field: {'name': 'category_column'}", - "Add 'metric' field: {'name': 'value_column', 'aggregate': 'SUM'}", - "Example: {'chart_type': 'pie', 'dimension': {'name': " - "'product'}, 'metric': {'name': 'revenue', 'aggregate': 'SUM'}}", - ], - error_code="MISSING_PIE_FIELDS", - ) - - return True, None - - @staticmethod - def _pre_validate_handlebars_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate handlebars chart configuration.""" - if "handlebars_template" not in config: - return False, ChartGenerationError( - error_type="missing_handlebars_template", - message="Handlebars chart missing required field: handlebars_template", - details="Handlebars charts require a 'handlebars_template' string " - "containing Handlebars HTML template markup", - suggestions=[ - "Add 'handlebars_template' with a Handlebars HTML template", - "Data is available as {{data}} array in the template", - "Example: '<ul>{{#each data}}<li>{{this.name}}: " - "{{this.value}}</li>{{/each}}</ul>'", - ], - error_code="MISSING_HANDLEBARS_TEMPLATE", - ) - - template = config.get("handlebars_template") - if not isinstance(template, str) or not template.strip(): - return False, ChartGenerationError( - error_type="invalid_handlebars_template", - message="Handlebars template must be a non-empty string", - details="The 'handlebars_template' field must be a non-empty string " - "containing valid Handlebars HTML template markup", - suggestions=[ - "Ensure handlebars_template is a non-empty string", - "Example: '<ul>{{#each data}}<li>{{this.name}}</li>{{/each}}</ul>'", - ], - error_code="INVALID_HANDLEBARS_TEMPLATE", - ) - - query_mode = config.get("query_mode", "aggregate") - if query_mode not in ("aggregate", "raw"): - return False, ChartGenerationError( - error_type="invalid_query_mode", - message="Invalid query_mode for handlebars chart", - details="query_mode must be either 'aggregate' or 'raw'", - suggestions=[ - "Use 'aggregate' for aggregated data (default)", - "Use 'raw' for individual rows", - ], - error_code="INVALID_QUERY_MODE", - ) - - if query_mode == "raw" and not config.get("columns"): - return False, ChartGenerationError( - error_type="missing_raw_columns", - message="Handlebars chart in 'raw' mode requires 'columns'", - details="When query_mode is 'raw', you must specify which columns " - "to include in the query results", - suggestions=[ - "Add 'columns': [{'name': 'column_name'}] for raw mode", - "Or use query_mode='aggregate' with 'metrics' " - "and optional 'groupby'", - ], - error_code="MISSING_RAW_COLUMNS", - ) - - if query_mode == "aggregate" and not config.get("metrics"): - return False, ChartGenerationError( - error_type="missing_aggregate_metrics", - message="Handlebars chart in 'aggregate' mode requires 'metrics'", - details="When query_mode is 'aggregate' (default), you must specify " - "at least one metric with an aggregate function", - suggestions=[ - "Add 'metrics': [{'name': 'column', 'aggregate': 'SUM'}]", - "Or use query_mode='raw' with 'columns' for individual rows", - ], - error_code="MISSING_AGGREGATE_METRICS", - ) - - return True, None - - @staticmethod - def _pre_validate_big_number_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate big number chart configuration.""" - if "metric" not in config: - return False, ChartGenerationError( - error_type="missing_metric", - message="Big Number chart missing required field: metric", - details="Big Number charts require a 'metric' field " - "specifying the value to display", - suggestions=[ - "Add 'metric' with name and aggregate: " - "{'name': 'revenue', 'aggregate': 'SUM'}", - "The aggregate function is required (SUM, COUNT, AVG, MIN, MAX)", - "Example: {'chart_type': 'big_number', " - "'metric': {'name': 'sales', 'aggregate': 'SUM'}}", - ], - error_code="MISSING_BIG_NUMBER_METRIC", - ) - - metric = config.get("metric", {}) - if not isinstance(metric, dict): - return False, ChartGenerationError( - error_type="invalid_metric_type", - message="Big Number metric must be a dict with 'name' and 'aggregate'", - details="The 'metric' field must be an object, " - f"got {type(metric).__name__}", - suggestions=[ - "Use a dict: {'name': 'col', 'aggregate': 'SUM'}", - "Valid aggregates: SUM, COUNT, AVG, MIN, MAX", - ], - error_code="INVALID_BIG_NUMBER_METRIC_TYPE", - ) - if not metric.get("aggregate") and not metric.get("saved_metric"): - return False, ChartGenerationError( - error_type="missing_metric_aggregate", - message="Big Number metric must include an aggregate function " - "or reference a saved metric", - details="The metric must have an 'aggregate' field " - "or 'saved_metric': true", - suggestions=[ - "Add 'aggregate' to your metric: " - "{'name': 'col', 'aggregate': 'SUM'}", - "Or use a saved metric: " - "{'name': 'total_sales', 'saved_metric': true}", - "Valid aggregates: SUM, COUNT, AVG, MIN, MAX", - ], - error_code="MISSING_BIG_NUMBER_AGGREGATE", - ) - - show_trendline = config.get("show_trendline", False) - temporal_column = config.get("temporal_column") - if show_trendline and not temporal_column: - return False, ChartGenerationError( - error_type="missing_temporal_column", - message="Trendline requires a temporal column", - details="When 'show_trendline' is True, a " - "'temporal_column' must be specified", - suggestions=[ - "Add 'temporal_column': 'date_column_name'", - "Or set 'show_trendline': false for number only", - "Use get_dataset_info to find temporal columns", - ], - error_code="MISSING_TEMPORAL_COLUMN", - ) - - return True, None - - @staticmethod - def _pre_validate_pivot_table_config( - config: Dict[str, Any], - ) -> Tuple[bool, ChartGenerationError | None]: - """Pre-validate pivot table configuration.""" - missing_fields = [] - - if "rows" not in config: - missing_fields.append("'rows' (row grouping columns)") - if "metrics" not in config: - missing_fields.append("'metrics' (aggregation metrics)") - - if missing_fields: - return False, ChartGenerationError( - error_type="missing_pivot_fields", - message=f"Pivot table missing required " - f"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", - ) - - if not isinstance(config.get("rows", []), list): - return False, ChartGenerationError( - error_type="invalid_rows_format", - message="Rows must be a list of columns", - details="The 'rows' field must be an array of column specifications", - suggestions=[ - "Wrap row columns in array: 'rows': [{'name': 'category'}]", - ], - error_code="INVALID_ROWS_FORMAT", - ) - - if not isinstance(config.get("metrics", []), list): - return False, ChartGenerationError( - error_type="invalid_metrics_format", - message="Metrics must be a list", - details="The 'metrics' field must be an array of metric specifications", - suggestions=[ - "Wrap metrics in array: 'metrics': [{'name': 'sales', " - "'aggregate': 'SUM'}]", - ], - error_code="INVALID_METRICS_FORMAT", + error_type="invalid_chart_type", + message=f"Chart type '{chart_type}' has no registered plugin", + details="Internal error: chart type is listed but has no plugin", + suggestions=["Use a supported chart_type"], + error_code="INVALID_CHART_TYPE", ) + if (error := plugin.pre_validate(config)) is not None: + return False, error return True, None Review Comment: The old per-chart `_pre_validate_*_config` helpers no longer exist. Tests were updated in c716a036 to go through the public `validate_chart_config` entry point or mock at the plugin boundary. All tests pass after the refactor. -- 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]
