fix(mcp): raise right error#39964
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR, | ||
| SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR, | ||
| SupersetErrorType.CONNECTION_DATABASE_TIMEOUT, | ||
| SupersetErrorType.GENERIC_DB_ENGINE_ERROR, |
There was a problem hiding this comment.
Suggestion: Including GENERIC_DB_ENGINE_ERROR in the connection-only allowlist causes _classify_as_database_error() to mark almost any compile failure as a database connectivity issue, because db_engine_spec.extract_errors() falls back to GENERIC_DB_ENGINE_ERROR for non-matching exceptions. This will misreport invalid SQL/chart config errors as "Unable to connect to the database." Remove this generic type from _CONNECTION_ERROR_TYPES (or add stricter checks on the extracted message/type) so only real connection/auth/network failures are classified as database connection errors. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ MCP generate_chart mislabels generic DB errors as connection failures.
- ⚠️ Preview-only compile checks report wrong root cause to users.Steps of Reproduction ✅
1. Call the MCP `generate_chart` tool, implemented at
`superset/mcp_service/chart/tool/generate_chart.py:93-153`, with a `GenerateChartRequest`
whose config produces a query-time database error that is not a connection/auth/network
issue (for example, an invalid SQL expression in a virtual dataset or an incompatible
aggregate), so that the underlying query fails at runtime.
2. When `generate_chart` runs the compile check after creating or preparing the chart, it
calls `_compile_chart(form_data, dataset.id)` in the saved-chart path at
`generate_chart.py:120-126` or in the preview-only path at `generate_chart.py:239-246`,
which routes into `_compile_chart` in `superset/mcp_service/chart/compile.py:133-205`.
3. Inside `_compile_chart` (`compile.py:156-205`), the `ChartDataCommand.run()` call can
raise `ChartDataQueryFailedError` (as defined in
`superset/commands/chart/exceptions.py:138-143` and raised in
`superset/commands/chart/data/get_data_command.py:15-22`) when `payload["queries"]`
contains an `"error"` string from the database; this exception is caught in the `except
(ChartDataQueryFailedError, ChartDataCacheLoadError) as exc` block at
`compile.py:206-226`.
4. In that exception handler, `_classify_as_database_error(exc, dataset_id)` is invoked
(`compile.py:206-208`). `_classify_as_database_error` (`compile.py:66-95`) calls
`dataset.database.db_engine_spec.extract_errors(exc)` (`compile.py:84-87`), which uses the
engine spec implementation `BaseEngineSpec.extract_errors` at
`superset/db_engine_specs/base.py:31-41`. When the error message doesn't match any custom
regex in `custom_errors` or `CUSTOM_DATABASE_ERRORS`, `extract_errors` returns a single
`SupersetError` with `error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR` as its
fallback (`base.py:35-41`). Because `_CONNECTION_ERROR_TYPES` in `compile.py:52-65`
explicitly includes `SupersetErrorType.GENERIC_DB_ENGINE_ERROR`, the `any(e.error_type in
_CONNECTION_ERROR_TYPES for e in errors)` check at `compile.py:86-87` evaluates to True
even for non-connection engine errors, causing `_classify_as_database_error` to return
True and `_compile_chart` to treat the failure as a database connection error, returning
`_build_database_error(...)` (`compile.py:207-219)`. This produces a
`ChartGenerationError` with `error_type="database_connection_error"` and message `"Unable
to connect to the database."` (`compile.py:98-110`), which is then surfaced back to
`generate_chart` where `compile_result.error_obj` is used as the error payload
(`generate_chart.py:143-158` and `generate_chart.py:15-31`), misleading users into
thinking the database is unreachable when the underlying issue was a generic engine/SQL
error.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/compile.py
**Line:** 64:64
**Comment:**
*Incorrect Condition Logic: Including `GENERIC_DB_ENGINE_ERROR` in the connection-only allowlist causes `_classify_as_database_error()` to mark almost any compile failure as a database connectivity issue, because `db_engine_spec.extract_errors()` falls back to `GENERIC_DB_ENGINE_ERROR` for non-matching exceptions. This will misreport invalid SQL/chart config errors as "Unable to connect to the database." Remove this generic type from `_CONNECTION_ERROR_TYPES` (or add stricter checks on the extracted message/type) so only real connection/auth/network failures are classified as database connection errors.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
We include GENERIC_DB_ENGINE_ERROR because many engines (BigQuery, Snowflake, Athena, Databricks, Trino) lack specific CONNECTION_* regex patterns — all their connection failures fall back to this type. Excluding it would mean connection errors on these databases still show as "chart configuration is invalid," defeating the purpose of this fix.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Keep
SupersetErrorType.GENERIC_DB_ENGINE_ERRORin the connection-error allowlist for chart compilation so engines without specificCONNECTION_*patterns still surface real database connection failures correctly.
Applied to:
superset/mcp_service/chart/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
The flagged issue is correct: including GENERIC_DB_ENGINE_ERROR in _CONNECTION_ERROR_TYPES causes _classify_as_database_error to misclassify non-connection errors as database connectivity issues, since extract_errors falls back to it for unmatched exceptions. Removing GENERIC_DB_ENGINE_ERROR from the set improves accuracy by limiting connection error classification to specific types. Here's the updated code snippet for _CONNECTION_ERROR_TYPES: superset/mcp_service/chart/compile.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39964 +/- ##
==========================================
- Coverage 63.88% 63.87% -0.02%
==========================================
Files 2583 2583
Lines 136604 136651 +47
Branches 31502 31508 +6
==========================================
+ Hits 87276 87281 +5
- Misses 47812 47854 +42
Partials 1516 1516
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #487b42
Actionable Suggestions - 1
-
superset/mcp_service/chart/compile.py - 1
- Overly broad exception catch blocks · Line 347-347
Additional Suggestions - 3
-
superset/mcp_service/chart/compile.py - 3
-
Performance Regression · Line 341-346Adding a database query (DatasetDAO.find_by_id) in the error handling path of chart compilation may impact performance when errors occur, as the compile check is intended to be fast.
-
Code Duplication · Line 213-219The two except blocks return identical CompileResult for database errors. Consider extracting a helper function to avoid duplication.
-
Broad Exception Handling · Line 347-347The except Exception clause catches all exceptions, which may mask unexpected errors. Consider catching more specific exceptions or removing the broad catch if not necessary.
-
Review Details
-
Files reviewed - 3 · Commit Range:
2c2aaa7..2c2aaa7- superset/mcp_service/chart/compile.py
- superset/mcp_service/chart/tool/generate_chart.py
- tests/unit_tests/mcp_service/chart/test_compile.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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) | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
Line 347 catches bare Exception which is too broad. Catch specific exception types instead (e.g., AttributeError, KeyError, TypeError) to avoid masking unexpected errors.
Code suggestion
Check the AI-generated fix before applying
| except Exception: # pylint: disable=broad-except | |
| except (AttributeError, KeyError, TypeError): |
Code Review Run #487b42
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #46d5f4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Problem:
When a database is unreachable, MCP's
generate_charttells the user "Chart query failed to execute. The chart configuration is invalid" with suggestions like "Check that all columns exist." Users think their chart config is wrong when the real problem is the database is down.Root cause:
Superset's query execution layer (
helpers.py) already creates structuredSupersetErrorobjects with proper error types (e.g.CONNECTION_HOST_DOWN_ERROR) viadb_engine_spec.extract_errors(). However,QueryCacheManagerdrops the structurederrorslist — only the raw error string survives. By the time_compile_chart()sees the error, it can't distinguish a connection failure from a config issue, so it always returns the same generic message.Solution:
Propagate the structured errors through the pipeline (3 additive lines in core), then use them in
_compile_chart()to return the correct error type.QueryCacheManager: preservequery_result.errorsalongsideerror_messageQueryContextProcessor: includeerrorsin the payload dict_compile_chart(): bypassChartDataCommand(which raises and loses the structured errors) — runQueryContext.get_payload()directly and checkquery["errors"]forCONNECTION_*typesgenerate_chart.py: usecompile_result.error_objinstead of hardcoding a generic errorBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION