bito-code-review[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3305706254
########## superset/mcp_service/chart/plugins/handlebars.py: ########## @@ -0,0 +1,193 @@ +# 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. + +"""Handlebars chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _handlebars_chart_what, + _summarize_filters, + map_handlebars_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, HandlebarsChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class HandlebarsChartPlugin(BaseChartPlugin): + """Plugin for handlebars chart type (custom HTML template charts).""" + + chart_type = "handlebars" + display_name = "Handlebars (Custom Template)" + native_viz_types = { + "handlebars": "Custom Template Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + if "handlebars_template" not in config: + return 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 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 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 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'", # noqa: E501 + ], + error_code="MISSING_RAW_COLUMNS", + ) + + if query_mode == "aggregate" and not config.get("metrics"): + return 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 None Review Comment: <!-- Bito Reply --> The `pre_validate` method in the `HandlebarsChartPlugin` class performs initial validation of the chart configuration before Pydantic parsing. It checks for the presence of required fields like `handlebars_template`, validates the `query_mode`, and ensures that either `columns` or `metrics` are provided based on the mode. This allows for early detection of common mistakes and provides structured error messages. **Interaction Diagram by [Bito](https://bito.ai/#sequence_diagram)** ```mermaid sequenceDiagram participant User participant pre_validate User->>pre_validate: Call pre_validate with config pre_validate->>pre_validate: Check for handlebars_template pre_validate->>pre_validate: Check query_mode pre_validate->>pre_validate: Validate columns/metrics based on mode pre_validate-->>User: Return validation result ``` **superset/mcp_service/chart/plugins/handlebars.py** ``` def pre_validate( self, config: dict[str, Any], ) -> ChartGenerationError | None: if "handlebars_template" not in config: return ChartGenerationError(...) template = config.get("handlebars_template") if not isinstance(template, str) or not template.strip(): return ChartGenerationError(...) query_mode = config.get("query_mode", "aggregate") if query_mode not in ("aggregate", "raw"): return ChartGenerationError(...) if query_mode == "raw" and not config.get("columns"): return ChartGenerationError(...) if query_mode == "aggregate" and not config.get("metrics"): return ChartGenerationError(...) return None ``` ########## tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py: ########## @@ -226,28 +220,15 @@ def test_validate_table_chart_skips_xy_validations(self): ], ) - # These should not be called for table charts - with ( - patch( - "superset.mcp_service.chart.validation.runtime.RuntimeValidator." - "_validate_format_compatibility" - ) as mock_format, - patch( - "superset.mcp_service.chart.validation.runtime.RuntimeValidator." - "_validate_cardinality" - ) as mock_cardinality, - patch( - "superset.mcp_service.chart.validation.runtime.RuntimeValidator." - "_validate_chart_type" - ) as mock_chart_type, - ): - # Mock chart type validator to return no warnings + # Plugin runtime dispatches to TableChartPlugin which returns no warnings. + # Chart type suggester is also stubbed to return no warnings. + with patch( + "superset.mcp_service.chart.validation.runtime.RuntimeValidator." + "_validate_chart_type" + ) as mock_chart_type: mock_chart_type.return_value = ([], []) is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) - # Format and cardinality validation should not be called for table charts - mock_format.assert_not_called() - mock_cardinality.assert_not_called() assert is_valid is True assert error is None Review Comment: <!-- Bito Reply --> The suggestion in the PR comment is asking whether the test `test_validate_table_chart_skips_xy_validations` has a coverage regression. The original test used to verify that XY-specific validations are skipped by asserting that `mock_format` and `mock_cardinality` are not called. However, in the refactored implementation, these assertions were removed, and the test now only checks the `mock_chart_type` return value. This change weakens the test coverage for ensuring that XY-specific validations are skipped for table charts. The suggestion is valid and highlights a potential regression in test coverage. ########## superset/mcp_service/chart/validation/runtime/__init__.py: ########## @@ -98,61 +85,28 @@ def validate_runtime_issues( return True, None @staticmethod - def _validate_format_compatibility(config: XYChartConfig) -> List[str]: - """Validate format-type compatibility.""" - warnings: List[str] = [] - - try: - # Import here to avoid circular imports - from .format_validator import FormatTypeValidator - - is_valid, format_warnings = ( - FormatTypeValidator.validate_format_compatibility(config) - ) - if format_warnings: - warnings.extend(format_warnings) - except ImportError: - logger.warning("Format validator not available") - except Exception as e: - logger.warning("Format validation failed: %s", e) - - return warnings - - @staticmethod - def _validate_cardinality( - config: XYChartConfig, dataset_id: int | str - ) -> Tuple[List[str], List[str]]: - """Validate cardinality issues.""" - warnings: List[str] = [] - suggestions: List[str] = [] + def _validate_plugin_runtime( + config: ChartConfig, dataset_id: int | str + ) -> List[str]: + """Delegate per-chart-type runtime warnings to the plugin registry. + Each plugin's get_runtime_warnings() method returns chart-type-specific + warnings (e.g. format/cardinality for XY). The registry dispatch removes + the previous isinstance(config, XYChartConfig) hardcoding. + """ try: - # Import here to avoid circular imports - from .cardinality_validator import CardinalityValidator - - # Determine chart type for cardinality thresholds - chart_type = config.kind if hasattr(config, "kind") else "default" - - # Check X-axis cardinality - if config.x is None: - return warnings, suggestions - is_ok, cardinality_info = CardinalityValidator.check_cardinality( - dataset_id=dataset_id, - x_column=config.x.name, - chart_type=chart_type, - group_by_column=config.group_by[0].name if config.group_by else None, - ) - - if not is_ok and cardinality_info: - warnings.extend(cardinality_info.get("warnings", [])) - suggestions.extend(cardinality_info.get("suggestions", [])) - - except ImportError: - logger.warning("Cardinality validator not available") - except Exception as e: - logger.warning("Cardinality validation failed: %s", e) - - return warnings, suggestions + from superset.mcp_service.chart.registry import get_registry + + chart_type = getattr(config, "chart_type", None) + if chart_type is None: + return [] + plugin = get_registry().get(chart_type) + if plugin is None: + return [] + return plugin.get_runtime_warnings(config, dataset_id) + except Exception as exc: Review Comment: <!-- Bito Reply --> The suggestion in the review comment is valid and appropriate to apply. It recommends catching specific exception types (`ImportError`, `AttributeError`, `RuntimeError`) instead of a broad `Exception` to avoid masking unexpected errors. This improves error handling by making it more explicit and targeted, which aligns with best practices in Python development. **superset/mcp_service/chart/validation/runtime/__init__.py** ``` except (ImportError, AttributeError, RuntimeError) as exc: ``` -- 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]
