codeant-ai-for-open-source[bot] commented on code in PR #38402:
URL: https://github.com/apache/superset/pull/38402#discussion_r2943417091
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -590,6 +590,108 @@ class MixedTimeseriesChartConfig(BaseModel):
filters: List[FilterConfig] | None = None
+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":
+ 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:
+ raise ValueError(
+ "Handlebars chart in 'raw' query mode does not use
'metrics'. "
+ "Remove 'metrics' or switch to 'aggregate' query mode."
+ )
+ if self.groupby:
+ raise ValueError(
+ "Handlebars chart in 'raw' query mode does not use
'groupby'. "
+ "Remove 'groupby' or switch to 'aggregate' query mode."
+ )
+ 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."
+ )
+ missing_agg = [m.name for m in self.metrics if not m.aggregate]
+ if missing_agg:
+ raise ValueError(
+ f"Handlebars chart in 'aggregate' query mode requires an "
+ f"aggregate function on every metric. Missing aggregate
for: "
+ f"{', '.join(missing_agg)}. "
+ f"Use one of: SUM, COUNT, AVG, MIN, MAX, COUNT_DISTINCT,
etc."
+ )
Review Comment:
**Suggestion:** In `aggregate` mode, `columns` are currently accepted but
silently ignored by downstream form_data mapping, which can make templates
reference fields that never arrive in query results. Reject `columns` in
aggregate mode to prevent incorrect charts caused by dropped inputs. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Handlebars aggregate configs silently drop user-provided columns.
- ❌ Template fields can be missing in rendered output.
- ⚠️ Affects generate_chart, update_chart, update_chart_preview flows.
```
</details>
```suggestion
if self.query_mode == "aggregate":
if self.columns:
raise ValueError(
"Handlebars chart in 'aggregate' query mode does not use
'columns'. "
"Remove 'columns' or switch to 'raw' query mode."
)
if not self.metrics:
raise ValueError(
"Handlebars chart in 'aggregate' query mode requires
'metrics' "
"field. Specify at least one metric with an aggregate
function."
)
missing_agg = [m.name for m in self.metrics if not m.aggregate]
if missing_agg:
raise ValueError(
f"Handlebars chart in 'aggregate' query mode requires an
"
f"aggregate function on every metric. Missing aggregate
for: "
f"{', '.join(missing_agg)}. "
f"Use one of: SUM, COUNT, AVG, MIN, MAX, COUNT_DISTINCT,
etc."
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP chart tools with handlebars aggregate config including `columns`
(accepted
request model paths: `generate_chart.py:123`, `update_chart.py:49`,
`update_chart_preview.py:48`; all use `@parse_request(...)` from
`schema_utils.py:404`).
2. Validation passes because `HandlebarsChartConfig.validate_query_fields()`
in
`schemas.py:678-691` enforces metrics/aggregates but does not reject
`columns` for
aggregate mode.
3. Execution reaches form-data mapping (`generate_chart.py:253`,
`update_chart.py:132`,
`update_chart_preview.py:72`) and dispatches to `map_handlebars_config()` via
`chart_utils.py:307-328`.
4. In `chart_utils.py:662-667` aggregate branch sets
`query_mode/groupby/metrics` only;
`columns` are ignored (raw-only mapping exists at `chart_utils.py:658-661`),
so template
expectations based on provided columns are silently unmet.
```
</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:** 678:691
**Comment:**
*Logic Error: In `aggregate` mode, `columns` are currently accepted but
silently ignored by downstream form_data mapping, which can make templates
reference fields that never arrive in query results. Reject `columns` in
aggregate mode to prevent incorrect charts caused by dropped inputs.
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=21ffc96f1c1e1b76fb538784a1d097dbbb8644c3286712bd3d38a9a95662affa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38402&comment_hash=21ffc96f1c1e1b76fb538784a1d097dbbb8644c3286712bd3d38a9a95662affa&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]
