codeant-ai-for-open-source[bot] commented on code in PR #38722:
URL: https://github.com/apache/superset/pull/38722#discussion_r2993898313
##########
superset/commands/report/execute.py:
##########
@@ -299,6 +299,19 @@ def get_dashboard_urls(
)
]
+ native_filter_params =
self._report_schedule.get_native_filters_params()
Review Comment:
**Suggestion:** The new branch misuses `get_native_filters_params()` by
treating its tuple return `(rison_string, warnings)` as a single value, which
both passes a tuple instead of the expected rison string into the URL and
silently drops any warning messages, leading to incorrect filter encoding and
missing warning logs; destructuring the return and extending `_filter_warnings`
fixes this. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard reports ignore native filter values when tabs disabled.
- ⚠️ Native filter warnings missing from report execution logs.
- ❌ Malformed native_filters URLs generated for dashboard permalinks.
```
</details>
```suggestion
native_filter_params, filter_warnings = (
self._report_schedule.get_native_filters_params()
)
if filter_warnings:
self._filter_warnings.extend(filter_warnings)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard report with native filters:
- Create a dashboard with a native filter and saved filter value.
- Create a ReportSchedule for that dashboard so that
`ReportSchedule.extra["dashboard"]["nativeFilters"]` is populated, which
`ReportSchedule.get_native_filters_params()` reads (see
`superset/reports/models.py:193-204`).
2. Ensure feature flags are set so the non-tabs branch is used:
- Enable alerts/reports and native filter support.
- Disable the `ALERT_REPORT_TABS` feature flag so the condition in
`BaseReportState.get_dashboard_urls` at
`superset/commands/report/execute.py:267-273`
is false and execution falls through to the new branch at
`execute.py:302-313`.
3. Trigger execution of the dashboard report:
- A Celery worker runs `AsyncExecuteReportScheduleCommand.run` at
`superset/commands/report/execute.py:1080-1096`, which drives the
`ReportScheduleStateMachine` and eventually calls
`BaseReportState._get_notification_content` (`execute.py:610-618`).
- For PNG/PDF dashboard reports, `_get_notification_content` calls
`self._get_screenshots()` at `execute.py:629`.
- In `_get_screenshots` (`execute.py:373-407`), because this is a
dashboard report
(`self._report_schedule.chart` is falsy), the `else` branch at
`execute.py:402-407`
executes `urls = self.get_dashboard_urls()`.
4. Observe the misuse of `get_native_filters_params` in the non-tabs branch:
- Inside `get_dashboard_urls` (`execute.py:258-79`), with
`ALERT_REPORT_TABS` disabled,
execution reaches the new block at `execute.py:302-313`:
`native_filter_params =
self._report_schedule.get_native_filters_params()`.
- `ReportSchedule.get_native_filters_params` is defined to return a tuple
`(rison_encoded_params, list_of_warning_messages)` at
`superset/reports/models.py:193-201`, so `native_filter_params` here is a
tuple, not
the rison string.
- That tuple is passed directly into the permalink state as
`["native_filters",
native_filter_params]` and then into `_get_tab_url`, which calls
`CreateDashboardPermalinkCommand.run` and ultimately leads to
`Superset.dashboard_permalink` in `superset/views/core.py:13-24`.
- In `dashboard_permalink`, the URL is assembled with `url =
f"{url}&native_filters={param_val}"` at `superset/views/core.py:19-21`,
so `param_val`
being a tuple produces a URL like `&native_filters=('(...rison...)',
['warning'])`
instead of a pure rison string. The dashboard front-end cannot parse this
value as a
valid native filter config, so the filter is not applied when the
screenshot URL is
loaded.
5. Observe that filter warnings are silently discarded on this path:
- In the tabs-enabled branch, `native_filter_params, filter_warnings =
self._report_schedule.get_native_filters_params()` and
`self._filter_warnings.extend(filter_warnings)` are executed at
`execute.py:269-273`,
so later `ReportNotTriggeredErrorState.next` and
`ReportSuccessState.next` include
these warnings in the log via the `"Include filter warnings in the log"`
block at
`execute.py:854-861` and `execute.py:1005-1011`.
- In the non-tabs branch, the current code at `execute.py:302-313` never
unpacks the
`(params, warnings)` tuple, so `self._filter_warnings` remains empty even
if
`get_native_filters_params` emitted warnings. As a result, report
execution logs never
show native filter warnings when `ALERT_REPORT_TABS` is disabled.
```
</details>
<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:** 302:302
**Comment:**
*Logic Error: The new branch misuses `get_native_filters_params()` by
treating its tuple return `(rison_string, warnings)` as a single value, which
both passes a tuple instead of the expected rison string into the URL and
silently drops any warning messages, leading to incorrect filter encoding and
missing warning logs; destructuring the return and extending `_filter_warnings`
fixes this.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38722&comment_hash=223c81a3d0cf763423ef3fd69cfdedf6a85202e4fe54c972d3c98717f6107aa1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38722&comment_hash=223c81a3d0cf763423ef3fd69cfdedf6a85202e4fe54c972d3c98717f6107aa1&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]
