codeant-ai-for-open-source[bot] commented on code in PR #41424:
URL: https://github.com/apache/superset/pull/41424#discussion_r3475109463
##########
superset/commands/report/execute.py:
##########
@@ -510,9 +513,32 @@ def _get_pdf(self) -> bytes:
return pdf
- def _get_csv_data(self) -> bytes:
+ def _get_data(self, result_format: ChartDataResultFormat) -> bytes:
+ """
+ Fetch tabular chart data (CSV or Excel) as raw bytes.
+
+ Both formats are produced by the chart data export endpoint, so the
+ bytes are fetched the same way and only differ by ``result_format``.
+ This reuses the export path's post-processing and index handling,
+ keeping report output consistent with a chart's manual export.
+ """
+ timeout_error: type[CommandException]
+ failed_error: type[CommandException]
+ if result_format == ChartDataResultFormat.XLSX:
+ label, timeout_error, failed_error = (
+ "Excel",
+ ReportScheduleXlsxTimeout,
+ ReportScheduleXlsxFailedError,
+ )
Review Comment:
**Suggestion:** The XLSX-specific error mapping is incomplete: when query
context is missing, `_update_query_context()` can raise
`ReportScheduleCsvFailedError` before the XLSX timeout/failure classes are
used, so Excel report failures are misclassified as CSV failures.
Wrap/translate that prefetch error path to `ReportScheduleXlsxFailedError` when
`result_format` is XLSX. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Excel report errors surfaced as CSV failure type.
- ⚠️ Error messages mention CSV despite Excel selection.
- ⚠️ Monitoring/metrics misattribute Excel failures to CSV.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart-based report schedule with `report_format ==
ReportDataFormat.XLSX`
so that `_get_notification_content()` in `BaseReportState`
(`superset/commands/report/execute.py:689-766`) will call
`_get_data(ChartDataResultFormat.XLSX)` at lines `720-723`.
2. Ensure the chart has no saved `query_context`
(`self._report_schedule.chart.query_context is None`), so `_get_data()` at
`execute.py:549-552` logs a warning and calls `_update_query_context()`
before attempting
to fetch data.
3. Induce a screenshot failure (for example, a WebDriver issue) so that
`_get_screenshots()` (`execute.py:422-504`) raises
`ReportScheduleScreenshotFailedError`
or `ReportScheduleScreenshotTimeout`; `_update_query_context()` at
`execute.py:636-655`
catches these and re-raises `ReportScheduleCsvFailedError` with a message
about missing
query context.
4. Because this `ReportScheduleCsvFailedError` is raised before the
`try/except` block in
`_get_data()` (lines `553-581`) and `_get_data()` only maps later failures
to the
XLSX-specific `ReportScheduleXlsxFailedError` / `ReportScheduleXlsxTimeout`,
the XLSX run
propagates a CSV-specific error (with message defined in
`superset/commands/report/exceptions.py:16-21`), misclassifying Excel
failures as CSV
failures.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7168a6bcf5ee4036a46dbe481593a97b&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=7168a6bcf5ee4036a46dbe481593a97b&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/commands/report/execute.py
**Line:** 527:532
**Comment:**
*Logic Error: The XLSX-specific error mapping is incomplete: when query
context is missing, `_update_query_context()` can raise
`ReportScheduleCsvFailedError` before the XLSX timeout/failure classes are
used, so Excel report failures are misclassified as CSV failures.
Wrap/translate that prefetch error path to `ReportScheduleXlsxFailedError` when
`result_format` is XLSX.
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%2F41424&comment_hash=069cb69d4ad6e0f8af89d0a507fb689253e3bab08addf8d058333e1a6441d818&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=069cb69d4ad6e0f8af89d0a507fb689253e3bab08addf8d058333e1a6441d818&reaction=dislike'>👎</a>
##########
superset/commands/report/execute.py:
##########
@@ -687,11 +715,16 @@ def _get_notification_content(self) ->
NotificationContent: # noqa: C901
error_text = "Unexpected missing pdf"
elif (
self._report_schedule.chart
- and self._report_schedule.report_format == ReportDataFormat.CSV
+ and self._report_schedule.report_format in
ReportDataFormat.tabular()
):
- csv_data = self._get_csv_data()
- if not csv_data:
- error_text = "Unexpected missing csv file"
+ if self._report_schedule.report_format ==
ReportDataFormat.XLSX:
+ xlsx_data = self._get_data(ChartDataResultFormat.XLSX)
+ if not xlsx_data:
+ error_text = "Unexpected missing Excel file"
Review Comment:
**Suggestion:** XLSX generation is now enabled for all tabular report
schedules, but webhook notifications still only serialize CSV/PDF/PNG
attachments. For webhook recipients, `xlsx` bytes are produced here and then
silently dropped downstream, so users receive a successful notification without
the requested file. Either gate XLSX to recipient types that support it or add
XLSX handling in webhook file packaging. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Webhook reports with Excel format lack XLSX attachment.
- ⚠️ Webhook consumers cannot access requested tabular report.
- ⚠️ Users see successful run but missing Excel content.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a chart-based report schedule with format `ReportDataFormat.XLSX`
and at least
one webhook recipient (type `ReportRecipientType.WEBHOOK`) in
`ReportSchedule.recipients`
(used by `BaseReportState._send` at
`superset/commands/report/execute.py:768-784`).
2. When the scheduler runs `AsyncExecuteReportScheduleCommand.run()`
(`superset/commands/report/execute.py:1198-1217`), it builds a
`BaseReportState` and calls
`BaseReportState.send()`, which in turn calls `_get_notification_content()`
at
`superset/commands/report/execute.py:689-766`.
3. In `_get_notification_content()`, the XLSX branch at lines `716-723`
executes: because
`self._report_schedule.chart` is set and `report_format` is in
`ReportDataFormat.tabular()`, `self._get_data(ChartDataResultFormat.XLSX)`
is called and
the returned bytes are stored in `xlsx_data`, which is then passed into
`NotificationContent(..., xlsx=xlsx_data, ...)` at lines `756-763`.
4. For the webhook recipient, `create_notification()`
(`superset/reports/notifications/__init__.py:18-30`) instantiates
`WebhookNotification`,
whose `_get_files()` method at
`superset/reports/notifications/webhook.py:79-95` only
attaches `content.csv`, `content.pdf`, and `content.screenshots`, ignoring
`content.xlsx`,
so the outgoing `requests.post()` in `WebhookNotification.send()`
(`webhook.py:134-164`)
never includes the Excel file even though the report format was XLSX and the
execution
succeeds.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5076a0776fae4f25b2552dd0116d7fcf&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=5076a0776fae4f25b2552dd0116d7fcf&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/commands/report/execute.py
**Line:** 716:723
**Comment:**
*Incomplete Implementation: XLSX generation is now enabled for all
tabular report schedules, but webhook notifications still only serialize
CSV/PDF/PNG attachments. For webhook recipients, `xlsx` bytes are produced here
and then silently dropped downstream, so users receive a successful
notification without the requested file. Either gate XLSX to recipient types
that support it or add XLSX handling in webhook file packaging.
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%2F41424&comment_hash=d0209730c4dd03c247476f0e6e63b938d4aa0abcec89bd2fd879064c4de125bc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41424&comment_hash=d0209730c4dd03c247476f0e6e63b938d4aa0abcec89bd2fd879064c4de125bc&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]
