gabotorresruiz commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4087693234
##########
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:
Not a blocker, and I am not reopening the cancellation point you already
answered. This is a different property of the same call.
`asyncio.to_thread` submits to the loop's *default* executor (it is
literally `loop.run_in_executor(None, ...)`), which is the same pool that
`superset/mcp_service/composite_token_verifier.py:184` and
`superset/mcp_service/guest_token_verifier.py:153` use for API key and guest
token verification. That pool is `min(32, cpu_count + 4)` threads, and a render
holds its thread for the whole capture, which by your own note is not released
early when the request is cancelled. So a handful of concurrent `format="png"`
calls can sit on the same threads every other request needs for transport auth.
Would a dedicated bounded executor, or an `asyncio.Semaphore` around the
render, be worth it here? It also gives you a natural place to cap how many
Chromium instances one process will launch at once, since
`_PlaywrightBrowserManager()` is created per call.
##########
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:
This block worries me a bit. With the shipped defaults I do not think a real
chart can come back through this path at all.
`ResponseSizeGuardMiddleware` runs on every tool call, and
`get_chart_preview` is in none of `INFO_TOOLS`, `DATA_QUERY_TOOLS`,
`COMMITTED_WRITE_TOOLS` or `STRING_FIELD_TRUNCATION_TOOLS`
(`superset/mcp_service/utils/response_size_utils.py`), and it is not in
`excluded_tools` in `MCP_RESPONSE_SIZE_CONFIG`
(`superset/mcp_service/mcp_config.py`). So `_handle_oversized_response` falls
all the way through to the hard `ToolError`. The default budget is
`DEFAULT_MAX_RESPONSE_BYTES = 50_000`.
I verified it on this branch. I rendered an ECharts line chart in headless
Chromium at the default `800x600` with `device_scale_factor=1`, put those exact
bytes into a `PNGPreview` inside a real `ChartPreview`, and called a tool named
`get_chart_preview` through a real `FastMCP` client with the real middleware
attached:
```text
raw PNG bytes : 96272
base64 chars : 128364
measured response bytes : 128944
guard max_bytes : 50000
is_error: True
"Response too large: 128,944 bytes (limit: 50,000) ... Reduction needed:
~61%"
```
The caller pays for the full Chromium launch and capture and then gets an
error instead of the image, and that error suggests narrowing a query that has
no narrowing knob. Sweeping the viewport on the same chart, `320x240` fit under
the budget and `400x300` was already over, so there is no `width`/`height`
inside your `64..4096` range that yields a useful preview.
Options, in the order I would pick them:
* add `get_chart_preview` to `excluded_tools` in `MCP_RESPONSE_SIZE_CONFIG`,
and say in the README that a PNG response is not bounded by `max_bytes`
* or return the image as an MCP image content block instead of base64 inside
the JSON payload, so it never counts against the text budget
* or keep the guard and bound the output (downscale, or JPEG/WebP) with a
documented ceiling
Whichever you pick, could we get a test in `test_get_chart_preview.py` that
pushes a realistically sized PNG through
`ResponseSizeGuardMiddleware.on_call_tool` and asserts a `PNGPreview` comes
back rather than a `ToolError`? Every current PNG test stops at
`_generate_png_preview`, which is exactly why this is invisible today.
`EMBEDDED_DIMENSIONS_MAX_PAGE_SIZE` in
`superset/mcp_service/semantic_layer/schemas.py` is a nice in repo example of
budgeting a response against this same limit.
Or am I missing a place where the guard is relaxed for this tool?
--
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]
