bito-code-review[bot] commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2932603700
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -39,6 +40,117 @@
from superset.mcp_service.utils.url_utils import get_superset_base_url
from superset.utils import json
+# =============================================================================
+# Visualization Type Constants
+# =============================================================================
+
+# Time series viz types that require a datetime column on the x-axis
+TIMESERIES_VIZ_TYPES = {
+ "echarts_timeseries",
+ "echarts_timeseries_line",
+ "echarts_timeseries_bar",
+ "echarts_area",
+ "echarts_timeseries_scatter",
+ "echarts_timeseries_smooth",
+ "echarts_timeseries_step",
+ "mixed_timeseries",
+ "line",
+ "area",
+}
+
+# Categorical viz types for bar/pie charts (use these for non-time data)
+# Note: dist_bar is deprecated, use 'bar' instead
+CATEGORICAL_VIZ_TYPES = {
+ "bar",
+ "pie",
+ "big_number",
+ "big_number_total",
+ "table",
+ "pivot_table_v2",
+}
+
+
+# =============================================================================
+# Dataset Resolution
+# =============================================================================
+
+
+def resolve_dataset(
+ dataset_id: int | str, check_access: bool = True
+) -> tuple[Any | None, str | None]:
+ """
+ Resolve a dataset by ID (numeric) or UUID with optional access checking.
+
+ Args:
+ dataset_id: Numeric dataset ID or UUID string
+ check_access: Whether to check user has access to the dataset
+
+ Returns:
+ Tuple of (dataset, error_message). If successful, error_message is
None.
+ If failed, dataset is None and error_message contains the reason.
+ """
+ from superset.daos.dataset import DatasetDAO
+
+ dataset = None
+
+ if isinstance(dataset_id, int) or (
+ isinstance(dataset_id, str) and dataset_id.isdigit()
+ ):
+ numeric_id = int(dataset_id) if isinstance(dataset_id, str) else
dataset_id
+ dataset = DatasetDAO.find_by_id(numeric_id)
+ else:
+ # Try UUID lookup
+ dataset = DatasetDAO.find_by_id(dataset_id, id_column="uuid")
+
+ if not dataset:
+ return None, f"Dataset not found: {dataset_id}"
+
+ if check_access and not has_dataset_access(dataset):
+ return None, "Access denied to dataset"
+
+ return dataset, None
+
+
+def validate_timeseries_config(
+ viz_type: str, form_data: Dict[str, Any], dataset: Any
+) -> str | None:
+ """
+ Validate that timeseries chart types have required datetime configuration.
+
+ Args:
+ viz_type: The visualization type
+ form_data: Chart form_data configuration
+ dataset: The dataset object
+
+ Returns:
+ Error message if validation fails, None if valid.
+ """
+ if viz_type not in TIMESERIES_VIZ_TYPES:
+ return None
+
+ has_x_axis = form_data.get("x_axis")
+ has_granularity = form_data.get("granularity_sqla")
+ has_time_column = form_data.get("time_column")
+
+ if has_x_axis or has_granularity or has_time_column:
+ return None
+
+ # Check if dataset has a main temporal column configured
+ main_dttm_col = getattr(dataset, "main_dttm_col", None)
+ if main_dttm_col:
+ return None
+
+ # Suggest appropriate categorical chart type
+ categorical_alternative = "bar" if "bar" in viz_type else "pie"
+
+ return (
+ f"Time series chart type '{viz_type}' requires a datetime column on
the "
+ f"x-axis. For categorical data like 'genre', use "
+ f"viz_type='{categorical_alternative}' instead. Alternatively, add
'x_axis' "
+ f"or 'granularity_sqla' to form_data with a datetime column name."
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Error Message Inconsistency</b></div>
<div id="fix">
The validate_timeseries_config function checks for 'time_column' as a valid
way to specify a datetime column, but the error message only mentions 'x_axis'
and 'granularity_sqla' as alternatives. This inconsistency could confuse users
trying to fix the configuration.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
f"viz_type='{categorical_alternative}' instead. Alternatively, add
'x_axis' "
f"or 'granularity_sqla' or 'time_column' to form_data with a datetime
column name."
````
</div>
</details>
</div>
<small><i>Code Review Run #902933</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/embedded_chart/exceptions.py:
##########
@@ -0,0 +1,40 @@
+# 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.
+from flask_babel import lazy_gettext as _
+
+from superset.exceptions import SupersetException
+
+
+class EmbeddedChartPermalinkNotFoundError(SupersetException):
+ """Raised when an embedded chart permalink is not found or has expired."""
+
+ status = 404
+ message = _("The embedded chart permalink could not be found or has
expired.")
+
+
+class EmbeddedChartAccessDeniedError(SupersetException):
+ """Raised when access to an embedded chart is denied."""
+
+ status = 401
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect HTTP Status Code</b></div>
<div id="fix">
The status code for EmbeddedChartAccessDeniedError is set to 401, but this
should be 403 since it represents access being denied to an authenticated user,
not an authentication failure. Similar exceptions like DatasetAccessDeniedError
use 403.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
status = 403
````
</div>
</details>
</div>
<small><i>Code Review Run #902933</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
