codeant-ai-for-open-source[bot] commented on code in PR #44082:
URL: https://github.com/apache/superset/pull/44082#discussion_r3986755453
##########
superset/dashboards/api.py:
##########
@@ -1825,25 +1848,131 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
)
job_id = str(uuid.uuid4())
+ if queued:
+ return self._export_xlsx_queued(
+ dashboard, active_data_mask, mode, job_id, lock_params
+ )
+
+ # Plan after locking because query-context resolution can be expensive.
+ # Release here unless the inline exporter takes over cleanup.
+ lock_delegated = False
+ try:
+ plan: InlineExportPlan = plan_inline_export(dashboard)
+ if not plan.fits_row_budget:
+ return self.response_400(
+ message=(
+ "This dashboard requests too many rows to export in a "
+ "single request. Configure EXCEL_EXPORT_S3_BUCKET to "
+ "export it in the background, or lower the row limits
of "
+ "its charts."
+ )
+ )
+ lock_delegated = True
+ return self._export_xlsx_inline(
+ dashboard,
+ active_data_mask,
+ job_id,
+ lock_params,
+ plan.query_contexts,
+ )
+ finally:
+ if not lock_delegated:
+ try:
+ ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE,
lock_params).run()
+ except Exception: # pylint: disable=broad-except
+ # The TTL is the fallback if release fails.
+ logger.exception(
+ "Failed to release in-flight export lock for dashboard
%s",
+ dashboard.id,
+ )
+
+ def _export_xlsx_queued( # pylint: disable=too-many-arguments
+ self,
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ mode: str,
+ job_id: str,
+ lock_params: dict[str, int],
+ ) -> WerkzeugResponse:
+ """Queue an export for upload and email delivery."""
try:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
"user_id": g.user.id,
- "active_data_mask": payload.get("active_data_mask", {}),
+ "active_data_mask": active_data_mask,
"job_id": job_id,
- "mode": payload.get("mode", "data"),
+ "mode": mode,
},
task_id=job_id,
)
except Exception:
- # If enqueuing fails (e.g. broker down) the task will never run to
- # release the lock, so free it now rather than block exports until
- # the TTL expires.
+ # No task will release the lock if enqueueing fails.
ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE, lock_params).run()
raise
return self.response(202, job_id=job_id)
+ @staticmethod
+ def _export_xlsx_inline( # pylint: disable=too-many-arguments
+ dashboard: Dashboard,
+ active_data_mask: dict[str, Any],
+ job_id: str,
+ lock_params: dict[str, int],
+ query_contexts: ResolvedQueryContexts,
+ ) -> WerkzeugResponse:
+ """Build a planned data export and return it in the response."""
+ tmp_path: str | None = None
+ try:
+ file_descriptor, tmp_path = tempfile.mkstemp(
+ suffix=".xlsx", prefix=f"dash-export-{job_id}-"
+ )
+ os.close(file_descriptor)
+
+ build_workbook(
+ tmp_path,
+ dashboard,
+ active_data_mask,
+ job_id,
+ EXPORT_MODE_DATA,
+ g.user,
+ query_contexts=query_contexts,
+ )
+ filename = get_filename(
+ dashboard.dashboard_title, dashboard.id, skip_id=False
+ )
+ response = send_file(
+ tmp_path,
+ mimetype=(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
+ ),
+ as_attachment=True,
+ download_name=f"{filename}.xlsx",
+ conditional=False,
+ max_age=0,
+ )
+ except Exception:
+ if tmp_path and os.path.exists(tmp_path):
+ os.remove(tmp_path)
+ raise
+ finally:
+ try:
+ ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE,
lock_params).run()
Review Comment:
**Suggestion:** The lock is released before the response finishes streaming,
allowing another export to start while this download and temporary-file cleanup
remain active. [stale reference]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=73f7a6b0103f4c628439201ee4df6e40&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=73f7a6b0103f4c628439201ee4df6e40&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 1959:1959
**Comment:**
*Stale Reference: The lock is released before the response finishes
streaming, allowing another export to start while this download and
temporary-file cleanup remain active.
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%2F44082&comment_hash=40a14a11efb059e81f6e69d3b1b567765847676b4ba02de53d43851ccad8abe2&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44082&comment_hash=40a14a11efb059e81f6e69d3b1b567765847676b4ba02de53d43851ccad8abe2&reaction=dislike'>๐</a>
##########
superset/dashboards/api.py:
##########
@@ -1797,20 +1811,29 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
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.
+ # Both delivery paths require a non-guest account with an email
address.
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.
+ active_data_mask = payload.get("active_data_mask", {})
+ mode = payload.get("mode", "data")
+
+ if not queued and mode == EXPORT_MODE_IMAGES:
+ # Webdriver rendering is too slow and unbounded for a web request.
+ return self.response_400(
+ message=(
+ "Exporting images to Excel runs in the background. "
+ "Configure EXCEL_EXPORT_S3_BUCKET to use it, or export "
+ "the dashboard's data instead."
+ )
+ )
+
+ # Allow one export per user and dashboard across web and worker
processes.
+ # The TTL releases the lock if normal cleanup fails.
lock_params = export_lock_params(g.user.id, dashboard.id)
try:
AcquireDistributedLock(
Review Comment:
**Suggestion:** The acquisition token is discarded, so later unconditional
releases can delete a newer request's lock after this export exceeds its TTL.
[race condition]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6082c4b6e784469fbbccfa83081da5c1&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=6082c4b6e784469fbbccfa83081da5c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 1839:1843
**Comment:**
*Race Condition: The acquisition token is discarded, so later
unconditional releases can delete a newer request's lock after this export
exceeds its TTL.
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%2F44082&comment_hash=8f59f223d8471b94818ec51fc84420cd06007d17ed6755712915352d9fb570f0&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44082&comment_hash=8f59f223d8471b94818ec51fc84420cd06007d17ed6755712915352d9fb570f0&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]
