EnxDev commented on code in PR #44004:
URL: https://github.com/apache/superset/pull/44004#discussion_r3970155407
##########
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:
**[P1] Keep the embedded-guest path detached-safe too.** The snapshot
protects the ordinary paths, but an embedded guest still passes the original
`chart` to `guest_scope.authorize_query()` later, after this lookup log context
has committed. That helper immediately reads `chart.id`, and
`ChartDataCommand.validate()` then follows `query_context.slice_` into `id`,
`query_context`, and `params_dict` in the guest tamper check. If this Slice is
detached in the scenario this PR reproduces, the guest request still ends as
`InternalError`. Could we re-fetch or reattach the Slice for the authorization
step (or otherwise give the tamper guard an attached stored chart), and extend
the regression test so the real guest authorization and validation path runs
after detachment?
--
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]
