bito-code-review[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3314808659
########## 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/tool/update_chart.py: ########## @@ -390,6 +413,24 @@ async def update_chart( # noqa: C901 # config is already a typed ChartConfig | None (validated by Pydantic) parsed_config = request.config + # Normalize column case to match dataset canonical names + # (mirrors generate_chart pipeline layer 4) + chart_datasource_id = getattr(chart, "datasource_id", None) + if parsed_config is not None and chart_datasource_id is not None: + from superset.mcp_service.chart.validation.dataset_validator import ( + DatasetValidator, + NORMALIZATION_EXCEPTIONS, + ) + + try: + parsed_config = DatasetValidator.normalize_column_names( + parsed_config, chart.datasource_id + ) + except NORMALIZATION_EXCEPTIONS as e: + logger.warning( + "Column normalization failed for chart %s: %s", chart.id, e + ) Review Comment: <!-- Bito Reply --> The suggestion in the PR comment is valid and appropriate to apply. It correctly identifies a gap in test coverage for the new column normalization logic in the `update_chart` function. The suggestion recommends adding unit tests to verify both the normalization of column names and the handling of exceptions during normalization, which are critical for ensuring the reliability and correctness of the feature. Implementing these tests in the `TestUpdateChartValidationGate` suite, as suggested, is a logical and maintainable approach. -- 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]
