codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3545215232
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -725,6 +726,7 @@ def test_info_security_dashboard(self):
"can_write",
"can_export",
"can_export_as_example",
+ "can_export_xlsx",
Review Comment:
**Suggestion:** The expected permission list includes `can_export_xlsx`, but
the new endpoint is decorated with `@permission_name("export")`, so it reuses
`can_export` rather than creating a new `can_export_xlsx` permission. This
makes the test assert a permission that is never registered and will fail;
update the expectation to match the actual permission mapping (or change the
endpoint permission name if a distinct permission is intended). [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard permissions info test fails expecting non-existent permission.
- ❌ CI pipeline blocked when integration tests run on PRs.
- ⚠️ Permission documentation confused by mismatch between tests and RBAC.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the integration test `test_info_security_dashboard` in
`tests/integration_tests/dashboards/api_tests.py:715-735`, which exercises
the dashboard
`_info` endpoint with `keys=["permissions"]`.
2. The test logs in as `ADMIN_USERNAME` and calls `GET
api/v1/dashboard/_info?q=...` via
`self.get_assert_metric(uri, "info")`, then parses `data["permissions"]`
from the
response.
3. The request is handled by `DashboardRestApi.info_headless` (defined in
`superset/views/base_api.py:35-41` and inherited by
`superset/dashboards/api.py`), which
builds the permissions list from `DashboardRestApi.include_route_methods` in
`superset/dashboards/api.py:250-274`.
4. The Excel export endpoint `export_xlsx` at
`superset/dashboards/api.py:1487-1491` is
decorated with `@permission_name("export")`, so it contributes the existing
`can_export`
permission rather than a new `can_export_xlsx` permission; since
`can_export_xlsx` only
appears in the test expectation at
`tests/integration_tests/dashboards/api_tests.py:729`
and is never registered anywhere else, the returned `data["permissions"]`
set does not
contain `"can_export_xlsx"`, causing the assertion to fail and the test to
error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9882931c0c134ffca919ff6e0c22f852&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9882931c0c134ffca919ff6e0c22f852&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/dashboards/api_tests.py
**Line:** 729:729
**Comment:**
*Api Mismatch: The expected permission list includes `can_export_xlsx`,
but the new endpoint is decorated with `@permission_name("export")`, so it
reuses `can_export` rather than creating a new `can_export_xlsx` permission.
This makes the test assert a permission that is never registered and will fail;
update the expectation to match the actual permission mapping (or change the
endpoint permission name if a distinct permission is intended).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=b7da08561e0be5fca46fae2c5fa211e15a3b2618b7526b3eb1dbd60ca3e2e15b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=b7da08561e0be5fca46fae2c5fa211e15a3b2618b7526b3eb1dbd60ca3e2e15b&reaction=dislike'>👎</a>
##########
superset/dashboards/api.py:
##########
@@ -1471,6 +1484,109 @@ def export_as_example(self, pk: int) -> Response:
response.set_cookie(token, "done", max_age=600)
return response
+ @expose("/<pk>/export_xlsx/", methods=("POST",))
+ @protect()
+ @safe
+ @permission_name("export")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.export_xlsx",
+ log_to_statsd=False,
+ )
+ def export_xlsx(self, pk: int) -> WerkzeugResponse:
+ """Export all of a dashboard's chart data to an Excel workbook (async).
+ ---
+ post:
+ summary: Export dashboard chart data to Excel
+ description: >-
+ Enqueues an async task that writes each chart's data to its own
+ worksheet, uploads the .xlsx to S3, and emails the requesting user
a
+ pre-signed download link. Returns immediately with a job id.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The dashboard id
+ requestBody:
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/DashboardExportXlsxPostSchema'
+ responses:
+ 202:
+ description: Export task accepted
+ content:
+ application/json:
+ schema:
+ $ref:
'#/components/schemas/DashboardExportXlsxResponseSchema'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ 501:
+ description: Excel export is not configured on this server
+ """
+ if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]:
+ return self.response(
+ 501, message="Excel export is not configured on this server."
+ )
+ try:
+ # Tolerate an empty/non-JSON body (e.g. a POST with no
Content-Type);
+ # request.json would otherwise raise 415.
+ payload = DashboardExportXlsxPostSchema().load(
+ request.get_json(silent=True) or {}
+ )
+ except ValidationError as error:
+ return self.response_400(message=error.messages)
+
+ dashboard = cast(Dashboard, self.datamodel.get(pk, self._base_filters))
+ if not dashboard:
+ return self.response_404()
+ try:
+ security_manager.raise_for_access(dashboard=dashboard)
+ except SupersetSecurityException:
+ return self.response_403()
+
+ # Email delivery is the only result channel, so an account with an
email
+ # address is required; embedded guest users are excluded in this
version.
+ if isinstance(g.user, GuestUser) or not getattr(g.user, "email", None):
+ return self.response_400(
+ message="Excel export requires an account with an email
address."
+ )
+ if not dashboard.slices:
+ return self.response_400(message="Dashboard has no charts to
export.")
+
+ # Throttle: one concurrent export per user+dashboard. The lock is set
+ # here and cleared by the task; its TTL guards against a lost cleanup.
+ inflight_key = f"excel-export-inflight:{g.user.id}:{dashboard.id}"
+ if cache_manager.cache.get(inflight_key):
+ return self.response(
+ 202,
+ message="An Excel export for this dashboard is already in
progress.",
+ )
Review Comment:
**Suggestion:** The endpoint advertises a `job_id` response schema for
`202`, but the duplicate-in-flight branch returns only a message, breaking API
contract consistency for clients that expect a correlation id. Return the
existing job id from cache in this branch (or use a different status
code/schema for “already running”). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ API contract misleads clients expecting job_id in 202.
- ⚠️ Integrations need custom handling for "already running" responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the OpenAPI description on `export_xlsx` in
`superset/dashboards/api.py:97-135`,
where the `202` response references
`#/components/schemas/DashboardExportXlsxResponseSchema`.
2. Inspect `DashboardExportXlsxResponseSchema` in
`superset/dashboards/schemas.py:20-23`,
which defines a single `job_id` field described as “Correlation id for the
async export
task.”
3. Trigger an export for a dashboard (configured as in previous steps) so
the first `POST
/api/v1/dashboard/<pk>/export_xlsx/` sets an in-flight lock and enqueues a
job; while that
job is still running and the lock exists, send another POST to the same
endpoint as the
same user.
4. The second request hits the throttle logic at
`superset/dashboards/api.py:1568-174`;
since `cache_manager.cache.get(inflight_key)` is truthy, it returns
`self.response(202,
message="An Excel export for this dashboard is already in progress.",)`
without a `job_id`
field, even though the documented `202` schema
(`DashboardExportXlsxResponseSchema`) only
defines `job_id`; clients built against the schema expecting `job_id` for
all `202`
responses must special-case this branch or will fail to correlate the
already-running job.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6b464ff96b1d4ca6b3bb98dd9a1d68fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6b464ff96b1d4ca6b3bb98dd9a1d68fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 1569:1573
**Comment:**
*Api Mismatch: The endpoint advertises a `job_id` response schema for
`202`, but the duplicate-in-flight branch returns only a message, breaking API
contract consistency for clients that expect a correlation id. Return the
existing job id from cache in this branch (or use a different status
code/schema for “already running”).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=2779f60c6d2f1e2a531d957436f746d97f5a92a6403c259809099e252c226392&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=2779f60c6d2f1e2a531d957436f746d97f5a92a6403c259809099e252c226392&reaction=dislike'>👎</a>
##########
superset/dashboards/excel_export/screenshot.py:
##########
@@ -0,0 +1,98 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Render a single dashboard chart to a PNG for the image-mode Excel export.
+
+This reuses the same headless render path scheduled reports use
+(:class:`~superset.utils.screenshots.ChartScreenshot`), but points it at an
+Explore URL whose ``form_data`` carries the live dashboard filter state — so an
+embedded image reflects the same filters the data path applies, rather than the
+chart's default saved state.
+"""
+
+from __future__ import annotations
+
+import logging
+from typing import Any
+
+from flask import current_app
+
+from superset.charts.data.dashboard_filter_context import (
+ get_dashboard_filter_context,
+)
+from superset.utils import json
+from superset.utils.screenshots import ChartScreenshot
+from superset.utils.urls import get_url_path
+
+logger = logging.getLogger(__name__)
+
+
+def render_chart_image(
+ chart: Any,
+ dashboard_id: int,
+ active_data_mask: dict[str, Any],
+ user: Any,
+) -> bytes | None:
+ """
+ Render ``chart`` (as seen on ``dashboard_id``) to PNG bytes.
+
+ The chart is rendered through Explore in standalone mode with the live
+ dashboard filter state injected as ``extra_form_data`` — the same object
the
+ data path merges into the query context — so the image and the data stay
+ consistent.
+
+ :param chart: The ``Slice`` to render
+ :param dashboard_id: The dashboard the chart is displayed on (for filter
scope)
+ :param active_data_mask: Live dashboard filter state keyed by native
filter id
+ :param user: The requesting user; the render runs with their permissions
+ :returns: PNG bytes, or ``None`` if the render failed (the caller skips and
+ notes the chart)
+ """
+ try:
+ filter_context = get_dashboard_filter_context(
+ dashboard_id=dashboard_id,
+ chart_id=chart.id,
+ active_data_mask=active_data_mask,
+ )
+
+ # Start from the chart's saved form data and force the slice id, then
+ # layer the live filters on top so the render matches the data path.
+ form_data: dict[str, Any] = json.loads(chart.params or "{}")
+ form_data["slice_id"] = chart.id
+ if filter_context.extra_form_data:
+ form_data["extra_form_data"] = filter_context.extra_form_data
Review Comment:
**Suggestion:** This assignment overwrites any chart-saved `extra_form_data`
instead of merging, which can drop chart-level filters/time settings and make
image exports diverge from data exports. Merge dashboard filter context into
existing `form_data["extra_form_data"]` rather than replacing it. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Image-mode exports may omit chart-level extra_form_data constraints.
- ⚠️ Data and image sheets can diverge, confusing dashboard consumers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger an Excel export in image mode by calling `POST
/api/v1/dashboard/<pk>/export_xlsx/` with `{"active_data_mask": ..., "mode":
"images"}` as
the frontend does in
`superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:31-36`;
this
enqueues `export_dashboard_excel` with `mode="images"`
(`superset/tasks/export_dashboard_excel.py:251-258`).
2. In the task, `_build_workbook` at
`superset/tasks/export_dashboard_excel.py:168-200`
iterates charts; for charts rendered as images (`_renders_as_image` at lines
96-98), it
calls `_write_chart_image_sheet`, which in turn calls
`render_chart_image(chart,
dashboard.id, active_data_mask, user)`
(`superset/tasks/export_dashboard_excel.py:101-116`).
3. `render_chart_image` in
`superset/dashboards/excel_export/screenshot.py:44-78` loads
the chart's saved form data JSON from `chart.params` (line 74), sets
`form_data["slice_id"] = chart.id`, and then, when
`filter_context.extra_form_data` is
present, unconditionally assigns `form_data["extra_form_data"] =
filter_context.extra_form_data` at lines 76-77, overwriting any
`extra_form_data` already
saved with the chart.
4. For charts that rely on saved `extra_form_data`—for example, multi-layer
visualizations
where `_apply_layer_filtering` in `superset/viz.py:35-63` processes
`self.form_data["extra_form_data"]` to produce layer-specific filters—this
overwrite drops
chart-level `extra_form_data` instead of merging dashboard filter
contributions into it;
the Explore URL built at
`superset/dashboards/excel_export/screenshot.py:79-82` therefore
uses only dashboard filter context, so the rendered PNG in image-mode
exports can diverge
from the data path (which applies dashboard filters via
`apply_dashboard_filter_context`
at `superset/tasks/export_dashboard_excel.py:138-145` while preserving
non-overridden
fields), leading to embedded images that omit chart constraints present in
exported data
sheets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=262fa4c2bef44ce085b0c24351431470&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=262fa4c2bef44ce085b0c24351431470&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/excel_export/screenshot.py
**Line:** 76:77
**Comment:**
*Logic Error: This assignment overwrites any chart-saved
`extra_form_data` instead of merging, which can drop chart-level filters/time
settings and make image exports diverge from data exports. Merge dashboard
filter context into existing `form_data["extra_form_data"]` rather than
replacing it.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=f8ca1f498e007b190d0f474368a67b1fd5122d66cd7994dec3129ad62e8a610e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=f8ca1f498e007b190d0f474368a67b1fd5122d66cd7994dec3129ad62e8a610e&reaction=dislike'>👎</a>
--
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]
