codeant-ai-for-open-source[bot] commented on code in PR #39973:
URL: https://github.com/apache/superset/pull/39973#discussion_r3573720250
##########
superset/commands/report/execute.py:
##########
@@ -274,23 +277,30 @@ def _get_url(
# Every content path (_get_screenshots, _get_csv_data,
# _get_embedded_data, _get_notification_content) funnels through this
# method, so this is the single choke point.
- if (
- self._report_schedule.chart_id is not None
- and self._report_schedule.chart is None
- ):
+ if self._report_schedule.chart_id is not None and chart is None:
raise ReportScheduleTargetChartDeletedError()
# Symmetric guard for dashboard targets. Dashboard soft delete lands
# in the sibling rollout; until then this cannot fire (a dashboard
# with dependent reports cannot be deleted), which makes it inert
# rather than wrong — and it keeps the report-target error vocabulary
# parallel across entities from day one.
- if (
- self._report_schedule.dashboard_id is not None
- and self._report_schedule.dashboard is None
- ):
+ if self._report_schedule.dashboard_id is not None and dashboard is
None:
raise ReportScheduleTargetDashboardDeletedError()
Review Comment:
**Suggestion:** The deleted-dashboard guard is evaluated unconditionally
before target resolution, so a report that still has a valid chart target can
now fail if a stale `dashboard_id` exists and its relationship loads as `None`.
Restrict this guard to dashboard-targeted schedules (or run it only in the
dashboard branch) so chart reports are not incorrectly blocked by an unrelated
dashboard relationship. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Chart reports fail when stale dashboard relationship exists.
- ⚠️ Report delivery interrupted by misconfigured dashboard metadata.
- ⚠️ Future dashboard soft-delete could trigger spurious failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `tests/unit_tests/commands/report/execute_test.py` (see existing
patterns at lines
25–72 and 95–120 from the current file), create a new test that builds a
`ReportSchedule`
mock (spec=ReportSchedule) with `chart` truthy and `chart_id` set, but with
a stale
non-target dashboard: `report_schedule.chart = True`,
`report_schedule.chart_id = 42`,
`report_schedule.dashboard_id = 7`, and `report_schedule.dashboard = None`.
2. Instantiate `BaseReportState` as in other tests (e.g., lines 47–71 and
109–112 in
`execute_test.py`), passing the mocked `report_schedule` into the
constructor: `state =
BaseReportState(report_schedule, datetime.utcnow(), uuid4())`.
3. Call `state._get_url()`; this executes `_get_url` in
`superset/commands/report/execute.py` (lines 28–108 of the excerpt), which
first assigns
`chart = self._report_schedule.chart` and `dashboard =
self._report_schedule.dashboard`,
then evaluates the guards at lines 51–59 (diff lines 280–288).
4. The chart guard at line 51 (`if self._report_schedule.chart_id is not
None and chart is
None`) does not fire because `chart` is truthy, but the dashboard guard at
line 58 / diff
line 287 (`if self._report_schedule.dashboard_id is not None and dashboard
is None`)
evaluates to True and raises `ReportScheduleTargetDashboardDeletedError`,
aborting URL
resolution even though the actual chart target is valid and the later chart
branch at
lines 73–92 would have successfully produced a URL.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=259f5d4622c743d6af08e58625d7f4c2&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=259f5d4622c743d6af08e58625d7f4c2&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:** 287:288
**Comment:**
*Incorrect Condition Logic: The deleted-dashboard guard is evaluated
unconditionally before target resolution, so a report that still has a valid
chart target can now fail if a stale `dashboard_id` exists and its relationship
loads as `None`. Restrict this guard to dashboard-targeted schedules (or run it
only in the dashboard branch) so chart reports are not incorrectly blocked by
an unrelated dashboard relationship.
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%2F39973&comment_hash=3e240bc1d0b838918a3d52bf9c883e236282105e4e782ff5e0de1aa8057d10a0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39973&comment_hash=3e240bc1d0b838918a3d52bf9c883e236282105e4e782ff5e0de1aa8057d10a0&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]
