eschutho commented on code in PR #44351:
URL: https://github.com/apache/superset/pull/44351#discussion_r4354540346
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -509,6 +521,15 @@ async def _handle_chart_sql_request(
error_type="Unsupported",
)
+ # Eagerly refresh all attributes while the session is still active. The
+ # log_context commit above (DBEventLogger.log) expires this instance via
+ # SQLAlchemy's expire_on_commit, and the awaits below are real event-loop
+ # yield points after which the MCP per-call session can be torn down —
+ # a later lazy reload (e.g. chart.params in _resolve_effective_form_data,
+ # chart.query_context in _sql_from_saved_query_context) would then raise
+ # DetachedInstanceError. See get_chart_preview.py for the same pattern.
+ db.session.refresh(chart)
Review Comment:
You're right — `chart.datasource_type` is read right after the lookup and
already loads all the expired column attributes in one go, so the explicit
`refresh()` added nothing. Removed it in 1daf090657; the PR now only adds the
`SQLAlchemyError` -> `ChartError` handler (without leaking error text), and the
tests exercise that handler directly.
--
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]
