codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3574638505
##########
superset/dashboards/api.py:
##########
@@ -1574,6 +1590,123 @@ 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. Acquire a shared,
+ # atomic distributed lock (Redis when configured, the metadata DB
+ # otherwise) so the guard works across the web server and workers and
is
+ # not a no-op under the default cache. The task releases it when it
+ # settles; the TTL is the backstop if that release is ever lost.
+ lock_params = export_lock_params(g.user.id, dashboard.id)
+ try:
+ AcquireDistributedLock(
+ EXPORT_LOCK_NAMESPACE,
+ lock_params,
+ ttl_seconds=EXPORT_LOCK_TTL_SECONDS,
+ ).run()
+ except LockAlreadyHeldException:
+ return self.response(
+ 202,
+ message="An Excel export for this dashboard is already in
progress.",
+ )
Review Comment:
**Suggestion:** The endpoint documents `202` as returning a job identifier,
but this `202` branch returns only a message when a lock is already held, which
breaks response-contract consistency for clients expecting `job_id`. Return a
different status (for example conflict) for duplicate in-flight exports or
include a job identifier in this path too. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Duplicate exports return 202 without documented job_id.
- ⚠️ API clients relying job_id see inconsistent payload.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From the dashboard UI, trigger an Excel export (frontend handler
`onExportXlsx` in
`superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:31-41`
issues
`POST /api/v1/dashboard/<id>/export_xlsx/` with `active_data_mask` and
`mode`).
2. Ensure the first request is accepted: `DashboardRestApi.export_xlsx` in
`superset/dashboards/api.py:94-209` enqueues the Celery task and returns
`self.response(202, job_id=job_id)` (see lines 191-209), matching
`DashboardExportXlsxResponseSchema.job_id` defined at
`superset/dashboards/schemas.py:135-138`.
3. While the first export task `export_dashboard_excel` (in
`superset/tasks/export_dashboard_excel.py:7-87`) is still running and the
lock has not yet
been released (release happens in the `finally` block at lines 88-99),
immediately trigger
a second export for the same dashboard and user via the same endpoint.
4. On this second request, `export_xlsx` attempts to acquire the distributed
lock and hits
the `except LockAlreadyHeldException` branch at
`superset/dashboards/api.py:1684-1688`,
returning `self.response(202, message="An Excel export for this dashboard is
already in
progress.")` without a `job_id`, even though the OpenAPI doc for the `202`
response at
lines 124-129 specifies `DashboardExportXlsxResponseSchema` (which only
defines `job_id`).
API clients relying on that schema will receive a `202` without the
documented identifier.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=da7e3976d24b4aeab89b3663e6a09cf8&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=da7e3976d24b4aeab89b3663e6a09cf8&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:** 1684:1688
**Comment:**
*Api Mismatch: The endpoint documents `202` as returning a job
identifier, but this `202` branch returns only a message when a lock is already
held, which breaks response-contract consistency for clients expecting
`job_id`. Return a different status (for example conflict) for duplicate
in-flight exports or include a job identifier in this path too.
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=61358f7d3e43a0eeb0b037e041c49869556ebf6c7e6f13a027fd35265d25e63c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=61358f7d3e43a0eeb0b037e041c49869556ebf6c7e6f13a027fd35265d25e63c&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]
