codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3571738992
##########
superset/dashboards/api.py:
##########
@@ -1574,6 +1585,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.",
+ )
+
+ job_id = str(uuid.uuid4())
+ cache_manager.cache.set(inflight_key, job_id,
timeout=EXPORT_INFLIGHT_CACHE_TTL)
Review Comment:
**Suggestion:** The in-flight throttle is implemented with a separate read
then write, which is not atomic. Two concurrent requests from the same user can
both observe no lock and both enqueue exports, defeating the “one concurrent
export per user+dashboard” guarantee. Use an atomic cache add/set-if-not-exists
operation instead of `get` followed by `set`. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Duplicate Celery tasks for same dashboard Excel export.
- ⚠️ Extra S3 uploads and emails for concurrent submissions.
- ⚠️ Throttle comment misleads about guaranteed single in-flight job.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Excel export by setting
current_app.config["EXCEL_EXPORT_S3_BUCKET"] so
DashboardRestApi.export_xlsx in superset/dashboards/api.py:1588-1690 passes
the check at
line 1637.
2. Authenticate as a user with can_export permission and an email, and open
a dashboard
that has at least one chart so checks at lines 1658-1665 succeed.
3. Send two concurrent POST requests to /api/v1/dashboard/<pk>/export_xlsx/
for the same
dashboard and user, invoking DashboardRestApi.export_xlsx at
superset/dashboards/api.py:1588-1690 nearly simultaneously.
4. In each request inflight_key is computed identically at line 1669;
cache_manager.cache.get(inflight_key) at line 1670 returns falsy because no
lock exists
yet, so both requests execute lines 1676-1677, each setting a different
job_id and calling
export_dashboard_excel.apply_async, resulting in duplicate Celery tasks
despite the
intended one-export-per-user+dashboard throttle.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=340994676aff48ee8be0aabcf7be548e&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=340994676aff48ee8be0aabcf7be548e&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:** 1669:1677
**Comment:**
*Race Condition: The in-flight throttle is implemented with a separate
read then write, which is not atomic. Two concurrent requests from the same
user can both observe no lock and both enqueue exports, defeating the “one
concurrent export per user+dashboard” guarantee. Use an atomic cache
add/set-if-not-exists operation instead of `get` followed by `set`.
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=c57e58caf0ad6857102a8a9a8a995ea60187f9f4c6fe0086bc30e0215ecb53ec&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=c57e58caf0ad6857102a8a9a8a995ea60187f9f4c6fe0086bc30e0215ecb53ec&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3262,6 +3264,100 @@ def
test_get_all_related_viewers_with_extra_filters(self):
response = json.loads(rv.data.decode("utf-8"))
assert response["count"] > 0
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
Review Comment:
**Suggestion:** This test is not isolated from global config: it assumes
`EXCEL_EXPORT_S3_BUCKET` is unset, but if the test environment sets that config
(for example via env-based overrides), the endpoint won’t return 501 and the
assertion becomes flaky. Explicitly override `EXCEL_EXPORT_S3_BUCKET` to an
empty/None value for this test so it deterministically validates the 501 path.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Dashboard export_xlsx 501-path test becomes configuration-dependent.
⚠️ Harder to run tests against customized Excel export config.
⚠️ Test suite stability tied to external config overrides.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In tests/integration_tests/superset_test_config.py (lines 25-26 import
superset.config), add an override like `EXCEL_EXPORT_S3_BUCKET = "exports"`
so the test
config module sets a nonempty bucket value rather than relying on the
default None from
superset/config.py:1443.
2. Confirm that the integration test app uses this module by inspecting
tests/integration_tests/test_app.py:28-32, where `superset_config_module` is
resolved from
the SUPERSET_CONFIG environment variable and defaults to
`tests.integration_tests.superset_test_config`.
3. Note the export endpoint implementation in
superset/dashboards/api.py:1637-1640, where
`export_xlsx` checks `if not current_app.config["EXCEL_EXPORT_S3_BUCKET"]`
and returns a
501 response only when the bucket config is falsy.
4. Run the specific test `test_export_xlsx_501_when_bucket_unset` in
tests/integration_tests/dashboards/api_tests.py:68-78; it posts to
`api/v1/dashboard/<id>/export_xlsx/` and asserts `rv.status_code == 501`,
but with
EXCEL_EXPORT_S3_BUCKET now set, the endpoint no longer returns 501 and the
assertion
fails, demonstrating that the test outcome depends on ambient config instead
of explicitly
forcing the unset state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5bfbf4cf52f54b84b940d54e431691b1&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=5bfbf4cf52f54b84b940d54e431691b1&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:** 3267:3277
**Comment:**
*Logic Error: This test is not isolated from global config: it assumes
`EXCEL_EXPORT_S3_BUCKET` is unset, but if the test environment sets that config
(for example via env-based overrides), the endpoint won’t return 501 and the
assertion becomes flaky. Explicitly override `EXCEL_EXPORT_S3_BUCKET` to an
empty/None value for this test so it deterministically validates the 501 path.
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=3f7b297bfcee1582aaa8bf5760cdfa4d13c17a0d4a9df0dca21cbd6d8f8fc30b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3f7b297bfcee1582aaa8bf5760cdfa4d13c17a0d4a9df0dca21cbd6d8f8fc30b&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]
