codeant-ai-for-open-source[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3493571488
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag `extract_column_refs` returning an empty list when a chart
plugin cannot be resolved; in this validation flow, schema validation already
rejects unknown `chart_type` values, so `[]` is an acceptable defensive
fallback.
**Applied to:**
- `superset/mcp_service/chart/validation/dataset_validator.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
