fix(mcp): prevent DetachedInstanceError in get_chart_preview#39921
fix(mcp): prevent DetachedInstanceError in get_chart_preview#39921aminghadersohi wants to merge 2 commits intoapache:masterfrom
Conversation
Call db.session.refresh() immediately after find_chart_by_identifier() so the Slice object's attributes are loaded into memory while the session is still active. Without this, a commit or expire inside validate_chart_dataset() can detach the object, causing DetachedInstanceError when strategy classes access lazy attributes. Also adds an explicit except SQLAlchemyError handler inside _get_chart_preview_internal so that any residual session errors return a clear ChartError with a "please retry" message rather than propagating to the outer wrapper with a generic message. Adds unit tests covering both the refresh call and the DetachedInstanceError fallback path. Closes #105716
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39921 +/- ##
==========================================
- Coverage 63.88% 63.88% -0.01%
==========================================
Files 2583 2583
Lines 136604 136610 +6
Branches 31502 31503 +1
==========================================
Hits 87276 87276
- Misses 47812 47818 +6
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:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the MCP get_chart_preview tool against intermittent SQLAlchemy session detachment issues by proactively refreshing loaded Slice instances and converting SQLAlchemy exceptions into a user-friendly ChartError response.
Changes:
- Refreshes the loaded chart ORM instance immediately after lookup to reduce the chance of later lazy-load access on a detached/expired instance.
- Adds a dedicated
except SQLAlchemyErrorhandler to return a clearer, retry-orientedChartErrorinstead of bubbling a generic failure. - Adds unit tests covering the refresh call and
DetachedInstanceError→ChartErrorbehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
superset/mcp_service/chart/tool/get_chart_preview.py |
Refreshes chart after lookup and adds explicit SQLAlchemy exception handling for clearer error responses. |
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py |
Adds tests asserting refresh invocation and that DetachedInstanceError is converted into a ChartError. |
- Broaden the db.session.refresh() comment to describe SQLAlchemy's general expire-on-commit behaviour rather than pointing at a specific function as the source of detachment - Switch logger.error() to logger.exception() in the SQLAlchemyError handler so the full stack trace is captured alongside the ctx.error message, aiding production debugging of intermittent session issues - Replace the MagicMock URLPreview stub in the refresh test with a real URLPreview instance so Pydantic discriminated-union validation succeeds; add assertions that the response is a ChartPreview (not a ChartError) to confirm the happy path is actually exercised
|
Thanks for the review! All three points addressed in the follow-up commit (546c805):
|
Code Review Agent Run #2c0f2bActionable 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 |
SUMMARY
get_chart_previewintermittently raisedDetachedInstanceErrorwhen aSliceORM object was loaded byfind_chart_by_identifier()and then accessed after a session expire insidevalidate_chart_dataset().Two changes:
Eager session refresh — call
db.session.refresh(chart)immediately after loading theSliceso all column attributes are populated in the object's__dict__while the session is still active. This preventsDetachedInstanceErrorwhen strategy classes later access lazy attributes.SQLAlchemy error handler — add an explicit
except SQLAlchemyErrorblock inside_get_chart_preview_internalso any residual session errors (e.g.DetachedInstanceError) produce a clearChartErrorwith a "database session error, please retry" message instead of propagating to the outer wrapper with a generic message.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only fix.
TESTING INSTRUCTIONS
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py::TestDetachedInstanceError:test_session_refresh_called_after_chart_load— verifiesdb.session.refresh()is called once after chart lookuptest_detached_instance_error_returns_chart_error— verifiesDetachedInstanceErrorraised inside the preview strategy returns aChartError(not an unhandled exception)pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py::TestDetachedInstanceError -xvsADDITIONAL INFORMATION