geido commented on code in PR #44004:
URL: https://github.com/apache/superset/pull/44004#discussion_r3989537142
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -416,38 +431,66 @@ async def get_chart_data( # noqa: C901
chart = find_chart_by_identifier(
request.identifier, query_options=chart_query_options
)
- if chart is not None:
- guest_dashboard_id = guest_scope.guest_dashboard_id(chart)
+ if not chart:
+ await ctx.warning(
+ "Chart not found: identifier=%s" % (request.identifier,)
+ )
+ logger.warning(
+ "get_chart_data: chart not found: identifier=%s",
request.identifier
+ )
+ display_id = str(request.identifier)[:200]
+ return ChartError(
+ error=(
+ f"No chart found with identifier: {display_id}."
+ " Use list_charts to get valid chart IDs."
+ ),
+ error_type="NotFound",
+ )
- if not chart:
- await ctx.warning("Chart not found: identifier=%s" %
(request.identifier,))
- logger.warning(
- "get_chart_data: chart not found: identifier=%s",
request.identifier
- )
- display_id = str(request.identifier)[:200]
- return ChartError(
- error=(
- f"No chart found with identifier: {display_id}."
- " Use list_charts to get valid chart IDs."
- ),
- error_type="NotFound",
+ # Copy the values this function needs into plain locals while the
+ # instance is freshly loaded and still attached.
+ #
+ # Reading them off the ORM object later is not safe: exiting an
+ # event_logger.log_context() commits the session
+ # (DBEventLogger.log -> db.session.commit), and a commit expires
+ # every loaded attribute. If the instance is also detached before
+ # the next read -- this tool is async and crosses many await
+ # points -- that read raises DetachedInstanceError, which the
+ # broad SQLAlchemyError handler below turns into a confusing
+ # internal-session error instead of chart data. Plain locals are
+ # immune to both expiry and detachment.
+ chart_id = chart.id
+ chart_name = chart.slice_name
+ chart_viz_type = chart.viz_type
+ chart_datasource_id = chart.datasource_id
+ chart_datasource_type = chart.datasource_type
+ chart_params = chart.params
+ chart_query_context = chart.query_context
+ chart_facts = _ChartFacts(
+ chart_id,
+ chart_name,
+ chart_viz_type,
+ chart_datasource_id,
+ chart_datasource_type,
)
+ guest_dashboard_id = guest_scope.guest_dashboard_id(chart)
Review Comment:
Fixed in a07f05c9ce — confirmed and addressed.
Reproduced it first: with the looked-up `Slice` detached, the guest request
ends as `InternalError: Instance <Slice at 0x0> is not bound to a Session;
attribute refresh operation cannot proceed (attribute: id)`, exactly as you
described. Verified the guard reads all three — `stored_chart.id`,
`stored_chart.query_context` and `stored_chart.params_dict`
(`superset/security/manager.py:1626` onward).
Went with the re-fetch rather than a stand-in. The snapshotted scalars
deliberately can't be used here: `query_context_modified` compares the guest
payload *against the stored chart*, so substituting values this request already
carries would make the comparison self-referential. The re-fetch goes back
through `find_chart_by_identifier`, so the guest's `ChartFilter` still applies,
and an empty result fails closed with `NotFound` instead of proceeding
unauthorized.
Regression test added as you suggested — it runs the real
`guest_scope.authorize_query` after the chart detaches, then asserts the pinned
`query_context.slice_` is the re-fetched instance and that each of the three
attributes is readable. It fails without the re-fetch with the error above.
Side effect worth noting: the ORM instance no longer escapes the lookup
block at all.
ruff-format also fixed — a missing blank line where the rebase merged your
xlsxwriter test against the new fixture.
--
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]
