rebenitez1802 commented on code in PR #45484:
URL: https://github.com/apache/superset/pull/45484#discussion_r4341674566
##########
superset/mcp_service/chart/compile.py:
##########
@@ -404,43 +409,164 @@ def _active_adhoc_filters(filters: list[Any]) ->
list[dict[str, Any]]:
]
-def _classify_as_database_error(exc: BaseException, dataset_id: int) -> bool:
- """Use the dataset's DB engine spec to classify the error.
+@dataclass(frozen=True)
+class _DatabaseFailure:
+ """A database failure found in an exception's ``__cause__`` chain."""
- Walks the ``__cause__`` chain for direct ``SQLAlchemyError`` instances,
- then falls back to the engine spec's ``extract_errors`` regex patterns —
- the same classification the Superset UI uses.
+ error: BaseException
+ # SQLAlchemy errors are database failures without engine pattern matching.
+ typed: bool
+ # Found beneath a validation wrapper, as annotation query failures are.
+ wrapped: bool
+
+
+def _find_database_failure(exc: BaseException) -> _DatabaseFailure | None:
+ """Find the deepest database failure in *exc*'s ``__cause__`` chain.
+
+ Validation wrappers can carry an annotation chart's database failure, so
+ they are walked through but never classified themselves.
"""
- # Direct SQLAlchemy errors (unwrapped or in cause chain)
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+ from superset.db_engine_specs.exceptions import SupersetDBAPIError
+
+ # A non-validation outer error (e.g. a cache load failure) keeps the engine
+ # pattern fallback even when no typed database error is in its chain.
+ failure = (
+ None
+ if isinstance(exc, (ChartDataQueryValidationError,
QueryObjectValidationError))
+ else _DatabaseFailure(exc, typed=False, wrapped=False)
+ )
+ wrapped = False
+ seen: set[int] = set()
current: BaseException | None = exc
- while current is not None:
+ while current is not None and id(current) not in seen:
+ seen.add(id(current))
if isinstance(current, SQLAlchemyError):
- return True
+ return _DatabaseFailure(current, typed=True, wrapped=wrapped)
+ if isinstance(
+ current, (ChartDataQueryValidationError,
QueryObjectValidationError)
+ ):
+ wrapped = True
+ elif isinstance(current, (ChartDataQueryFailedError,
SupersetDBAPIError)):
+ failure = _DatabaseFailure(current, typed=False, wrapped=wrapped)
current = current.__cause__
+ return failure
- # Use the dataset's engine spec to classify (same as the UI)
- try:
- from superset.daos.dataset import DatasetDAO
- dataset = DatasetDAO.find_by_id(dataset_id)
- if dataset and dataset.database and isinstance(exc, Exception):
- errors = dataset.database.db_engine_spec.extract_errors(exc)
- return any(e.error_type in CONNECTION_ERROR_TYPES for e in errors)
+def _annotation_engine_specs(form_data: dict[str, Any]) -> tuple[list[Any],
bool]:
+ """Return the engine specs of the charts the annotation layers query.
+
+ The flag is True when the failing layer's engine is certain: every chart
+ layer resolved to the same engine. The exception does not name the layer
+ that raised it, so with several engines (or an unresolved chart) any of
+ them could be the failing one.
+ """
+ from superset.daos.chart import ChartDAO
+ from superset.utils.core import
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+
+ layers = form_data.get("annotation_layers")
Review Comment:
🟡 Medium — Annotation classification ignores Mixed Timeseries
`annotation_layers_b`
This helper and `_has_chart_annotation_layers` (`compile.py:495`) only read
`annotation_layers`. Query B's layers come from `annotation_layers_b`
(`common/form_data_query_context.py:98`), and MCP sets that field
(`chart_utils.py:3336`). If only query B has a chart layer, the source is
reported as `"dataset"` with the generic fallback still on, so any annotation
failure becomes `database_connection_error` with the wrong message. With layer
A on Postgres and layer B on MySQL, a MySQL syntax error from B is classified
as a connection error, because Postgres is treated as the single certain
engine. Fix: collect layers from both keys in both helpers, and add a Mixed
Timeseries test.
--
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]
