aminghadersohi commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4121420292
##########
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:
Fixed in 5bfb99d033: all_plugins() is a module-level operation and
_RegistryProxy delegates to it. Added a regression test covering both
interfaces, insertion order, and disabled plugins; it fails before the fix with
AttributeError. Checked #44746 commit 49b2e0a2 first: it does not address this
API consistency issue. The same fix is owed on #44746.
##########
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:
Not applicable to the stated plugin-import-failure path.
update_chart_preview.py:238-240 calls map_config_to_form_data before the
merge/validation gate. chart_utils.py:375-386 looks up the request plugin and
raises ValueError if it is missing, so an empty registry cannot reach
validate_and_compile. This guard also exists at the reviewed head fc13729e.
Checked #44746 commit 49b2e0a2; it handles disabled plugins, not failed
imports. Merge 0edd9d5ef2 brings that commit here while preserving this
fail-fast guard.
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -292,7 +298,7 @@ def update_chart_preview( # noqa: C901
config,
new_form_data,
dataset,
- run_compile_check=config.chart_type in ("gauge", "treemap_v2"),
+ run_compile_check=bool(plugin and
plugin.requires_compile_check),
Review Comment:
Addressed by merging fork/mcp-chart-plugin-contract in 0edd9d5ef2, including
#44746 commit 49b2e0a2. That commit resolves the request plugin with
include_disabled=True and retains disabled-plugin form-data mapping, so a
disabled Gauge/Treemap keeps its owning contract and requires_compile_check
flag. The imported parametrized
test_registered_update_preview_preserves_cached_controls exercises disabled
Treemap updates and asserts run_compile_check=True. No duplicate contract fix
was needed here.
--
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]
