bito-code-review[bot] commented on code in PR #37185:
URL: https://github.com/apache/superset/pull/37185#discussion_r2696482251
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -34,6 +36,113 @@
from superset.mcp_service.utils.url_utils import get_superset_base_url
from superset.utils import json
+logger = logging.getLogger(__name__)
+
+
+@dataclass
+class DatasetValidationResult:
+ """Result of dataset accessibility validation."""
+
+ is_valid: bool
+ dataset_id: int | None
+ dataset_name: str | None
+ warnings: list[str]
+ error: str | None = None
+
+
+def validate_chart_dataset(
+ chart: Any,
+ check_access: bool = True,
+) -> DatasetValidationResult:
+ """
+ Validate that a chart's dataset exists and is accessible.
+
+ This shared utility should be called by MCP tools after creating or
retrieving
+ charts to detect issues like missing or deleted datasets early.
+
+ Args:
+ chart: A chart-like object with datasource_id, datasource_type
attributes
+ check_access: Whether to also check user permissions (default True)
+
+ Returns:
+ DatasetValidationResult with validation status and any warnings
+ """
+ from superset.daos.dataset import DatasetDAO
+ from superset.mcp_service.auth import has_dataset_access
+
+ warnings: list[str] = []
+ datasource_id = getattr(chart, "datasource_id", None)
+
+ # Check if chart has a datasource reference
+ if datasource_id is None:
+ return DatasetValidationResult(
+ is_valid=False,
+ dataset_id=None,
+ dataset_name=None,
+ warnings=[],
+ error="Chart has no dataset reference (datasource_id is None)",
+ )
+
+ # Try to look up the dataset
+ try:
+ dataset = DatasetDAO.find_by_id(datasource_id)
+
+ if dataset is None:
+ return DatasetValidationResult(
+ is_valid=False,
+ dataset_id=datasource_id,
+ dataset_name=None,
+ warnings=[],
+ error=(
+ f"Dataset (ID: {datasource_id}) has been deleted or does
not "
+ f"exist. The chart will not render correctly. "
+ f"Consider updating the chart to use a different dataset."
+ ),
+ )
+
+ dataset_name = getattr(dataset, "table_name", None) or getattr(
+ dataset, "name", None
+ )
+
+ # Check if it's a virtual dataset (SQL Lab query)
+ is_virtual = bool(getattr(dataset, "sql", None))
+ if is_virtual:
+ warnings.append(
+ f"This chart uses a virtual dataset (SQL-based). "
+ f"If the dataset '{dataset_name}' is deleted, this chart will
break."
+ )
+
+ # Check access permissions if requested
+ if check_access and not has_dataset_access(dataset):
+ return DatasetValidationResult(
+ is_valid=False,
+ dataset_id=datasource_id,
+ dataset_name=dataset_name,
+ warnings=warnings,
+ error=(
+ f"Access denied to dataset '{dataset_name}' (ID:
{datasource_id}). "
+ f"You do not have permission to view this dataset."
+ ),
+ )
+
+ return DatasetValidationResult(
+ is_valid=True,
+ dataset_id=datasource_id,
+ dataset_name=dataset_name,
+ warnings=warnings,
+ error=None,
+ )
+
+ except Exception as e:
+ logger.warning("Error validating chart dataset %s: %s", datasource_id,
e)
+ return DatasetValidationResult(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Replace the broad `Exception` catch with specific exception types (e.g.,
`DatasetDAO.DoesNotExist`, `AttributeError`, etc.) to improve error handling.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
error=None,
)
except (AttributeError, ValueError, RuntimeError) as e:
logger.warning("Error validating chart dataset %s: %s",
datasource_id, e)
return DatasetValidationResult(
````
</div>
</details>
</div>
<small><i>Code Review Run #4253df</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]
