bito-code-review[bot] commented on code in PR #44746:
URL: https://github.com/apache/superset/pull/44746#discussion_r4120978479
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -866,19 +867,26 @@ async def update_chart( # noqa: C901
# config is already a typed ChartConfig | None (validated by Pydantic)
try:
+ config_plugin = (
+ get_registry().get(request.config.chart_type)
+ if request.config is not None
+ else None
+ )
parsed_config = (
- resolve_treemap_update_config(
+ config_plugin.resolve_update_config(
Review Comment:
<!-- Bito Reply -->
The changes in this pull request address several issues related to plugin
handling and error validation in the chart service. Specifically, the updates
ensure that disabled registered plugins are correctly included in lookups for
saved chart updates, preserving necessary lifecycle hooks and contracts.
Additionally, the fix introduces robust error handling for row-limit
conversions and Gantt field validation, replacing generic failures with
structured validation errors. These changes are supported by new parametrized
regression tests and have passed the relevant unit and pre-commit suites.
##########
superset/mcp_service/chart/plugins/gantt.py:
##########
@@ -235,3 +243,66 @@ def schema_error_hint(self) -> ChartGenerationError | None:
],
error_code="GANTT_VALIDATION_ERROR",
)
+
+ def build_query_dicts(
+ self,
+ form_data: dict[str, Any],
+ *,
+ viz_type: str,
+ engine: str,
+ row_limit: int | None,
+ order_desc: bool | None,
+ ) -> list[dict[str, Any]] | None:
+ from superset.mcp_service.chart.chart_helpers import (
+ build_single_query_dict,
+ resolve_gantt_query_fields,
+ )
+
+ columns, metrics, orderby, series_columns = resolve_gantt_query_fields(
+ form_data
+ )
+ query = build_single_query_dict(
+ form_data, columns, metrics, row_limit=row_limit
+ )
+ query["orderby"] = orderby
+ query["series_columns"] = series_columns
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is appropriate and improves the code
by ensuring that malformed form data is handled gracefully rather than
propagating an uncaught exception. By catching the ValueError and returning a
structured error or None, the plugin adheres to the expected contract for chart
data handlers. This change aligns with the implementation described in the pull
request, where the plugin now correctly translates resolver errors into
structured validation errors.
**superset/mcp_service/chart/plugins/gantt.py**
```
try:
columns, metrics, orderby, series_columns =
resolve_gantt_query_fields(
form_data
)
except ValueError:
return None
```
##########
superset/mcp_service/chart/plugins/treemap.py:
##########
@@ -150,3 +162,110 @@ def schema_error_hint(self) -> ChartGenerationError |
None:
],
error_code="TREEMAP_VALIDATION_ERROR",
)
+
+ def resolve_query_fields(
+ self, form_data: Mapping[str, Any], viz_type: str
+ ) -> tuple[list[Any], list[Any]] | None:
+ # Treemap has exactly these roles; stale controls from another plugin
+ # must not override its singular metric or ordered hierarchy.
+ metric = form_data.get("metric")
+ hierarchy = form_data.get("groupby") or []
+ return ([metric] if metric else []), (
+ [hierarchy] if isinstance(hierarchy, str) else list(hierarchy)
+ )
+
+ def build_query_dicts(
+ self,
+ form_data: dict[str, Any],
+ *,
+ viz_type: str,
+ engine: str,
+ row_limit: int | None,
+ order_desc: bool | None,
+ ) -> list[dict[str, Any]] | None:
+ from superset.mcp_service.chart.chart_helpers import (
+ apply_treemap_query_fields,
+ build_single_query_dict,
+ )
+
+ metrics, hierarchy = cast(
+ tuple[list[Any], list[Any]],
+ self.resolve_query_fields(form_data, viz_type),
+ )
+ query = build_single_query_dict(
+ form_data,
+ hierarchy,
+ metrics,
+ row_limit=row_limit,
+ order_desc=order_desc,
+ )
+ apply_treemap_query_fields(
+ query,
+ form_data,
+ hierarchy,
+ row_limit if row_limit is not None else form_data.get("row_limit"),
+ )
+ return [query]
+
+ def normalize_query_result(self, result: Any, form_data: Mapping[str,
Any]) -> Any:
+ from superset.mcp_service.chart.query_result import (
+ normalize_treemap_query_result,
+ )
+
+ return normalize_treemap_query_result(result, form_data)
+
+ def compile_row_limit(self, form_data: Mapping[str, Any]) -> int:
Review Comment:
<!-- Bito Reply -->
The suggestion to guard the `compile_row_limit` method is appropriate and
improves the code's robustness. By mirroring the error handling used in the
sibling `preview_row_limit` method, the code now correctly catches `TypeError`,
`ValueError`, and `OverflowError` during the conversion of the row limit,
preventing unhandled exceptions and ensuring a consistent fallback to the
10-row limit.
**superset/mcp_service/chart/plugins/treemap.py**
```
def compile_row_limit(self, form_data: Mapping[str, Any]) -> int:
try:
return int(form_data.get("row_limit") or 10)
except (TypeError, ValueError, OverflowError):
return 10
```
--
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]
