EnxDev commented on code in PR #44004:
URL: https://github.com/apache/superset/pull/44004#discussion_r3992403504
##########
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:
Thanks for chasing this through. I checked a07f05c: the guest-only lookup
happens after the committing lookup block and immediately before authorization
and validation, so the tamper guard no longer inherits the expired `Slice`. The
focused test file also passes locally (134 tests). This clears my blocker.
--
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]
