codeant-ai-for-open-source[bot] commented on code in PR #38402:
URL: https://github.com/apache/superset/pull/38402#discussion_r2930720646
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -916,12 +958,24 @@ def _mixed_timeseries_what(config:
MixedTimeseriesChartConfig) -> str:
return f"{primary} + {secondary}"
+def _handlebars_chart_what(config: HandlebarsChartConfig) -> str:
+ """Build the 'what' portion for a handlebars chart name."""
+ if config.query_mode == "raw" and config.columns:
+ cols = ", ".join(col.name for col in config.columns[:3])
+ return f"Handlebars – {cols}"
+ elif config.metrics:
+ metrics = ", ".join(col.name for col in config.metrics[:3])
+ return f"Handlebars – {metrics}"
Review Comment:
**Suggestion:** The chart-name fragment for Handlebars already contains an
en-dash delimiter, but `generate_chart_name` and `_truncate` also use the same
delimiter to append/split context. This can produce malformed names and
incorrect truncation when filters are present. Use a different separator in the
Handlebars-specific fragment to avoid delimiter collisions. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Saved chart names lose metric/column specificity.
- ⚠️ Preview alt_text becomes less descriptive for users.
- ⚠️ Chart management UX worsens with ambiguous names.
```
</details>
```suggestion
if config.query_mode == "raw" and config.columns:
cols = ", ".join(col.name for col in config.columns[:3])
return f"Handlebars: {cols}"
elif config.metrics:
metrics = ", ".join(col.name for col in config.metrics[:3])
return f"Handlebars: {metrics}"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP tool `generate_chart` (entrypoint
`superset/mcp_service/chart/tool/generate_chart.py:123`) with a Handlebars
config
(`chart_type=\"handlebars\"`) and long column/metric names plus filters;
this is valid
because `ColumnRef.name` allows up to 255 chars and Handlebars supports
filters
(`superset/mcp_service/chart/schemas.py:60-66`, `:667-720`).
2. In the normal chart-creation flow, `generate_chart` computes the name via
`generate_chart_name(request.config, ...)`
(`superset/mcp_service/chart/tool/generate_chart.py:347`), which calls
`_handlebars_chart_what()` and returns a base string already containing `" –
"`
(`superset/mcp_service/chart/chart_utils.py:961-968`).
3. `generate_chart_name` then appends context using the same delimiter
(`name = f\"{what}
– {context}\"` at `superset/mcp_service/chart/chart_utils.py:1015-1017`),
producing names
with multiple `" – "` separators.
4. When name length exceeds 60, `_truncate()` splits at the first `" – "`
(`superset/mcp_service/chart/chart_utils.py:872-880`), so Handlebars names
can collapse to
just `"Handlebars"` and lose identifying details; that degraded name is
persisted/returned
in user-facing flows (`slice_name` in create payload at
`generate_chart.py:347-343/37`,
and also used in preview/update metadata at `update_chart_preview.py:98-103`
and
`update_chart.py:142,170`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/chart_utils.py
**Line:** 963:968
**Comment:**
*Logic Error: The chart-name fragment for Handlebars already contains
an en-dash delimiter, but `generate_chart_name` and `_truncate` also use the
same delimiter to append/split context. This can produce malformed names and
incorrect truncation when filters are present. Use a different separator in the
Handlebars-specific fragment to avoid delimiter collisions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=c0645c2adbab82153a665b9fcfb8f2971d2f246ca565908f4ce48fd39759b976&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=c0645c2adbab82153a665b9fcfb8f2971d2f246ca565908f4ce48fd39759b976&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -664,6 +664,88 @@ class MixedTimeseriesChartConfig(BaseModel):
filters: List[FilterConfig] | None = Field(None, description="Filters to
apply")
+class HandlebarsChartConfig(BaseModel):
+ model_config = ConfigDict(extra="forbid")
+
+ chart_type: Literal["handlebars"] = Field(
+ ...,
+ description=(
+ "Chart type discriminator - MUST be 'handlebars' for custom HTML "
+ "template charts. Handlebars charts render query results using "
+ "Handlebars templates, enabling fully custom layouts like KPI
cards, "
+ "leaderboards, and formatted reports."
+ ),
+ )
+ handlebars_template: str = Field(
+ ...,
+ description=(
+ "Handlebars HTML template string. Data is available as {{data}}
array. "
+ "Built-in helpers: {{dateFormat val format='YYYY-MM-DD'}}, "
+ "{{formatNumber val}}, {{stringify obj}}. "
+ "Example: '<ul>{{#each data}}<li>{{this.name}}:
{{this.value}}</li>"
+ "{{/each}}</ul>'"
+ ),
+ min_length=1,
+ max_length=50000,
+ )
+ query_mode: Literal["aggregate", "raw"] = Field(
+ "aggregate",
+ description=(
+ "Query mode: 'aggregate' groups data with metrics, "
+ "'raw' returns individual rows"
+ ),
+ )
+ columns: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Columns to display in raw mode (query_mode='raw'). "
+ "Each column specifies a column name to include in the query
results."
+ ),
+ )
+ groupby: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Columns to group by in aggregate mode (query_mode='aggregate'). "
+ "These become the dimensions for aggregation."
+ ),
+ )
+ metrics: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Metrics to aggregate in aggregate mode. "
+ "Each must have an aggregate function (e.g., SUM, COUNT)."
+ ),
+ )
+ filters: list[FilterConfig] | None = Field(None, description="Filters to
apply")
+ row_limit: int = Field(
+ 1000,
+ description="Maximum number of rows",
+ ge=1,
+ le=50000,
+ )
+ order_desc: bool = Field(True, description="Sort in descending order")
+ style_template: str | None = Field(
+ None,
+ description="Optional CSS styles to apply to the rendered template",
+ max_length=10000,
+ )
+
+ @model_validator(mode="after")
+ def validate_query_fields(self) -> "HandlebarsChartConfig":
+ """Validate that the right fields are provided for the query mode."""
+ if self.query_mode == "raw" and not self.columns:
+ raise ValueError(
+ "Handlebars chart in 'raw' query mode requires 'columns'
field. "
+ "Specify which columns to include in the query results."
+ )
+ if self.query_mode == "aggregate" and not self.metrics:
+ raise ValueError(
+ "Handlebars chart in 'aggregate' query mode requires 'metrics'
field. "
+ "Specify at least one metric with an aggregate function."
+ )
Review Comment:
**Suggestion:** Aggregate-mode metrics are only checked for presence, not
for an aggregate function. Because downstream mapping defaults missing
aggregates to `SUM`, non-numeric metric columns can generate invalid SQL at
runtime. Validate that every metric in aggregate mode includes an explicit
aggregate. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Handlebars aggregate config silently changes metric semantics.
- ❌ Non-numeric SUM metrics can fail chart compilation.
- ⚠️ Affects generate_chart, update_chart, generate_explore_link flows.
```
</details>
```suggestion
if self.query_mode == "aggregate":
if not self.metrics:
raise ValueError(
"Handlebars chart in 'aggregate' query mode requires
'metrics' field. "
"Specify at least one metric with an aggregate function."
)
if any(metric.aggregate is None for metric in self.metrics):
raise ValueError(
"Handlebars chart metrics in 'aggregate' query mode must
include "
"an explicit 'aggregate' function for each metric."
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP `generate_chart`
(`superset/mcp_service/chart/tool/generate_chart.py:121-125`)
with `config.chart_type="handlebars"`, `query_mode="aggregate"`, and
`metrics=[{"name":"product_name"}]` (no `aggregate`).
2. Request parsing via `@parse_request(GenerateChartRequest)`
(`generate_chart.py:122`)
builds `HandlebarsChartConfig`; `validate_query_fields()` in
`superset/mcp_service/chart/schemas.py:734-746` only checks metrics
presence, so this
payload passes.
3. `generate_chart()` maps config through `map_config_to_form_data()`
(`generate_chart.py:252-254`) → `map_handlebars_config()`
(`superset/mcp_service/chart/chart_utils.py:638-660`) →
`create_metric_object()`
(`chart_utils.py:414-444`), which silently sets missing aggregates to `SUM`
(`aggregate =
col.aggregate or "SUM"`).
4. Resulting query semantics differ from submitted config; and for
non-numeric columns
this can fail at compile/query time (`_compile_chart()` in
`generate_chart.py:63-118`)
because Handlebars aggregation compatibility is not checked in dataset
validation
(`dataset_validator.py:106-112` only handles `TableChartConfig |
XYChartConfig`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 741:745
**Comment:**
*Logic Error: Aggregate-mode metrics are only checked for presence, not
for an aggregate function. Because downstream mapping defaults missing
aggregates to `SUM`, non-numeric metric columns can generate invalid SQL at
runtime. Validate that every metric in aggregate mode includes an explicit
aggregate.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=d0df770d04817ca128c47f4e1dc8378fb14d48e5091181bc0d7a40dd469a1800&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=d0df770d04817ca128c47f4e1dc8378fb14d48e5091181bc0d7a40dd469a1800&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -664,6 +664,88 @@ class MixedTimeseriesChartConfig(BaseModel):
filters: List[FilterConfig] | None = Field(None, description="Filters to
apply")
+class HandlebarsChartConfig(BaseModel):
+ model_config = ConfigDict(extra="forbid")
+
+ chart_type: Literal["handlebars"] = Field(
+ ...,
+ description=(
+ "Chart type discriminator - MUST be 'handlebars' for custom HTML "
+ "template charts. Handlebars charts render query results using "
+ "Handlebars templates, enabling fully custom layouts like KPI
cards, "
+ "leaderboards, and formatted reports."
+ ),
+ )
+ handlebars_template: str = Field(
+ ...,
+ description=(
+ "Handlebars HTML template string. Data is available as {{data}}
array. "
+ "Built-in helpers: {{dateFormat val format='YYYY-MM-DD'}}, "
+ "{{formatNumber val}}, {{stringify obj}}. "
+ "Example: '<ul>{{#each data}}<li>{{this.name}}:
{{this.value}}</li>"
+ "{{/each}}</ul>'"
+ ),
+ min_length=1,
+ max_length=50000,
+ )
+ query_mode: Literal["aggregate", "raw"] = Field(
+ "aggregate",
+ description=(
+ "Query mode: 'aggregate' groups data with metrics, "
+ "'raw' returns individual rows"
+ ),
+ )
+ columns: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Columns to display in raw mode (query_mode='raw'). "
+ "Each column specifies a column name to include in the query
results."
+ ),
+ )
+ groupby: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Columns to group by in aggregate mode (query_mode='aggregate'). "
+ "These become the dimensions for aggregation."
+ ),
+ )
+ metrics: list[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Metrics to aggregate in aggregate mode. "
+ "Each must have an aggregate function (e.g., SUM, COUNT)."
+ ),
+ )
+ filters: list[FilterConfig] | None = Field(None, description="Filters to
apply")
+ row_limit: int = Field(
+ 1000,
+ description="Maximum number of rows",
+ ge=1,
+ le=50000,
+ )
+ order_desc: bool = Field(True, description="Sort in descending order")
+ style_template: str | None = Field(
+ None,
+ description="Optional CSS styles to apply to the rendered template",
+ max_length=10000,
+ )
+
+ @model_validator(mode="after")
+ def validate_query_fields(self) -> "HandlebarsChartConfig":
+ """Validate that the right fields are provided for the query mode."""
+ if self.query_mode == "raw" and not self.columns:
+ raise ValueError(
+ "Handlebars chart in 'raw' query mode requires 'columns'
field. "
+ "Specify which columns to include in the query results."
+ )
Review Comment:
**Suggestion:** Raw mode currently accepts `metrics` and `groupby` but
silently ignores them during form-data mapping, which can produce a chart that
does not match the submitted config. Reject these aggregate-only fields when
`query_mode` is `raw` to prevent incorrect behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Raw-mode payload accepts unsupported aggregate-only fields.
- ❌ Returned form_data ignores submitted metrics/groupby silently.
- ⚠️ Confuses users across explore and chart update flows.
```
</details>
```suggestion
if self.query_mode == "raw":
if not self.columns:
raise ValueError(
"Handlebars chart in 'raw' query mode requires 'columns'
field. "
"Specify which columns to include in the query results."
)
if self.metrics or self.groupby:
raise ValueError(
"Handlebars chart in 'raw' query mode does not support
'metrics' "
"or 'groupby'. Use 'columns' only, or switch to
'aggregate' mode."
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call `generate_explore_link`
(`superset/mcp_service/explore/tool/generate_explore_link.py:42-46`) with
`chart_type="handlebars"`, `query_mode="raw"`, valid `columns`, plus extra
`metrics`/`groupby`.
2. `HandlebarsChartConfig.validate_query_fields()`
(`superset/mcp_service/chart/schemas.py:734-746`) accepts this because raw
mode currently
only requires `columns`.
3. Mapping in `map_handlebars_config()`
(`superset/mcp_service/chart/chart_utils.py:650-659`) takes raw branch and
only emits
`all_columns`; `metrics` and `groupby` are ignored.
4. Tool returns `form_data` (`generate_explore_link.py:153-157`) that does
not reflect
submitted config, producing a silent user-visible mismatch between requested
and executed
query settings.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 736:740
**Comment:**
*Logic Error: Raw mode currently accepts `metrics` and `groupby` but
silently ignores them during form-data mapping, which can produce a chart that
does not match the submitted config. Reject these aggregate-only fields when
`query_mode` is `raw` to prevent incorrect behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=c20ccb84b841e2e78422599be2be3a7ece1c32b793b07c36aa3f5316511559bd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=c20ccb84b841e2e78422599be2be3a7ece1c32b793b07c36aa3f5316511559bd&reaction=dislike'>👎</a>
--
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]
