codeant-ai-for-open-source[bot] commented on code in PR #39964:
URL: https://github.com/apache/superset/pull/39964#discussion_r3207521441
##########
superset/mcp_service/chart/compile.py:
##########
@@ -46,6 +49,21 @@
logger = logging.getLogger(__name__)
+_CONNECTION_ERROR_TYPES = {
+ SupersetErrorType.CONNECTION_INVALID_USERNAME_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_PASSWORD_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR,
+ SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_PORT_ERROR,
+ SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+ SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR,
+ SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
+ SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
Review 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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP generate_chart mislabels generic DB errors as connection failures.
- ⚠️ Preview-only compile checks report wrong root cause to users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Fcompile.py%0A%2A%2ALine%3A%2A%2A%2064%3A64%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20Including%20%60GENERIC_DB_ENGINE_ERROR%60%20in%20the%20connection-only%20allowlist%20causes%20%60_classify_as_database_error%28%29%60%20to%20mark%20almost%20any%20compile%20failure%20as%20a%20database%20connectivity%20issue%2C%20because%20%60db_engine_spec.extract_errors%28%29%60%20falls%20back%20to%20%60GENERIC_DB_ENGINE_ERROR%60%20for%20non-matching%20exceptions.%20This%20will%20misreport%20invalid%20SQL%2Fchart%20config%20errors%20as%20%22Unable%20to%20connect%20to%20the%20database.%22%20Remove%20this%20generic%20type%20from%20%60_CONNECTION_ERROR_TYPES%60%20%28or%20add%20stricter%20checks%20on%20the%20extracted%20message%2Ftype%29%20so%20only%20real%20connection%2Fauth%2Fnetwork%20
failures%20are%20classified%20as%20database%20connection%20errors.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Fcompile.py%0A%2A%2ALine%3A%2A%2A%2064%3A64%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncorrect%20Condition%20Logic%3A%20Including%20%60GENERIC_DB_ENGINE_ERROR%60%20in%20the%20connection-only%20al
lowlist%20causes%20%60_classify_as_database_error%28%29%60%20to%20mark%20almost%20any%20compile%20failure%20as%20a%20database%20connectivity%20issue%2C%20because%20%60db_engine_spec.extract_errors%28%29%60%20falls%20back%20to%20%60GENERIC_DB_ENGINE_ERROR%60%20for%20non-matching%20exceptions.%20This%20will%20misreport%20invalid%20SQL%2Fchart%20config%20errors%20as%20%22Unable%20to%20connect%20to%20the%20database.%22%20Remove%20this%20generic%20type%20from%20%60_CONNECTION_ERROR_TYPES%60%20%28or%20add%20stricter%20checks%20on%20the%20extracted%20message%2Ftype%29%20so%20only%20real%20connection%2Fauth%2Fnetwork%20failures%20are%20classified%20as%20database%20connection%20errors.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20a
sk%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39964&comment_hash=aab795e81425d03374103a8bbfa76cc30b37f92334b35b7272118283192f1dde&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39964&comment_hash=aab795e81425d03374103a8bbfa76cc30b37f92334b35b7272118283192f1dde&reaction=dislike'>👎</a>
--
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]
