Copilot commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3325634827
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -390,6 +390,24 @@ async def update_chart( # noqa: C901
# config is already a typed ChartConfig | None (validated by Pydantic)
parsed_config = request.config
+ # Normalize column case to match dataset canonical names
+ # (mirrors generate_chart pipeline layer 4)
+ chart_datasource_id = getattr(chart, "datasource_id", None)
+ if parsed_config is not None and chart_datasource_id is not None:
+ from superset.mcp_service.chart.validation.dataset_validator
import (
+ DatasetValidator,
+ NORMALIZATION_EXCEPTIONS,
+ )
+
+ try:
+ parsed_config = DatasetValidator.normalize_column_names(
+ parsed_config, chart.datasource_id
+ )
Review Comment:
`chart_datasource_id` is computed via `getattr(...)` and checked for None,
but the normalization call uses `chart.datasource_id` instead of the
already-validated local variable. This reintroduces the attribute access you
just guarded against and can fail if `chart` is a mock/partial object or has a
property side effect.
##########
superset/mcp_service/chart/validation/schema_validator.py:
##########
@@ -178,351 +172,33 @@ def _pre_validate_chart_type(
error_code="INVALID_CHART_TYPE",
)
- return chart_type_validators[chart_type](config)
-
- @staticmethod
- def _pre_validate_xy_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate XY chart configuration."""
- # x is optional — defaults to dataset's main_dttm_col in map_xy_config
- if "y" not in config:
- return False, ChartGenerationError(
- error_type="missing_xy_fields",
- message="XY chart missing required field: 'y' (Y-axis
metrics)",
- details="XY charts require Y-axis (metrics) specifications. "
- "X-axis is optional and defaults to the dataset's primary "
- "datetime column when omitted.",
- suggestions=[
- "Add 'y' field: [{'name': 'metric_column', 'aggregate':
'SUM'}] "
- "for Y-axis",
- "Example: {'chart_type': 'xy', 'x': {'name': 'date'}, "
- "'y': [{'name': 'sales', 'aggregate': 'SUM'}]}",
- ],
- error_code="MISSING_XY_FIELDS",
- )
-
- # Validate Y is a list
- if not isinstance(config.get("y", []), list):
- return False, ChartGenerationError(
- error_type="invalid_y_format",
- message="Y-axis must be a list of metrics",
- details="The 'y' field must be an array of metric
specifications",
- suggestions=[
- "Wrap Y-axis metric in array: 'y': [{'name': 'column', "
- "'aggregate': 'SUM'}]",
- "Multiple metrics supported: 'y': [metric1, metric2, ...]",
- ],
- error_code="INVALID_Y_FORMAT",
- )
-
- return True, None
-
- @staticmethod
- def _pre_validate_table_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate table chart configuration."""
- if "columns" not in config:
- return False, ChartGenerationError(
- error_type="missing_columns",
- message="Table chart missing required field: columns",
- details="Table charts require a 'columns' array to specify
which "
- "columns to display",
- suggestions=[
- "Add 'columns' field with array of column specifications",
- "Example: 'columns': [{'name': 'product'}, {'name':
'sales', "
- "'aggregate': 'SUM'}]",
- "Each column can have optional 'aggregate' for metrics",
- ],
- error_code="MISSING_COLUMNS",
- )
-
- if not isinstance(config.get("columns", []), list):
+ if not registry.is_enabled(chart_type):
+ valid_types = ", ".join(registry.all_types())
return False, ChartGenerationError(
- error_type="invalid_columns_format",
- message="Columns must be a list",
- details="The 'columns' field must be an array of column
specifications",
+ error_type="disabled_chart_type",
+ message=f"Chart type '{chart_type}' is not enabled on this
instance",
Review Comment:
`_pre_validate_chart_type()` calls the dynamic plugin filter twice
(`is_enabled()` then `get()`), which can lead to inconsistent behavior when
`MCP_CHART_PLUGIN_ENABLED_FUNC` depends on request context (or is
non-deterministic) and also does unnecessary repeated work. Prefer a single
`registry.get()` lookup and treat `None` as disabled (unknown types are already
handled by `is_registered`).
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -390,6 +390,24 @@ async def update_chart( # noqa: C901
# config is already a typed ChartConfig | None (validated by Pydantic)
parsed_config = request.config
+ # Normalize column case to match dataset canonical names
+ # (mirrors generate_chart pipeline layer 4)
+ chart_datasource_id = getattr(chart, "datasource_id", None)
+ if parsed_config is not None and chart_datasource_id is not None:
+ from superset.mcp_service.chart.validation.dataset_validator
import (
+ DatasetValidator,
+ NORMALIZATION_EXCEPTIONS,
+ )
+
+ try:
+ parsed_config = DatasetValidator.normalize_column_names(
+ parsed_config, chart.datasource_id
+ )
Review Comment:
`chart_datasource_id` is computed via `getattr(...)` and checked for None,
but the normalization call uses `chart.datasource_id` instead of the
already-validated local variable. This reintroduces the attribute access you
just guarded against and can fail with mock/partial chart objects.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -101,7 +104,14 @@ class ChartInfo(BaseModel):
id: int | None = Field(None, description="Chart ID")
slice_name: str | None = Field(None, description="Chart name")
- viz_type: str | None = Field(None, description="Visualization type")
+ viz_type: str | None = Field(None, description="Visualization type
(internal ID)")
+ chart_type_display_name: str | None = Field(
+ None,
+ description=(
+ "User-friendly chart type name (e.g. 'Line Chart', 'Pivot Table').
"
+ "Use this field when referring to chart types — never expose
viz_type."
Review Comment:
`chart_type_display_name`’s schema description says “never expose viz_type”,
but MCP instructions and API behavior still require using `viz_type` when no
display name is available. This contradiction can confuse schema-driven
clients; consider wording this as a preference (use display name when present;
fall back to viz_type when null).
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -390,6 +390,24 @@ async def update_chart( # noqa: C901
# config is already a typed ChartConfig | None (validated by Pydantic)
parsed_config = request.config
+ # Normalize column case to match dataset canonical names
+ # (mirrors generate_chart pipeline layer 4)
+ chart_datasource_id = getattr(chart, "datasource_id", None)
+ if parsed_config is not None and chart_datasource_id is not None:
+ from superset.mcp_service.chart.validation.dataset_validator
import (
+ DatasetValidator,
+ NORMALIZATION_EXCEPTIONS,
+ )
+
+ try:
+ parsed_config = DatasetValidator.normalize_column_names(
+ parsed_config, chart.datasource_id
+ )
Review Comment:
`chart_datasource_id` is computed via `getattr(...)` and checked for None,
but the normalization call uses `chart.datasource_id` instead of the
already-validated local variable. This reintroduces the attribute access you
just guarded against and can fail with mock/partial chart objects.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -101,7 +104,14 @@ class ChartInfo(BaseModel):
id: int | None = Field(None, description="Chart ID")
slice_name: str | None = Field(None, description="Chart name")
- viz_type: str | None = Field(None, description="Visualization type")
+ viz_type: str | None = Field(None, description="Visualization type
(internal ID)")
+ chart_type_display_name: str | None = Field(
+ None,
+ description=(
+ "User-friendly chart type name (e.g. 'Line Chart', 'Pivot Table').
"
+ "Use this field when referring to chart types — never expose
viz_type."
Review Comment:
`chart_type_display_name`’s schema description says “never expose viz_type”,
but MCP instructions and API behavior still require using `viz_type` when no
display name is available. This contradiction can confuse schema-driven
clients; consider wording this as a preference (use display name when present;
fall back to viz_type when null).
##########
superset/mcp_service/chart/validation/schema_validator.py:
##########
@@ -178,351 +172,33 @@ def _pre_validate_chart_type(
error_code="INVALID_CHART_TYPE",
)
- return chart_type_validators[chart_type](config)
-
- @staticmethod
- def _pre_validate_xy_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate XY chart configuration."""
- # x is optional — defaults to dataset's main_dttm_col in map_xy_config
- if "y" not in config:
- return False, ChartGenerationError(
- error_type="missing_xy_fields",
- message="XY chart missing required field: 'y' (Y-axis
metrics)",
- details="XY charts require Y-axis (metrics) specifications. "
- "X-axis is optional and defaults to the dataset's primary "
- "datetime column when omitted.",
- suggestions=[
- "Add 'y' field: [{'name': 'metric_column', 'aggregate':
'SUM'}] "
- "for Y-axis",
- "Example: {'chart_type': 'xy', 'x': {'name': 'date'}, "
- "'y': [{'name': 'sales', 'aggregate': 'SUM'}]}",
- ],
- error_code="MISSING_XY_FIELDS",
- )
-
- # Validate Y is a list
- if not isinstance(config.get("y", []), list):
- return False, ChartGenerationError(
- error_type="invalid_y_format",
- message="Y-axis must be a list of metrics",
- details="The 'y' field must be an array of metric
specifications",
- suggestions=[
- "Wrap Y-axis metric in array: 'y': [{'name': 'column', "
- "'aggregate': 'SUM'}]",
- "Multiple metrics supported: 'y': [metric1, metric2, ...]",
- ],
- error_code="INVALID_Y_FORMAT",
- )
-
- return True, None
-
- @staticmethod
- def _pre_validate_table_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate table chart configuration."""
- if "columns" not in config:
- return False, ChartGenerationError(
- error_type="missing_columns",
- message="Table chart missing required field: columns",
- details="Table charts require a 'columns' array to specify
which "
- "columns to display",
- suggestions=[
- "Add 'columns' field with array of column specifications",
- "Example: 'columns': [{'name': 'product'}, {'name':
'sales', "
- "'aggregate': 'SUM'}]",
- "Each column can have optional 'aggregate' for metrics",
- ],
- error_code="MISSING_COLUMNS",
- )
-
- if not isinstance(config.get("columns", []), list):
+ if not registry.is_enabled(chart_type):
+ valid_types = ", ".join(registry.all_types())
return False, ChartGenerationError(
- error_type="invalid_columns_format",
- message="Columns must be a list",
- details="The 'columns' field must be an array of column
specifications",
+ error_type="disabled_chart_type",
+ message=f"Chart type '{chart_type}' is not enabled on this
instance",
Review Comment:
`_pre_validate_chart_type()` calls the dynamic plugin filter twice
(`is_enabled()` then `get()`), which can lead to inconsistent behavior when
`MCP_CHART_PLUGIN_ENABLED_FUNC` depends on request context (or is
non-deterministic) and also does unnecessary repeated work. Prefer a single
`registry.get()` lookup and treat `None` as disabled (unknown types are already
handled by `is_registered`).
##########
superset/mcp_service/chart/validation/schema_validator.py:
##########
@@ -178,351 +172,33 @@ def _pre_validate_chart_type(
error_code="INVALID_CHART_TYPE",
)
- return chart_type_validators[chart_type](config)
-
- @staticmethod
- def _pre_validate_xy_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate XY chart configuration."""
- # x is optional — defaults to dataset's main_dttm_col in map_xy_config
- if "y" not in config:
- return False, ChartGenerationError(
- error_type="missing_xy_fields",
- message="XY chart missing required field: 'y' (Y-axis
metrics)",
- details="XY charts require Y-axis (metrics) specifications. "
- "X-axis is optional and defaults to the dataset's primary "
- "datetime column when omitted.",
- suggestions=[
- "Add 'y' field: [{'name': 'metric_column', 'aggregate':
'SUM'}] "
- "for Y-axis",
- "Example: {'chart_type': 'xy', 'x': {'name': 'date'}, "
- "'y': [{'name': 'sales', 'aggregate': 'SUM'}]}",
- ],
- error_code="MISSING_XY_FIELDS",
- )
-
- # Validate Y is a list
- if not isinstance(config.get("y", []), list):
- return False, ChartGenerationError(
- error_type="invalid_y_format",
- message="Y-axis must be a list of metrics",
- details="The 'y' field must be an array of metric
specifications",
- suggestions=[
- "Wrap Y-axis metric in array: 'y': [{'name': 'column', "
- "'aggregate': 'SUM'}]",
- "Multiple metrics supported: 'y': [metric1, metric2, ...]",
- ],
- error_code="INVALID_Y_FORMAT",
- )
-
- return True, None
-
- @staticmethod
- def _pre_validate_table_config(
- config: Dict[str, Any],
- ) -> Tuple[bool, ChartGenerationError | None]:
- """Pre-validate table chart configuration."""
- if "columns" not in config:
- return False, ChartGenerationError(
- error_type="missing_columns",
- message="Table chart missing required field: columns",
- details="Table charts require a 'columns' array to specify
which "
- "columns to display",
- suggestions=[
- "Add 'columns' field with array of column specifications",
- "Example: 'columns': [{'name': 'product'}, {'name':
'sales', "
- "'aggregate': 'SUM'}]",
- "Each column can have optional 'aggregate' for metrics",
- ],
- error_code="MISSING_COLUMNS",
- )
-
- if not isinstance(config.get("columns", []), list):
+ if not registry.is_enabled(chart_type):
+ valid_types = ", ".join(registry.all_types())
return False, ChartGenerationError(
- error_type="invalid_columns_format",
- message="Columns must be a list",
- details="The 'columns' field must be an array of column
specifications",
+ error_type="disabled_chart_type",
+ message=f"Chart type '{chart_type}' is not enabled on this
instance",
Review Comment:
`_pre_validate_chart_type()` calls the dynamic plugin filter twice
(`is_enabled()` then `get()`), which can lead to inconsistent behavior when
`MCP_CHART_PLUGIN_ENABLED_FUNC` depends on request context (or is
non-deterministic) and also does unnecessary repeated work. Prefer a single
`registry.get()` lookup and treat `None` as disabled (unknown types are already
handled by `is_registered`).
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -320,29 +320,37 @@ def map_config_to_form_data(
| BigNumberChartConfig,
dataset_id: int | str | None = None,
) -> Dict[str, Any]:
- """Map chart config to Superset form_data."""
- if isinstance(config, TableChartConfig):
- return map_table_config(config)
- elif isinstance(config, XYChartConfig):
- return map_xy_config(config, dataset_id=dataset_id)
- elif isinstance(config, PieChartConfig):
- return map_pie_config(config)
- elif isinstance(config, PivotTableChartConfig):
- return map_pivot_table_config(config)
- elif isinstance(config, MixedTimeseriesChartConfig):
- return map_mixed_timeseries_config(config, dataset_id=dataset_id)
- elif isinstance(config, HandlebarsChartConfig):
- return map_handlebars_config(config)
- elif isinstance(config, BigNumberChartConfig):
- if config.show_trendline and config.temporal_column:
- if not is_column_truly_temporal(config.temporal_column,
dataset_id):
- raise ValueError(
- f"Big Number trendline requires a temporal SQL column; "
- f"'{config.temporal_column}' is not temporal."
- )
- return map_big_number_config(config)
- else:
- raise ValueError(f"Unsupported config type: {type(config)}")
+ """Map chart config to Superset form_data via the plugin registry.
+
+ The previous if/elif chain across all 7 chart types has been replaced by a
+ single registry lookup. Cross-field constraints (e.g. BigNumber trendline
+ temporal check) are now owned by each plugin's post_map_validate() method
+ rather than being baked into this dispatcher.
+ """
+ # Local import: plugins call map_*_config from their to_form_data()
methods,
+ # so chart_utils is loaded before plugins finish registering. A top-level
+ # import of registry here would trigger plugin loading mid-import = cycle.
+ from superset.mcp_service.chart.registry import get_registry
+
+ chart_type = getattr(config, "chart_type", None)
+ plugin = get_registry().get(chart_type) if chart_type else None
+
+ if plugin is None:
+ if chart_type is None:
+ raise ValueError(f"Unsupported config type: {type(config)}")
+ raise ValueError(
+ f"Unsupported config type: {type(config)}
(chart_type={chart_type!r})"
+ )
+
+ form_data = plugin.to_form_data(config, dataset_id=dataset_id)
+
+ # Run post-map validation (e.g. BigNumber trendline temporal type check).
+ # Raise ValueError to preserve backward-compatible error handling in
callers.
+ error = plugin.post_map_validate(config, form_data, dataset_id=dataset_id)
+ if error is not None:
+ raise ValueError(error.message)
Review Comment:
`post_map_validate()` returns a structured `ChartGenerationError`
(details/suggestions/error_code), but `map_config_to_form_data()` discards
everything except `error.message` by raising a bare `ValueError`. Since callers
wrap `ValueError` into a generic MCP error, this loses actionable context (e.g.
BigNumber trendline temporal column guidance). At minimum, include `details`
and `suggestions` in the raised message (or propagate the structured error
end-to-end).
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -320,29 +320,37 @@ def map_config_to_form_data(
| BigNumberChartConfig,
dataset_id: int | str | None = None,
) -> Dict[str, Any]:
- """Map chart config to Superset form_data."""
- if isinstance(config, TableChartConfig):
- return map_table_config(config)
- elif isinstance(config, XYChartConfig):
- return map_xy_config(config, dataset_id=dataset_id)
- elif isinstance(config, PieChartConfig):
- return map_pie_config(config)
- elif isinstance(config, PivotTableChartConfig):
- return map_pivot_table_config(config)
- elif isinstance(config, MixedTimeseriesChartConfig):
- return map_mixed_timeseries_config(config, dataset_id=dataset_id)
- elif isinstance(config, HandlebarsChartConfig):
- return map_handlebars_config(config)
- elif isinstance(config, BigNumberChartConfig):
- if config.show_trendline and config.temporal_column:
- if not is_column_truly_temporal(config.temporal_column,
dataset_id):
- raise ValueError(
- f"Big Number trendline requires a temporal SQL column; "
- f"'{config.temporal_column}' is not temporal."
- )
- return map_big_number_config(config)
- else:
- raise ValueError(f"Unsupported config type: {type(config)}")
+ """Map chart config to Superset form_data via the plugin registry.
+
+ The previous if/elif chain across all 7 chart types has been replaced by a
+ single registry lookup. Cross-field constraints (e.g. BigNumber trendline
+ temporal check) are now owned by each plugin's post_map_validate() method
+ rather than being baked into this dispatcher.
+ """
+ # Local import: plugins call map_*_config from their to_form_data()
methods,
+ # so chart_utils is loaded before plugins finish registering. A top-level
+ # import of registry here would trigger plugin loading mid-import = cycle.
+ from superset.mcp_service.chart.registry import get_registry
+
+ chart_type = getattr(config, "chart_type", None)
+ plugin = get_registry().get(chart_type) if chart_type else None
+
+ if plugin is None:
+ if chart_type is None:
+ raise ValueError(f"Unsupported config type: {type(config)}")
+ raise ValueError(
+ f"Unsupported config type: {type(config)}
(chart_type={chart_type!r})"
+ )
+
+ form_data = plugin.to_form_data(config, dataset_id=dataset_id)
+
+ # Run post-map validation (e.g. BigNumber trendline temporal type check).
+ # Raise ValueError to preserve backward-compatible error handling in
callers.
+ error = plugin.post_map_validate(config, form_data, dataset_id=dataset_id)
+ if error is not None:
+ raise ValueError(error.message)
Review Comment:
`post_map_validate()` returns a structured `ChartGenerationError`
(details/suggestions/error_code), but `map_config_to_form_data()` discards
everything except `error.message` by raising a bare `ValueError`. Since callers
wrap `ValueError` into a generic MCP error, this loses actionable context (e.g.
BigNumber trendline temporal column guidance). At minimum, include `details`
and `suggestions` in the raised message (or propagate the structured error
end-to-end).
--
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]
