aminghadersohi commented on code in PR #37185:
URL: https://github.com/apache/superset/pull/37185#discussion_r2853654456
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -114,7 +117,7 @@ def generate(self) -> URLPreview | ChartError:
error=f"Could not generate screenshot for chart
{self.chart.id}",
error_type="ScreenshotError",
Review Comment:
Addressed in f275420 - added `AttributeError` and `TypeError` to the
exception handlers in both `_get_chart_preview_internal` and the outer
`get_chart_preview`. Screenshot/WebDriver operations are no longer used (URL
preview returns unsupported format error), but these additions cover attribute
access and json parsing edge cases.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -34,6 +37,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 (AttributeError, ValueError, RuntimeError) as e:
+ logger.warning("Error validating chart dataset %s: %s", datasource_id,
e)
Review Comment:
Addressed in f275420 - added `SQLAlchemyError` to the catch clause so DAO/DB
exceptions return a safe `DatasetValidationResult` instead of propagating.
Using `logger.exception` for full traceback. Opted for specific exceptions
rather than broad `Exception` per project coding standards.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -34,6 +37,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
Review Comment:
Addressed in f275420 - changed `dataset_id: int | None` to `dataset_id: int
| str | None` to support UUID datasets.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -92,8 +202,9 @@ def generate_explore_link(dataset_id: int | str, form_data:
Dict[str, Any]) -> s
# Return URL with just the form_data_key
return f"{base_url}/explore/?form_data_key={form_data_key}"
- except Exception:
+ except (CommandException, KeyError, ValueError) as e:
Review Comment:
Addressed in f275420 - added `SupersetException`, `SQLAlchemyError`,
`AttributeError`, and `TypeError` to the catch clause so the fallback URL is
always returned.
--
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]
