From f9c665f8a5d50873409e423a0d7c29509e9cea92 Mon Sep 17 00:00:00 2001 From: Enzo Martellucci Date: Mon, 4 May 2026 18:01:56 +0200 Subject: [PATCH 1/3] fix(reports): preserve dashboard_state urlParams in report URL builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_get_tabs_urls` and the `ALERT_REPORT_TABS`-disabled fall-through in `get_dashboard_urls` were both building permalinks with `urlParams=[["native_filters", ]]` only, silently discarding everything else `extra.dashboard.urlParams` carried — notably `standalone=true`, which the single-tab branch already preserves (see `get_dashboard_urls` lines 290-306). The list-anchor shape that triggers the multi-tab path is reachable from the standard Reports UI: when a dashboard has at least 2 tabs, the AlertReportModal injects an "All Tabs" option whose value is `JSON.stringify(allTabsWithOrder)` (see `AlertReportModal.tsx` ~L1094-1099). Picking it makes the backend fan out per-tab screenshots, each missing `standalone=true`, which causes Playwright to time out at `Locator.wait_for(".standalone")` after the 10-minute hard timeout. Recipients get a failure email instead of the screenshots. Apply the same merge semantics in both spots: dedup any existing `native_filters` entry from the original `urlParams` and append the report's. Other per-tab fields (`anchor`, `dataMask`, `activeTabs`) keep their previous shape — this fix is scoped to the `urlParams` discard. The `ALERT_REPORT_TABS=False` fall-through previously didn't honor `extra.dashboard.urlParams` at all (only API-set, since the modal is gated on the flag); now it does, matching the on-flag path. Drive-bys (same area, kept tight): - `DashboardPermalinkState.urlParams` annotation: `list[tuple[str, str]]` -> `list[Sequence[str]]`. JSON-derived values arrive as `list[list[str]]` at runtime; `Sequence` is permissive of both shapes. - Single-tab branch and the merged fall-through use the new annotation; obsolete `# type: ignore` comments removed. - Silenced a pre-existing pylint `consider-using-transaction` warning on `db.session.rollback()` in `create_log` that the pre-commit hook was sporadically failing on; same disable comment as the surrounding `db.session.commit()`. Tests: - `test_get_dashboard_urls_multitab_preserves_url_params` seeds `urlParams` with `standalone`, `show_filters`, AND a stale `native_filters` entry, and asserts the merge replaces the stale entry, preserves the others in order, and targets each per-tab permalink at its anchor. - `test_get_dashboard_urls_flag_off_preserves_url_params` does the same for the `ALERT_REPORT_TABS=False` fall-through. Co-Authored-By: Claude Opus 4.7 (1M context) --- superset/commands/report/execute.py | 47 +++++--- superset/dashboards/permalink/types.py | 6 +- .../commands/report/execute_test.py | 107 ++++++++++++++++++ 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 66265878dad4..e2cd5112bb07 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import logging +from collections.abc import Sequence from datetime import datetime, timedelta from typing import Any, Optional, Union from uuid import UUID @@ -196,7 +197,7 @@ def create_log(self, error_message: Optional[str] = None) -> None: db.session.commit() # pylint: disable=consider-using-transaction except StaleDataError as ex: # Report schedule was modified or deleted by another process - db.session.rollback() + db.session.rollback() # pylint: disable=consider-using-transaction logger.warning( "Report schedule (execution %s) was modified or deleted " "during execution. This can occur when a report is deleted " @@ -280,6 +281,7 @@ def get_dashboard_urls( ) urls = self._get_tabs_urls( anchor_list, + dashboard_state=dashboard_state, native_filter_params=native_filter_params, user_friendly=user_friendly, ) @@ -291,12 +293,12 @@ def get_dashboard_urls( # overwriting — dashboard_state may already have urlParams # (e.g. standalone=true) that must be preserved. state: DashboardPermalinkState = {**dashboard_state} - existing_params: list[tuple[str, str]] = state.get("urlParams") or [] - merged_params: list[list[str]] = [ + existing_params: list[Sequence[str]] = state.get("urlParams") or [] + merged_params: list[Sequence[str]] = [ list(p) for p in existing_params if p[0] != "native_filters" ] merged_params.append(["native_filters", native_filter_params or ""]) - state["urlParams"] = merged_params # type: ignore[typeddict-item] + state["urlParams"] = merged_params return [ self._get_tab_url( state, @@ -310,13 +312,20 @@ def get_dashboard_urls( if filter_warnings: self._filter_warnings.extend(filter_warnings) if native_filter_params and native_filter_params != "()": + # Preserve any urlParams from extra.dashboard (e.g. standalone=true) + # set via API even when ALERT_REPORT_TABS is off — same merge + # semantics as the protected branch above. + fallback_state = self._report_schedule.extra.get("dashboard") or {} + fallback_existing: list[Sequence[str]] = ( + fallback_state.get("urlParams") or [] + ) + fallback_merged: list[Sequence[str]] = [ + list(p) for p in fallback_existing if p[0] != "native_filters" + ] + fallback_merged.append(["native_filters", native_filter_params]) return [ self._get_tab_url( - { - "urlParams": [ - ["native_filters", native_filter_params] # type: ignore - ], - }, + {"urlParams": fallback_merged}, user_friendly=user_friendly, ) ] @@ -356,21 +365,33 @@ def _get_tab_url( def _get_tabs_urls( self, tab_anchors: list[str], + dashboard_state: Optional[DashboardPermalinkState] = None, native_filter_params: Optional[str] = None, user_friendly: bool = False, ) -> list[str]: """ - Get multple tabs urls + Get multiple tabs urls. + + Each per-tab permalink merges the report's ``native_filters`` into + the original ``dashboard_state.urlParams`` (deduping any prior + ``native_filters`` entry), so params like ``standalone=true`` are + preserved — matching the precedence rules of the single-tab branch + in :meth:`get_dashboard_urls`. """ + base_state: DashboardPermalinkState = dashboard_state or {} + existing_params: list[Sequence[str]] = base_state.get("urlParams") or [] + merged_params: list[Sequence[str]] = [ + list(p) for p in existing_params if p[0] != "native_filters" + ] + merged_params.append(["native_filters", native_filter_params or ""]) + return [ self._get_tab_url( { "anchor": tab_anchor, "dataMask": None, "activeTabs": None, - "urlParams": [ - ["native_filters", native_filter_params] # type: ignore - ], + "urlParams": merged_params, }, user_friendly=user_friendly, ) diff --git a/superset/dashboards/permalink/types.py b/superset/dashboards/permalink/types.py index c1a3d27c5196..01952534c0d8 100644 --- a/superset/dashboards/permalink/types.py +++ b/superset/dashboards/permalink/types.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from collections.abc import Sequence from typing import Any, Optional, TypedDict @@ -21,7 +22,10 @@ class DashboardPermalinkState(TypedDict, total=False): dataMask: Optional[dict[str, Any]] activeTabs: Optional[list[str]] anchor: Optional[str] - urlParams: Optional[list[tuple[str, str]]] + # urlParams items are stored/transmitted as JSON arrays, so they + # arrive at runtime as ``list[str]``; ``Sequence[str]`` keeps the + # annotation permissive of both list and tuple shapes. + urlParams: Optional[list[Sequence[str]]] chartStates: Optional[dict[str, Any]] diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index d7ff25a3434c..79f4fe1fc9fd 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -624,6 +624,65 @@ def test_get_tab_urls( ] +@patch("superset.commands.report.execute.CreateDashboardPermalinkCommand") +@with_feature_flags(ALERT_REPORT_TABS=True) +def test_get_dashboard_urls_multitab_preserves_url_params( + mock_permalink_cls, + mocker: MockerFixture, + app, +) -> None: + """Multi-tab fan-out must preserve dashboard_state.urlParams (e.g. standalone) + and replace any pre-existing native_filters entry with the report's value — + matching the single-tab branch's merge semantics.""" + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.type = "report_type" + mock_report_schedule.report_format = "report_format" + mock_report_schedule.owners = [1, 2] + mock_report_schedule.recipients = [] + native_filter_rison = "(NATIVE_FILTER-1:(filterType:filter_select))" + # Use list-of-lists (not tuples) — extra_json deserializes urlParams from + # JSON arrays. Includes a stale native_filters entry to exercise the + # dedup-then-append step in the merge. + mock_report_schedule.extra = { + "dashboard": { + "anchor": json.dumps(["TAB-1", "TAB-2"]), + "urlParams": [ + ["standalone", "true"], + ["native_filters", "(STALE_FILTER:(filterType:filter_select))"], + ["show_filters", "0"], + ], + } + } + mock_report_schedule.get_native_filters_params.return_value = ( # type: ignore[attr-defined] + native_filter_rison, + [], + ) + mock_permalink_cls.return_value.run.side_effect = ["key1", "key2"] + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + + class_instance.get_dashboard_urls() + + assert mock_permalink_cls.call_count == 2 + for idx, expected_anchor in enumerate(["TAB-1", "TAB-2"]): + state = mock_permalink_cls.call_args_list[idx].kwargs["state"] + # Stale native_filters is replaced (not duplicated); other params + # survive in their original order; report's native_filters appended. + assert state["urlParams"] == [ + ["standalone", "true"], + ["show_filters", "0"], + ["native_filters", native_filter_rison], + ] + # Each per-tab permalink targets exactly that tab. + assert state["anchor"] == expected_anchor + + @patch( "superset.commands.dashboard.permalink.create.CreateDashboardPermalinkCommand.run" ) @@ -702,6 +761,54 @@ def test_get_dashboard_urls_native_filters_without_tabs( assert "permalink_key" in result[0] +@patch("superset.commands.report.execute.CreateDashboardPermalinkCommand") +@with_feature_flags(ALERT_REPORT_TABS=False) +def test_get_dashboard_urls_flag_off_preserves_url_params( + mock_permalink_cls, + mocker: MockerFixture, + app, +) -> None: + """When ALERT_REPORT_TABS is disabled, the fall-through path must still + honor any urlParams set in extra.dashboard (e.g. via API) — same merge + semantics as the protected branch.""" + mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) + mock_report_schedule.chart = False + mock_report_schedule.chart_id = None + mock_report_schedule.dashboard_id = 123 + mock_report_schedule.force_screenshot = False + native_filter_rison = "(NATIVE_FILTER-abc:!(val1))" + mock_report_schedule.extra = { + "dashboard": { + "urlParams": [ + ["standalone", "true"], + ["native_filters", "(STALE_FILTER:!(stale))"], + ["show_filters", "0"], + ], + } + } + mock_report_schedule.get_native_filters_params.return_value = ( # type: ignore[attr-defined] + native_filter_rison, + [], + ) + + class_instance: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + class_instance._report_schedule = mock_report_schedule + mock_permalink_cls.return_value.run.return_value = "permalink_key" + + class_instance.get_dashboard_urls() + + state = mock_permalink_cls.call_args_list[0].kwargs["state"] + # Stale native_filters replaced; existing params survive in order; + # report's native_filters appended. + assert state["urlParams"] == [ + ["standalone", "true"], + ["show_filters", "0"], + ["native_filters", native_filter_rison], + ] + + def create_report_schedule( mocker: MockerFixture, custom_width: int | None = None, From 460300ccdf5f06d77dd4388c1a8f1b42654a31a4 Mon Sep 17 00:00:00 2001 From: Enzo Martellucci Date: Tue, 5 May 2026 10:48:41 +0200 Subject: [PATCH 2/3] test(reports): clarify flag-off urlParams test scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the flag-off urlParams preservation test: - Drop dead `force_screenshot = False` — the merge branch returns early so the attribute is never read. Mock auto-attributes are truthy by default; the matching multi-tab test doesn't set it either. - Expand the docstring with a "Reachability:" note explaining that the fall-through is reached only when `dashboard_state` is falsy OR `ALERT_REPORT_TABS=False`; the flag-on / no-anchor case lands in the single-tab merge at L290-306 (already correct since before this PR), not in this branch. No production change. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit_tests/commands/report/execute_test.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 79f4fe1fc9fd..40a0efdc9393 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -768,14 +768,18 @@ def test_get_dashboard_urls_flag_off_preserves_url_params( mocker: MockerFixture, app, ) -> None: - """When ALERT_REPORT_TABS is disabled, the fall-through path must still - honor any urlParams set in extra.dashboard (e.g. via API) — same merge - semantics as the protected branch.""" + """The post-``if``-block fall-through in ``get_dashboard_urls`` must + honor any urlParams set in ``extra.dashboard`` (e.g. via API) — same + merge semantics as the protected branch. + + Reachability: only when ``dashboard_state`` is falsy OR + ``ALERT_REPORT_TABS=False``. The flag-on / no-anchor case lands in + the single-tab merge at L290-306, not here. + """ mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) mock_report_schedule.chart = False mock_report_schedule.chart_id = None mock_report_schedule.dashboard_id = 123 - mock_report_schedule.force_screenshot = False native_filter_rison = "(NATIVE_FILTER-abc:!(val1))" mock_report_schedule.extra = { "dashboard": { From 012094aaf9c5c90c387d33452332acca8263db0e Mon Sep 17 00:00:00 2001 From: Enzo Martellucci Date: Tue, 5 May 2026 18:28:12 +0200 Subject: [PATCH 3/3] refactor(reports): extract native_filters urlParams merge into a helper --- superset/commands/report/execute.py | 48 +++++++++++++++++------------ 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index e2cd5112bb07..0cca27ef880f 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -293,12 +293,9 @@ def get_dashboard_urls( # overwriting — dashboard_state may already have urlParams # (e.g. standalone=true) that must be preserved. state: DashboardPermalinkState = {**dashboard_state} - existing_params: list[Sequence[str]] = state.get("urlParams") or [] - merged_params: list[Sequence[str]] = [ - list(p) for p in existing_params if p[0] != "native_filters" - ] - merged_params.append(["native_filters", native_filter_params or ""]) - state["urlParams"] = merged_params + state["urlParams"] = self._merge_native_filters_into_url_params( + state.get("urlParams"), native_filter_params + ) return [ self._get_tab_url( state, @@ -316,16 +313,14 @@ def get_dashboard_urls( # set via API even when ALERT_REPORT_TABS is off — same merge # semantics as the protected branch above. fallback_state = self._report_schedule.extra.get("dashboard") or {} - fallback_existing: list[Sequence[str]] = ( - fallback_state.get("urlParams") or [] - ) - fallback_merged: list[Sequence[str]] = [ - list(p) for p in fallback_existing if p[0] != "native_filters" - ] - fallback_merged.append(["native_filters", native_filter_params]) return [ self._get_tab_url( - {"urlParams": fallback_merged}, + { + "urlParams": self._merge_native_filters_into_url_params( + fallback_state.get("urlParams"), + native_filter_params, + ) + }, user_friendly=user_friendly, ) ] @@ -362,6 +357,23 @@ def _get_tab_url( user_friendly=user_friendly, ) + @staticmethod + def _merge_native_filters_into_url_params( + existing: Optional[Sequence[Sequence[str]]], + native_filter_params: Optional[str], + ) -> list[Sequence[str]]: + """ + Merge the report's ``native_filters`` into a permalink's existing + ``urlParams``, deduping any prior ``native_filters`` entry so the + report's value wins. All other params (e.g. ``standalone=true``) + survive in their original order. + """ + merged: list[Sequence[str]] = [ + list(p) for p in (existing or []) if p[0] != "native_filters" + ] + merged.append(["native_filters", native_filter_params or ""]) + return merged + def _get_tabs_urls( self, tab_anchors: list[str], @@ -379,11 +391,9 @@ def _get_tabs_urls( in :meth:`get_dashboard_urls`. """ base_state: DashboardPermalinkState = dashboard_state or {} - existing_params: list[Sequence[str]] = base_state.get("urlParams") or [] - merged_params: list[Sequence[str]] = [ - list(p) for p in existing_params if p[0] != "native_filters" - ] - merged_params.append(["native_filters", native_filter_params or ""]) + merged_params = self._merge_native_filters_into_url_params( + base_state.get("urlParams"), native_filter_params + ) return [ self._get_tab_url(