deepujain commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4098752638
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1087,6 +1100,113 @@ def generate(
return strategy.generate()
+async def _run_png_render(
+ render: Callable[[], PNGPreview | ChartError],
+) -> PNGPreview | ChartError:
+ """Run the blocking PNG render in a worker thread with error mapping.
+
+ Chart-level access denials raised inside render() surface as Forbidden,
+ not as a rendering failure. Browser errors may contain URLs or page data,
+ so those stay server-side.
+ """
+ try:
+ return await asyncio.to_thread(render)
+ except SupersetSecurityException:
+ return ChartError(error="Chart access denied", error_type="Forbidden")
+ except Exception:
+ logger.exception("PNG chart rendering failed")
+ return ChartError(error="Chart rendering failed",
error_type="RenderError")
+
+
+async def _generate_png_preview(
+ chart_id: int, request: GetChartPreviewRequest
+) -> PNGPreview | ChartError:
+ """Render saved charts as the caller in an isolated browser and app
context."""
+ if (
+ not chart_id
+ or request.form_data_key
+ or request.extra_form_data
+ or guest_scope.is_guest_read()
+ ):
+ return ChartError(
+ error="PNG previews require a saved chart and a non-guest user, "
+ "without unsaved state or extra filters.",
+ error_type="UnsupportedFormat",
+ )
+ user_id = getattr(getattr(g, "user", None), "id", None)
+ if not isinstance(user_id, int):
+ return ChartError(error="Authentication required",
error_type="Forbidden")
+ width = 800 if request.width is None else request.width
+ height = 600 if request.height is None else request.height
+ if not (64 <= width <= 4096 and 64 <= height <= 4096):
+ return ChartError(
+ error="PNG dimensions must be between 64 and 4096 pixels.",
+ error_type="ValidationError",
+ )
+
+ def render() -> PNGPreview | ChartError:
+ from superset import is_feature_enabled
+ from superset.mcp_service.auth import _mcp_tool_call_context
+ from superset.utils.core import override_user
+ from superset.utils.screenshots import validate_screenshot_image
+ from superset.utils.urls import get_url_path
+ from superset.utils.webdriver import (
+ _PlaywrightBrowserManager,
+ WebDriverPlaywright,
+ )
+
+ # Resolve ORM objects in a fresh context; neither the request session
nor
+ # Flask's mutable g is shared with the rendering worker.
+ with _mcp_tool_call_context():
+ user = security_manager.find_user(id=user_id)
+ if user is None or not user.is_active:
+ return ChartError(
+ error="Authentication required", error_type="Forbidden"
+ )
+ with override_user(user):
+ chart = find_chart_by_identifier(chart_id)
+ if chart is None:
+ return ChartError(error="Chart not found",
error_type="NotFound")
+ security_manager.raise_for_access(chart=chart)
+ if is_feature_enabled(
+ "GRANULAR_EXPORT_CONTROLS"
+ ) and not security_manager.can_access("can_export_image",
"Superset"):
+ return ChartError(
+ error="Image export is not permitted",
error_type="Forbidden"
+ )
+ url = get_url_path(
+ "Superset.slice", slice_id=chart_id, standalone="true"
+ )
+ manager = _PlaywrightBrowserManager()
+ try:
+ driver = WebDriverPlaywright(
+ "",
+ (width, height),
+ require_complete_capture=True,
+ browser_manager=manager,
+ )
+ image = driver.get_screenshot(url, "chart-container",
user=user)
+ if (
+ image is None
+ or not image.startswith(b"\x89PNG\r\n\x1a\n")
+ or validate_screenshot_image(image)
+ ):
+ return ChartError(
+ error="Chart rendering failed",
error_type="RenderError"
+ )
+ with Image.open(BytesIO(image)) as rendered:
+ image_width, image_height = rendered.size
+ return PNGPreview(
+ data=base64.b64encode(image).decode("ascii"),
+ width=image_width,
+ height=image_height,
+ )
Review Comment:
Fixed in 3dbb1e1195. The default response-size configuration now excludes
get_chart_preview, and the README explicitly states that all preview formats
are outside max_bytes unless the operator removes that exclusion. A real
800x600 PNG exceeding the default budget now traverses
ResponseSizeGuardMiddleware.on_call_tool intact; the new test fails with
ToolError on the previous code. The existing default-config assertion was
updated to the new contract. All 308 preview, middleware and WebDriver tests
and staged hooks pass.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1087,6 +1100,113 @@ def generate(
return strategy.generate()
+async def _run_png_render(
+ render: Callable[[], PNGPreview | ChartError],
+) -> PNGPreview | ChartError:
+ """Run the blocking PNG render in a worker thread with error mapping.
+
+ Chart-level access denials raised inside render() surface as Forbidden,
+ not as a rendering failure. Browser errors may contain URLs or page data,
+ so those stay server-side.
+ """
+ try:
+ return await asyncio.to_thread(render)
Review Comment:
Fixed in 3dbb1e1195 with a dedicated two-worker PNG executor. It preserves
caller ContextVars and leaves the default authentication pool available. The
regression holds two render workers, cancels one awaiting request, verifies the
third capture cannot start early, and confirms default-pool work still
completes. That test fails on the old shared executor. Running captures still
finish under the existing screenshot timeouts; the README documents this limit.
--
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]
