codeant-ai-for-open-source[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3280422753
########## superset/mcp_service/chart/plugins/mixed_timeseries.py: ########## @@ -0,0 +1,165 @@ +# 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. + +"""Mixed timeseries chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _mixed_timeseries_what, + _summarize_filters, + map_mixed_timeseries_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, MixedTimeseriesChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class MixedTimeseriesChartPlugin(BaseChartPlugin): + """Plugin for mixed_timeseries chart type.""" + + chart_type = "mixed_timeseries" + display_name = "Mixed Timeseries" + native_viz_types = { + "mixed_timeseries": "Mixed Timeseries Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "x" not in config: + missing_fields.append("'x' (X-axis temporal column)") + if "y" not in config: + missing_fields.append("'y' (primary Y-axis metrics)") + if "y_secondary" not in config: + missing_fields.append("'y_secondary' (secondary Y-axis metrics)") Review Comment: **Suggestion:** The pre-validation only checks for `x`, `y`, and `y_secondary` keys, but the schema explicitly accepts aliases (`x_axis`, `metrics`, `metrics_b`). Valid payloads that use those aliases will be rejected before Pydantic parsing, causing false validation failures. Make pre-validation honor the same aliases as the schema. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ SchemaValidator rejects mixed_timeseries configs using alias keys. - ⚠️ Direct callers cannot rely on x_axis/metrics/metrics_b aliases. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use the schema-level entrypoint `SchemaValidator.validate_request` at `superset/mcp_service/chart/validation/schema_validator.py:40-57` with a raw request dict shaped like: `{"dataset_id": 1, "config": {"chart_type": "mixed_timeseries", "x_axis": {"name": "ds"}, "metrics": [{"name": "revenue", "aggregate": "SUM"}], "metrics_b": [{"name": "orders", "aggregate": "COUNT"}]}}`. 2. Inside `_pre_validate` at `schema_validator.py:64-143`, the function extracts `config = data["config"]` and dispatches to `_pre_validate_chart_type(chart_type, config)` at `schema_validator.py:145-173`, passing this raw config dict (still using the alias keys `x_axis`, `metrics`, and `metrics_b`). 3. `_pre_validate_chart_type` looks up the plugin from the registry and calls `plugin.pre_validate(config)` at `schema_validator.py:190-201`; for `chart_type="mixed_timeseries"` this resolves to `MixedTimeseriesChartPlugin.pre_validate` defined in `superset/mcp_service/chart/plugins/mixed_timeseries.py:44-55`. 4. `MixedTimeseriesChartPlugin.pre_validate` only checks for `"x"`, `"y"`, and `"y_secondary"` keys (lines 50-55) and does not recognize the schema-supported aliases `x_axis`, `metrics`, and `metrics_b` defined on `MixedTimeseriesChartConfig` in `superset/mcp_service/chart/schemas.py:18-51`, so `missing_fields` becomes non-empty, a `ChartGenerationError` is returned, and `SchemaValidator.validate_request` exits early at line 51, rejecting a payload that would successfully parse via Pydantic if alias keys were honored. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78ad6d02f48f47778e318cdaea7288f8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=78ad6d02f48f47778e318cdaea7288f8&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/mixed_timeseries.py **Line:** 50:55 **Comment:** *Api Mismatch: The pre-validation only checks for `x`, `y`, and `y_secondary` keys, but the schema explicitly accepts aliases (`x_axis`, `metrics`, `metrics_b`). Valid payloads that use those aliases will be rejected before Pydantic parsing, causing false validation failures. Make pre-validation honor the same aliases as the schema. 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=94828fb25b98d8eca1e944606fb7043967c57c9e4321aebb05ddd1edcc9f94c5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=94828fb25b98d8eca1e944606fb7043967c57c9e4321aebb05ddd1edcc9f94c5&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/plugins/mixed_timeseries.py: ########## @@ -0,0 +1,165 @@ +# 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. + +"""Mixed timeseries chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _mixed_timeseries_what, + _summarize_filters, + map_mixed_timeseries_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, MixedTimeseriesChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class MixedTimeseriesChartPlugin(BaseChartPlugin): + """Plugin for mixed_timeseries chart type.""" + + chart_type = "mixed_timeseries" + display_name = "Mixed Timeseries" + native_viz_types = { + "mixed_timeseries": "Mixed Timeseries Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if "x" not in config: + missing_fields.append("'x' (X-axis temporal column)") + if "y" not in config: + missing_fields.append("'y' (primary Y-axis metrics)") + if "y_secondary" not in config: + missing_fields.append("'y_secondary' (secondary Y-axis metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_mixed_timeseries_fields", + message=( + f"Mixed timeseries chart missing required fields: " + f"{', '.join(missing_fields)}" + ), + details=( + "Mixed timeseries charts require an x-axis, primary metrics, " + "and secondary metrics" + ), + suggestions=[ + "Add 'x' field: {'name': 'date_column'}", + "Add 'y' field: [{'name': 'revenue', 'aggregate': 'SUM'}]", + "Add 'y_secondary': [{'name': 'orders', 'aggregate': 'COUNT'}]", + "Optional: 'primary_kind' and 'secondary_kind' for chart types", + ], + error_code="MISSING_MIXED_TIMESERIES_FIELDS", + ) + + for field_name in ["y", "y_secondary"]: + if not isinstance(config.get(field_name, []), list): + return ChartGenerationError( + error_type=f"invalid_{field_name}_format", + message=f"'{field_name}' must be a list of metrics", + details=( + f"The '{field_name}' field must be an array of metric " + "specifications" + ), + suggestions=[ + f"Wrap in array: '{field_name}': " + "[{'name': 'col', 'aggregate': 'SUM'}]", + ], + error_code=f"INVALID_{field_name.upper()}_FORMAT", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, MixedTimeseriesChartConfig): + return [] + refs: list[ColumnRef] = [config.x] Review Comment: **Suggestion:** Metric name normalization rewrites every entry in `y`/`y_secondary` without checking `saved_metric`, so saved metrics can be incorrectly remapped to similarly named dataset columns (column lookup is prioritized), breaking saved-metric resolution later. Skip canonical-column normalization for items marked as `saved_metric`. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Mixed_timeseries charts using saved metrics may fail at runtime. - ⚠️ Saved metrics shadowed by columns resolve to wrong identifiers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the MCP tool `generate_chart` defined in `superset/mcp_service/chart/tool/generate_chart.py:95-132` with a `GenerateChartRequest` whose `config.chart_type` is `"mixed_timeseries"` and whose `y`/`y_secondary` lists contain saved metrics, e.g. `{"name": "count", "saved_metric": true}` as described by `ColumnRef.saved_metric` in `superset/mcp_service/chart/schemas.py:27-58`. 2. Inside `generate_chart`, the code invokes `ValidationPipeline.validate_request_with_warnings(request.model_dump())` at `generate_chart.py:205-210`; in `ValidationPipeline.validate_request_with_warnings` (`superset/mcp_service/chart/validation/pipeline.py:90-145`), after schema and dataset validation, it calls `_normalize_column_names` at `pipeline.py:227-260`. 3. `_normalize_column_names` delegates to `DatasetValidator.normalize_column_names(config, dataset_id, dataset_context)` at `superset/mcp_service/chart/validation/dataset_validator.py:351-398`; this looks up the `mixed_timeseries` plugin from the registry and calls `plugin.normalize_column_refs(config, dataset_context)`, which is `MixedTimeseriesChartPlugin.normalize_column_refs` at `superset/mcp_service/chart/plugins/mixed_timeseries.py:84-106`. 4. In `normalize_column_refs`, helper `_norm_list` (lines 93-98) iterates over every entry in `y` and `y_secondary` and unconditionally rewrites `col["name"]` using `DatasetValidator._get_canonical_column_name(col["name"], dataset_context)`, ignoring `col.get("saved_metric")`; `_get_canonical_column_name` at `dataset_validator.py:303-334` searches `dataset_context.available_columns` first and only then `available_metrics`, so in a dataset that defines both a column `"COUNT"` and a saved metric `"count"`, a saved-metric reference `{"name": "count", "saved_metric": true}` in `y` is normalized to `"COUNT"`. Later, `map_mixed_timeseries_config` in `superset/mcp_service/chart/chart_utils.py:21-46` calls `create_metric_object` on these `ColumnRef` objects; for saved metrics it returns `col.name` (now `"COUNT"`) as the metric identifier. Because Superset's metric lookup expects the original metric name (e.g. `"count"`), the chart's metrics no longer resolve correctly, causing mixed_timeseries charts using such saved metrics to fail or behave incorrectly at query time. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f170504cd22f4f7b98f812a23f16dac5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f170504cd22f4f7b98f812a23f16dac5&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/mixed_timeseries.py **Line:** 93:98 **Comment:** *Logic Error: Metric name normalization rewrites every entry in `y`/`y_secondary` without checking `saved_metric`, so saved metrics can be incorrectly remapped to similarly named dataset columns (column lookup is prioritized), breaking saved-metric resolution later. Skip canonical-column normalization for items marked as `saved_metric`. 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=fcd94f52edcdca6c6714e1f1a09c7f79cd6a770a1c3fb5629a4ffdb89a4f47b2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=fcd94f52edcdca6c6714e1f1a09c7f79cd6a770a1c3fb5629a4ffdb89a4f47b2&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]
