aminghadersohi commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3329456201
##########
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:
Already fixed in the current code. `_pre_validate_chart_type()`
(schema_validator.py lines 178–192) makes a single `registry.get(chart_type)`
call, which internally calls `_is_plugin_enabled()` exactly once. The old
separate `is_enabled()` + `get()` sequence was removed and replaced with the
single-call pattern documented with a comment on lines 175–178. No operator
callable is invoked more than once per validation.
##########
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:
Already fixed in the current code. `_pre_validate_chart_type()`
(schema_validator.py lines 178–192) makes a single `registry.get(chart_type)`
call, which internally calls `_is_plugin_enabled()` exactly once. The old
separate `is_enabled()` + `get()` sequence was removed and replaced with the
single-call pattern documented with a comment on lines 175–178. No operator
callable is invoked more than once per validation.
##########
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:
Already fixed in the current code. `_pre_validate_chart_type()`
(schema_validator.py lines 178–192) makes a single `registry.get(chart_type)`
call, which internally calls `_is_plugin_enabled()` exactly once. The old
separate `is_enabled()` + `get()` sequence was removed and replaced with the
single-call pattern documented with a comment on lines 175–178. No operator
callable is invoked more than once per validation.
--
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]
