codeant-ai-for-open-source[bot] commented on code in PR #38890:
URL: https://github.com/apache/superset/pull/38890#discussion_r3006142867
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -807,6 +807,9 @@ async def generate_chart( # noqa: C901
return GenerateChartResponse.model_validate(result)
except (CommandException, SQLAlchemyError, KeyError, ValueError) as e:
+ from superset import db
+
+ db.session.rollback()
Review Comment:
**Suggestion:** Calling the database rollback directly inside the exception
handler means that if `db.session.rollback()` itself raises a `SQLAlchemyError`
(e.g., due to a broken connection), this new exception will escape the handler
and prevent the code from logging the original error and returning a
standardized error response, resulting in an unhandled failure instead of the
intended graceful error handling. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP `generate_chart` tool may crash on rollback failure.
- ⚠️ Standardized `ChartGenerationError` response may not be returned.
- ⚠️ Original failure context may be lost due to secondary error.
```
</details>
```suggestion
try:
db.session.rollback()
except SQLAlchemyError:
logger.warning(
"Database rollback failed during error handling",
exc_info=True
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP tool entrypoint `generate_chart` in
`superset/mcp_service/chart/tool/generate_chart.py` with any request that
triggers one of
the handled exceptions (`CommandException`, `SQLAlchemyError`, `KeyError`, or
`ValueError`) inside the main try block (e.g., a failing chart creation or
validation).
2. Control flows to the `except (CommandException, SQLAlchemyError,
KeyError, ValueError)
as e:` block near the end of `generate_chart`, where `from superset import
db` and
`db.session.rollback()` are executed (lines ~810–812 in the PR context).
3. Under a database-connection failure scenario (e.g., broken connection or
invalid
session state), SQLAlchemy can raise a new `SQLAlchemyError` from
`db.session.rollback()`
itself, before any of the subsequent logging and error-building logic
executes.
4. Because the rollback call is not wrapped in its own protection, this new
`SQLAlchemyError` escapes the handler, preventing the later `await
ctx.error(...)`,
`ChartErrorBuilder.build_error(...)`, and standardized
`GenerateChartResponse` return from
running, resulting in an unhandled exception for the MCP caller instead of
the intended
graceful error response.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/generate_chart.py
**Line:** 812:812
**Comment:**
*Possible Bug: Calling the database rollback directly inside the
exception handler means that if `db.session.rollback()` itself raises a
`SQLAlchemyError` (e.g., due to a broken connection), this new exception will
escape the handler and prevent the code from logging the original error and
returning a standardized error response, resulting in an unhandled failure
instead of the intended graceful error handling.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38890&comment_hash=c140bab1929bed495b6dd0ba0770751dfed2acac5c1efa422fcaecf1cb558780&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38890&comment_hash=c140bab1929bed495b6dd0ba0770751dfed2acac5c1efa422fcaecf1cb558780&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]
