codeant-ai-for-open-source[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3482789988
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -269,59 +265,31 @@ def _get_dataset_context(dataset_id: int | str) ->
DatasetContext | None:
return None
@staticmethod
- def _extract_column_references(config: Any) -> List[ColumnRef]: # noqa:
C901
- """Extract all column references from a chart configuration.
-
- Covers every supported ``ChartConfig`` variant so fast-path tools
- (``generate_explore_link``, ``update_chart_preview``) that only run
- Tier-1 validation still catch bad column refs in pie / pivot table /
- mixed timeseries / handlebars / big number charts — not just XY and
- table.
+ def _extract_column_references(
+ config: ChartConfig,
+ ) -> List[ColumnRef]:
+ """Extract all column references from configuration via the plugin
registry.
+
+ Previously only handled TableChartConfig and XYChartConfig, causing
+ 5 of 7 chart types to silently skip column validation. Now delegates
+ to the plugin for each chart type so all types are covered.
"""
- refs: List[ColumnRef] = []
-
- if isinstance(config, TableChartConfig):
- refs.extend(config.columns)
- elif isinstance(config, XYChartConfig):
- if config.x is not None:
- refs.append(config.x)
- refs.extend(config.y)
- if config.group_by:
- refs.extend(config.group_by)
- elif isinstance(config, PieChartConfig):
- refs.append(config.dimension)
- refs.append(config.metric)
- elif isinstance(config, PivotTableChartConfig):
- refs.extend(config.rows)
- if config.columns:
- refs.extend(config.columns)
- refs.extend(config.metrics)
- elif isinstance(config, MixedTimeseriesChartConfig):
- refs.append(config.x)
- refs.extend(config.y)
- if config.group_by:
- refs.extend(config.group_by)
- refs.extend(config.y_secondary)
- if config.group_by_secondary:
- refs.extend(config.group_by_secondary)
- elif isinstance(config, HandlebarsChartConfig):
- if config.columns:
- refs.extend(config.columns)
- if config.groupby:
- refs.extend(config.groupby)
- if config.metrics:
- refs.extend(config.metrics)
- elif isinstance(config, BigNumberChartConfig):
- refs.append(config.metric)
- if config.temporal_column:
- refs.append(ColumnRef(name=config.temporal_column))
-
- # Filter columns (shared by every config type that defines
``filters``).
- if filters := getattr(config, "filters", None):
- for filter_config in filters:
- refs.append(ColumnRef(name=filter_config.column))
-
- return refs
+ # Local import: plugins call DatasetValidator helpers from
+ # normalize_column_refs().
+ # A top-level import of registry in dataset_validator would make
loading this
+ # module implicitly trigger plugin registration, creating a circular
dependency.
+ from superset.mcp_service.chart.registry import get_registry
+
+ chart_type = getattr(config, "chart_type", None)
+ if chart_type is None:
+ return []
+
+ plugin = get_registry().get(chart_type)
+ if plugin is None:
+ logger.warning("No plugin registered for chart_type=%r",
chart_type)
+ return []
+
Review Comment:
**Suggestion:** Returning an empty reference list when no plugin is found
makes dataset validation silently succeed, which bypasses column validation and
defers failures to later stages with less actionable errors. Treat missing
plugin resolution as a validation failure instead of returning `[]`.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ validate_and_compile may pass configs without column validation.
- ❌ Tier-1 checks in generate_explore_link depend on dataset validation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a typed chart config that relies on dataset validation, e.g.,
`XYChartConfig(chart_type="xy", x=ColumnRef(name="bogus_dim"), y=[...])`, as
in
`superset/tests/unit_tests/mcp_service/chart/test_compile.py:6-13`, and call
`validate_and_compile(config, {}, ds, run_compile_check=False)` from
`superset/mcp_service/chart/compile.py:139-155`.
2. Inside `validate_and_compile`, Tier‑1 dataset validation runs
`DatasetValidator.validate_against_dataset(config, dataset.id,
dataset_context)` at
`compile.py:170-172`; this delegates to `_extract_column_references(config)`
at
`superset/mcp_service/chart/validation/dataset_validator.py:84,268-292` to
gather all
referenced columns.
3. If the chart plugin registry fails to resolve the plugin for
`config.chart_type`—for
example, because `_ensure_plugins_loaded()` in
`superset/mcp_service/chart/registry.py:73-98` has latched
`_plugins_load_failed=True`
after a plugin import error—then `get_registry().get(chart_type)` at
`dataset_validator.py:287` returns `None`, triggering the warning `"No
plugin registered
for chart_type=%r"` and an immediate `return []` at lines 289-291.
4. With `column_refs=[]`, the downstream checks `_validate_saved_metrics`,
`_validate_columns_exist`, and `_validate_aggregations`
(`dataset_validator.py:86-107,
114-174, 559-622`) all short‑circuit as "no problems found", causing
`validate_against_dataset` to return `(True, None)` at line 111 and
`validate_and_compile`
to treat the config as passing Tier‑1 validation at `compile.py:170-185`,
even though no
columns were inspected—reintroducing the "silently skip column validation"
behavior
whenever plugin resolution fails.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7defa351b4314449a92355da4465d298&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=7defa351b4314449a92355da4465d298&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/validation/dataset_validator.py
**Line:** 287:291
**Comment:**
*Incomplete Implementation: Returning an empty reference list when no
plugin is found makes dataset validation silently succeed, which bypasses
column validation and defers failures to later stages with less actionable
errors. Treat missing plugin resolution as a validation failure instead of
returning `[]`.
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=f0cec3479e876a1b8226e3a19232b570c0474eca678e86c19e6f2609f7712f6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=f0cec3479e876a1b8226e3a19232b570c0474eca678e86c19e6f2609f7712f6b&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]
