aminghadersohi commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3314807327
##########
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:
Valid point. Test coverage for the column normalization path in
`update_chart.py` (lines 416-432) will be tracked as follow-up work — adding
those tests here would broaden the scope of this PR past the registry refactor.
##########
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:
By design — `pre_validate` runs on the raw `dict` before Pydantic parsing
and gives early fast-fail feedback to LLM clients with actionable error
messages. `HandlebarsChartConfig.validate_query_fields()` runs after Pydantic
parsing on the typed model. The two layers serve different purposes:
`pre_validate` catches structural issues early (missing keys, wrong types) with
specific guidance, while the schema validator enforces cross-field invariants
on the parsed model.
--
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]
