codeant-ai-for-open-source[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3482476063
########## superset/mcp_service/chart/plugins/pie.py: ########## @@ -0,0 +1,139 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Pie chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _pie_chart_what, + _summarize_filters, + map_pie_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PieChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PieChartPlugin(BaseChartPlugin): + """Plugin for pie chart type.""" + + chart_type = "pie" + display_name = "Pie / Donut Chart" + native_viz_types = { + "pie": "Pie Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "dimension" not in config and "groupby" not in config: + missing_fields.append("'dimension' (category column for slices)") + if "metric" not in config: + missing_fields.append("'metric' (value metric for slice sizes)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pie_fields", + message=( + f"Pie chart missing required fields: {', '.join(missing_fields)}" + ), + details=( + "Pie charts require a dimension (categories) and a metric (values)" + ), + suggestions=[ + "Add 'dimension' field: {'name': 'category_column'}", + "Add 'metric' field: {'name': 'value_column', 'aggregate': 'SUM'}", + "Example: {'chart_type': 'pie', 'dimension': {'name': 'product'}, " + "'metric': {'name': 'revenue', 'aggregate': 'SUM'}}", + ], + error_code="MISSING_PIE_FIELDS", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, PieChartConfig): + return [] + refs: list[ColumnRef] = [config.dimension, config.metric] Review Comment: **Suggestion:** The dimension field is passed through as a `ColumnRef` without enforcing that it is a real column reference. If a client sends `dimension` with `saved_metric=True`, dataset validation will skip column existence checks for it, but `to_form_data` still uses that name as `groupby`, producing invalid query SQL at runtime. Reject metric-style flags on the pie dimension (at least `saved_metric` and `aggregate`) or coerce them off before validation. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Pie `generate_chart` accepts metric-only dimension, breaks at query. - ❌ Pie `generate_explore_link` can emit invalid group-by SQL. - ⚠️ LLM-built pie configs may frequently misuse saved metrics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the MCP `generate_chart` tool (documented in `superset/mcp_service/app.py` around lines 210-268) with a request whose `config` is a pie chart: `{"chart_type": "pie", "dimension": {"name": "total_revenue", "saved_metric": true}, "metric": {"name": "total_revenue", "saved_metric": true}, ...}`, where `total_revenue` is a valid saved metric name from `get_dataset_info`. 2. The request is schema-validated by `SchemaValidator.validate_request` in `superset/mcp_service/chart/validation/schema_validator.py:42-56`, which parses `config` into a `PieChartConfig` where both `dimension` and `metric` are `ColumnRef` instances, and does not forbid `saved_metric=True` on the `dimension` field. 3. The validation pipeline then performs dataset validation via `ValidationPipeline._validate_dataset` in `superset/mcp_service/chart/validation/pipeline.py:18-31`, which calls `DatasetValidator.validate_against_dataset` in `superset/mcp_service/chart/validation/dataset_validator.py:56-111`. That method extracts column refs using `DatasetValidator._extract_column_references` at `dataset_validator.py:8-33`, which delegates to `PieChartPlugin.extract_column_refs` at `superset/mcp_service/chart/plugins/pie.py:75-82`, returning `[config.dimension, config.metric, ...]`. 4. In `DatasetValidator._validate_saved_metrics` (`dataset_validator.py:511-46`), both `dimension` and `metric` with `saved_metric=True` are validated only against `available_metrics` and pass; then `_validate_columns_exist` (`dataset_validator.py:114-174`) explicitly skips any `ColumnRef` with `saved_metric=True`, so the pie `dimension` is never checked against `available_columns`. Later, `PieChartPlugin.to_form_data` at `superset/mcp_service/chart/plugins/pie.py:84-87` calls `map_pie_config` in `superset/mcp_service/chart/chart_utils.py:9-36`, which sets `"groupby": [config.dimension.name]`. Because `config.dimension.name` is a saved metric name, not a real column, the generated Superset form_data groups by a metric expression instead of a column, contradicting the guidance in `app.py:3-20` ("Do NOT use a saved metric name as if it were a column — it will fail") and causing invalid SQL / query failures when the chart is executed. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f352d7ff6ab74c11942e3afbedba58b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f352d7ff6ab74c11942e3afbedba58b6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/plugins/pie.py **Line:** 78:78 **Comment:** *Logic Error: The dimension field is passed through as a `ColumnRef` without enforcing that it is a real column reference. If a client sends `dimension` with `saved_metric=True`, dataset validation will skip column existence checks for it, but `to_form_data` still uses that name as `groupby`, producing invalid query SQL at runtime. Reject metric-style flags on the pie dimension (at least `saved_metric` and `aggregate`) or coerce them off before validation. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=70d321f202279fa61f3f04ca83b5f991ea74cad67bb2abc3d4f33c2297601492&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=70d321f202279fa61f3f04ca83b5f991ea74cad67bb2abc3d4f33c2297601492&reaction=dislike'>👎</a> ########## superset/mcp_service/app.py: ########## @@ -668,6 +670,7 @@ def create_mcp_app( # NOTE: Always add new prompt/resource imports here when creating new prompts/resources. # Prompts use @mcp.prompt decorators and resources use @mcp.resource decorators. # They register automatically on import, similar to tools. +import superset.mcp_service.chart.plugins # noqa: F401, E402 — registers all chart type plugins Review Comment: **Suggestion:** This unconditional top-level import makes MCP startup hard-fail if any chart plugin import raises, which bypasses the registry's built-in lazy-load/error-containment path and can take down unrelated MCP tools. Defer plugin bootstrap through the registry loader (or guard this import with controlled fallback) so a plugin import issue does not crash app initialization. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP server startup fails if any chart plugin import errors. - ⚠️ All MCP tools unavailable, even unrelated to chart generation. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the MCP server via `superset/mcp_service/server.py:903`, which calls `init_fastmcp_server()` and imports `superset.mcp_service.app` at `server.py:15`. 2. During import of `superset/mcp_service/app.py`, module-level code at `app.py:116–118` executes `mcp = create_mcp_app()`, constructing the default FastMCP instance before tool imports. 3. After instance creation, module-level import at `superset/mcp_service/app.py:54` runs `import superset.mcp_service.chart.plugins`, which in turn imports all plugin modules and registers them in the registry via `superset/mcp_service/chart/plugins/__init__.py:41–48`. 4. Introduce an import-time error in any plugin module (for example, add `raise RuntimeError("broken plugin")` at the top of `superset/mcp_service/chart/plugins/xy.py`), then restart the MCP server: the unguarded import at `app.py:54` propagates the exception, causing `superset.mcp_service.app` import to fail and preventing `init_fastmcp_server()` and all MCP tools from loading, even though the lazy loader `_ensure_plugins_loaded()` in `superset/mcp_service/chart/registry.py:73–99` is designed to catch and contain plugin import failures. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd8dceae943244c4929f8539a049171b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fd8dceae943244c4929f8539a049171b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/app.py **Line:** 673:673 **Comment:** *Logic Error: This unconditional top-level import makes MCP startup hard-fail if any chart plugin import raises, which bypasses the registry's built-in lazy-load/error-containment path and can take down unrelated MCP tools. Defer plugin bootstrap through the registry loader (or guard this import with controlled fallback) so a plugin import issue does not crash app initialization. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=06155f255a74f760952f6e7eb391103a486aa32ad33e64da9e6acf4f47e725f3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=06155f255a74f760952f6e7eb391103a486aa32ad33e64da9e6acf4f47e725f3&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]
