aminghadersohi commented on code in PR #45484:
URL: https://github.com/apache/superset/pull/45484#discussion_r4337324474
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns —
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
Review Comment:
Fixed in e0991386a2e89e3a2c2814caf410f66f72869e5a. The cause-chain walk in
compile.py (`_find_database_failure`) also takes `SupersetDBAPIError`
subclasses as database failures, so a typed DB-API error raised while the
annotation chart builds its payload goes through the engine patterns instead of
becoming `compile_error`.
Regression test:
`tests/unit_tests/mcp_service/chart/test_compile.py::test_compile_chart_annotation_dbapi_error_is_connection_error`
raises `SupersetDBAPIOperationalError` from the annotation chart through the
real `get_viz_annotation_data` wrapping. It failed before the fix
(`compile_error`) and passes now.
Siblings checked:
- superset/mcp_service/chart/compile.py:452 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
- superset/mcp_service/chart/compile.py:446 — not affected: a
SQLAlchemyError in the chain already classified directly and still does
- superset/mcp_service/chart/compile.py:218 — not affected: a non-validation
outer error (ChartDataCacheLoadError) keeps the engine-pattern fallback
- superset/connectors/sqla/utils.py:232 — not affected:
SupersetGenericDBErrorException is a SupersetErrorException, not a DB-API
error, and its classification is unchanged
- superset/mcp_service/middleware.py:263 — not affected: reads one __cause__
step and does not classify DB errors
- superset/mcp_service/chart/tool/get_chart_data.py:156 — not affected:
reads one __cause__ step and does not classify DB errors
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns —
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
current = current.__cause__
+ if database_exc is None:
+ return False
+
# 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)
+ if dataset and dataset.database and isinstance(database_exc,
Exception):
+ errors =
dataset.database.db_engine_spec.extract_errors(database_exc)
Review Comment:
Fixed in e0991386a2e89e3a2c2814caf410f66f72869e5a. When the failure sits
beneath a validation wrapper and the chart has table/line annotation layers,
compile.py resolves those annotation charts' databases
(`_annotation_engine_specs`) and matches the error with their engine specs. It
falls back to the dataset's spec only when no annotation database can be found.
The connection error message then reads "Unable to connect to the database of
an annotation layer's chart."
Regression test:
`tests/unit_tests/mcp_service/chart/test_compile.py::test_compile_chart_annotation_failure_uses_annotation_engine_spec`
(main dataset on PostgresEngineSpec, annotation chart on MySQLEngineSpec).
Both cases failed before the fix: the MySQL syntax error became
`database_connection_error` through the Postgres generic fallback, and the
MySQL connection error had no annotation-layer message. Both pass now.
Siblings checked:
- superset/mcp_service/chart/compile.py:458 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
- superset/mcp_service/chart/compile.py:523 — not affected: the main-dataset
path (no validation wrapper, or no chart annotation layers) still uses the
dataset's spec
- superset/common/query_context_processor.py:922 — not affected: NATIVE
annotation layers read the metadata database, not an external one
- docs/docs/using-superset/using-ai-with-superset.mdx:483 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
##########
superset/mcp_service/chart/compile.py:
##########
@@ -411,20 +411,35 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
then falls back to the engine spec's ``extract_errors`` regex patterns —
the same classification the Superset UI uses.
"""
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+
+ # Validation wrappers can carry an annotation chart's database failure.
+ # Only a non-validation query failure may use the engine's generic
fallback.
+ database_exc = None if isinstance(exc, ChartDataQueryValidationError) else
exc
# Direct SQLAlchemy errors (unwrapped or in cause chain)
current: BaseException | None = exc
while current is not None:
if isinstance(current, SQLAlchemyError):
return True
+ if isinstance(current, ChartDataQueryFailedError) and not isinstance(
+ current, ChartDataQueryValidationError
+ ):
+ database_exc = current
current = current.__cause__
Review Comment:
Fixed in e0991386a2e89e3a2c2814caf410f66f72869e5a. The walk in
`_find_database_failure` tracks a `seen` set of `id(current)` and stops on a
repeat.
Regression test:
`tests/unit_tests/mcp_service/chart/test_compile.py::test_classify_as_database_error_stops_on_cyclic_cause_chain`
runs the classifier on a two-exception cycle in a daemon thread and requires
it to finish within 5s with no classification. With the previous loop the same
cycle was still running after 5s; the test passes now.
Siblings checked:
- superset/mcp_service/chart/compile.py:444 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
- superset/mcp_service/middleware.py:263 — not affected: single __cause__
step, no loop
- superset/mcp_service/middleware.py:289 — not affected: single __cause__
step, no loop
- superset/mcp_service/utils/validation.py:189 — not affected: single
__cause__ step, no loop
- superset/mcp_service/chart/tool/get_chart_data.py:156 — not affected:
single __cause__ step, no loop
##########
tests/unit_tests/mcp_service/chart/test_compile.py:
##########
@@ -1133,3 +1134,121 @@ def
test_compile_chart_returns_structured_error_for_malformed_gantt_form_data(
assert result.error_obj.error_type == "compile_error"
assert "tooltip_columns must contain at most 50 entries" in (result.error
or "")
mock_cmd_cls.assert_not_called()
+
+
[email protected](
+ "message",
+ [
+ "Error in jinja expression in metric expression: "
+ "Unsafe template value for key time_grain: TimeGrain",
+ "Invalid post-processing operation",
+ ],
+)
+def test_compile_chart_validation_failure_is_not_connection_error(
+ mocker: MockerFixture, message: str
+) -> None:
+ """Typed template/processing failures must bypass generic DB
classification."""
+ from superset.db_engine_specs.base import BaseEngineSpec
+ from superset.exceptions import (
+ QueryObjectValidationError,
+ QueryObjectValidationMessage,
+ )
+
+ context = Mock()
+ context.get_payload.return_value = {
+ "queries": [
+ {"error":
QueryObjectValidationMessage(QueryObjectValidationError(message))}
+ ]
+ }
+ mocker.patch(
+
"superset.mcp_service.chart.chart_helpers.build_query_context_from_form_data",
+ return_value=context,
+ )
+ mocker.patch("superset.charts.data.form_data.set_query_context_form_data")
+ dataset = Mock()
+ dataset.database.db_engine_spec = BaseEngineSpec
+ mocker.patch("superset.daos.dataset.DatasetDAO.find_by_id",
return_value=dataset)
+
+ result = _compile_chart({"metrics": ["count"]}, dataset_id=1)
+
+ assert not result.success
+ assert result.error_obj is not None
+ assert result.error_obj.error_type == "compile_error"
+ assert result.error_obj.error_code == "CHART_COMPILE_FAILED"
+ assert message in result.error_obj.details
+
+
[email protected](
+ ("message", "validation_failure", "expected_error_type"),
+ [
+ (
+ "(psycopg2.OperationalError) connection to server failed: "
+ "Connection refused",
+ False,
+ "database_connection_error",
+ ),
+ (
+ "Unsafe template value for key time_grain: TimeGrain",
+ True,
+ "compile_error",
+ ),
+ ("Invalid post-processing operation", True, "compile_error"),
+ ],
+)
+def test_compile_chart_annotation_failure_preserves_error_classification(
Review Comment:
Fixed in e0991386a2e89e3a2c2814caf410f66f72869e5a. I added cases that depend
on the engine's own patterns instead of BaseEngineSpec's generic fallback.
Regression test:
`tests/unit_tests/mcp_service/chart/test_compile.py::test_compile_chart_annotation_failure_uses_engine_error_patterns`
uses PostgresEngineSpec with a real psycopg2 connection-refused text
(`database_connection_error`) and a Postgres syntax error that must stay
`compile_error`. I checked the syntax case can fail: forcing every annotation
failure to count as a connection error makes it fail.
`tests/unit_tests/mcp_service/chart/test_compile.py::test_compile_chart_annotation_dbapi_error_is_connection_error`
puts a `SupersetDBAPIError` in the chain; it failed before the fix and passes
now.
Siblings checked:
- tests/unit_tests/mcp_service/chart/test_compile.py:1399 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a (cross-engine case: annotation chart
on MySQL, dataset on Postgres)
- tests/unit_tests/mcp_service/chart/test_compile.py:1198 — not affected:
the existing BaseEngineSpec test still covers the validation-wrap cases, which
do not depend on engine patterns
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1523,7 +1527,9 @@ def configure_temporal_handling(
if x_is_temporal:
form_data["granularity_sqla"] = form_data.get("x_axis")
if time_grain:
- form_data["time_grain_sqla"] = time_grain
+ form_data["time_grain_sqla"] = (
+ time_grain.value if isinstance(time_grain, TimeGrain) else
time_grain
+ )
Review Comment:
Fixed in e0991386a2e89e3a2c2814caf410f66f72869e5a.
`build_query_dicts_from_form_data` in chart_helpers.py builds every MCP query
(`build_query_context_from_form_data` and the get_chart_data fallback both call
it). It replaces enum members with their values in `form_data` and
`extra_form_data` via `coerce_enum_values`, so a future enum field that a
mapper does not unwrap cannot reach template rendering. The per-mapper `.value`
calls stay so returned and saved form_data are plain too.
`configure_temporal_handling` is annotated `TimeGrain | str | None`.
Regression test:
`tests/unit_tests/mcp_service/chart/test_chart_helpers.py::test_build_query_dicts_from_form_data_passes_enum_values_as_strings`
passes a TimeGrain and an unrelated nested Enum. It failed before the fix
(`extras.time_grain_sqla` was a TimeGrain) and passes now.
Siblings checked:
- superset/mcp_service/chart/chart_helpers.py:1191 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
- superset/mcp_service/chart/chart_utils.py:1513 — fixed in
e0991386a2e89e3a2c2814caf410f66f72869e5a
- superset/mcp_service/chart/plugins/treemap.py:200 — not affected: runs
inside build_query_dicts_from_form_data after the coercion (same for
waterfall.py:226, geographic.py:247, histogram.py:232, gantt.py:275)
- superset/mcp_service/chart/tool/get_chart_data.py:746 — not affected:
saved params are loaded from JSON and cannot hold enums
- superset/mcp_service/chart/plugins/interactive_pivot.py:101 — not
affected: already unwraps with .value (as do chart_utils.py:2056, 2693, 2914,
3039, 3317, 3455)
--
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]
