aminghadersohi commented on code in PR #44152:
URL: https://github.com/apache/superset/pull/44152#discussion_r4046255250
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2925,7 +2991,7 @@ class UpdateChartRequest(ChartRequestNormalizerMixin,
QueryCacheControl):
description="Chart ID or UUID",
validation_alias=AliasChoices("identifier", "id", "chart_id"),
)
- config: ChartConfig | None = Field(
+ config: ChartConfig | TreemapChartUpdateConfig | None = Field(
Review Comment:
Confirmed and fixed in fcf5905. You were right — I reproduced it:
`UpdateChartRequest.model_validate({"identifier": 1, "config": {"row_limit":
50}})` parsed as a `TreemapChartUpdateConfig` with `chart_type="treemap_v2"`,
so a partial update against an existing pie or table chart would have rewritten
its `viz_type` and params. Because the model also uses `extra="ignore"`,
essentially any object validated.
`TreemapChartUpdateConfig.chart_type` is now required (`Field(...)`), which
is the option you suggested. `TreemapChartConfig` re-declares it with the
default, since that subclass is only reached through the discriminated
`ChartConfig` union — which already requires the key in client payloads — and
internal call sites construct it directly.
Both request models now reject a config without the discriminator, while a
partial Treemap update that does carry `chart_type` still works and still
records only the fields the caller set. Covered by
`test_update_request_rejects_config_without_discriminator` and
`test_update_request_still_accepts_partial_treemap_with_discriminator`.
##########
superset/mcp_service/chart/query_result.py:
##########
@@ -223,3 +225,83 @@ def validate_gauge_query_result(
"""Check Gauge results using the same finite-dial contract as rendering."""
normalized = normalize_gauge_query_result(result, form_data)
return normalized if isinstance(normalized, ChartError) else None
+
+
+def normalize_chart_query_result(result: Any, form_data: Mapping[str, Any]) ->
Any:
+ """Validate chart-specific result contracts before consumers use rows."""
+ if form_data.get("viz_type") != "treemap_v2":
+ return normalize_gauge_query_result(result, form_data)
+ if failure := query_result_failure(result):
+ return failure
+ label = metric_result_label(form_data.get("metric"))
+ hierarchy = form_data.get("groupby")
+ if (
+ not label
+ or not isinstance(hierarchy, list)
+ or not hierarchy
+ or not all(isinstance(column, str) and column for column in hierarchy)
Review Comment:
Confirmed and fixed in fcf5905 by normalizing rather than rejecting, as you
suggested.
`normalize_chart_query_result` now resolves each `groupby` entry through a
new `column_result_label` helper that mirrors the frontend `getColumnLabel`
rules exactly — physical string columns pass through, otherwise `label`,
otherwise `sqlExpression` — so an adhoc Custom SQL column is matched against
the output key its rows are actually keyed by.
`treemap_preview.py` needed the same treatment: `treemap_ascii` and
`treemap_vega_lite` were indexing rows with the raw `form_data["groupby"]`
entries, which would have raised on a dict even once validation passed. Both
now go through the shared `treemap_hierarchy_labels` helper.
The uniqueness contract is preserved on the resolved labels rather than the
raw entries, so two columns that collapse onto the same output label are still
rejected, as are entries that resolve to nothing. Covered by
`test_adhoc_hierarchy_column_is_normalized_to_its_output_label`,
`test_adhoc_hierarchy_column_without_label_falls_back_to_sql`,
`test_adhoc_hierarchy_previews_render_with_resolved_labels` and
`test_unresolvable_or_duplicate_hierarchy_columns_stay_rejected`.
--
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]
