bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4121421999
##########
superset/mcp_service/chart/registry.py:
##########
@@ -285,6 +305,14 @@ def is_enabled(self, chart_type: str) -> bool:
def display_name_for_viz_type(self, viz_type: str) -> str | None:
return display_name_for_viz_type(viz_type)
+ def plugin_for_viz_type(self, viz_type: str | None) -> "ChartTypePlugin |
None":
+ return plugin_for_viz_type(viz_type)
+
+ def all_plugins(self) -> list["ChartTypePlugin"]:
+ """Return every registered plugin, enabled or not, in insertion
order."""
+ _ensure_plugins_loaded()
+ return list(_REGISTRY.values())
Review Comment:
<!-- Bito Reply -->
The suggestion to move `all_plugins()` to a module-level function is
appropriate to maintain the established architectural pattern of the registry.
Implementing it as a module-level function with a thin proxy delegate ensures
consistency with existing methods like `get` and `plugin_for_viz_type`,
allowing for direct imports as requested.
**superset/mcp_service/chart/registry.py**
```
def all_plugins() -> list["ChartTypePlugin"]:
"""Return every registered plugin, enabled or not, in insertion order."""
_ensure_plugins_loaded()
return list(_REGISTRY.values())
```
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -245,19 +250,20 @@ def update_chart_preview( # noqa: C901
dataset_rebind=dataset_rebind,
)
- merged_gantt_config = validate_gantt_form_data(
- new_form_data,
- request.dataset_id,
- dataset_context=(
- build_dataset_context_from_orm(dataset)
- if new_form_data.get("viz_type") == "gantt_chart"
- else None
- ),
+ merged_plugin = plugin_for_viz_type(new_form_data.get("viz_type"))
+ merged_config = (
+ merged_plugin.validate_merged_form_data(
+ new_form_data,
+ request.dataset_id,
+ dataset_context=lambda:
build_dataset_context_from_orm(dataset),
+ )
+ if merged_plugin is not None
+ else None
)
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the registry-gated validation is noted.
However, the current implementation in `update_chart_preview.py` includes a
fail-fast guard in `chart_utils.py` that raises a `ValueError` if the request
plugin is missing, preventing unvalidated merged state from reaching
`validate_and_compile`. The existing logic correctly handles the dependency on
registry population, and the merge preserves this safety mechanism.
**superset/mcp_service/chart/tool/update_chart_preview.py**
```
merged_plugin = plugin_for_viz_type(new_form_data.get("viz_type"))
merged_config = (
merged_plugin.validate_merged_form_data(
new_form_data,
request.dataset_id,
dataset_context=lambda:
build_dataset_context_from_orm(dataset),
)
if merged_plugin is not None
else None
)
```
--
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]
