diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 66265878dad4..0cca27ef880f 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,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[tuple[str, str]] = state.get("urlParams") or [] - merged_params: list[list[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"] = self._merge_native_filters_into_url_params( + state.get("urlParams"), native_filter_params + ) return [ self._get_tab_url( state, @@ -310,12 +309,17 @@ 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 {} return [ self._get_tab_url( { - "urlParams": [ - ["native_filters", native_filter_params] # type: ignore - ], + "urlParams": self._merge_native_filters_into_url_params( + fallback_state.get("urlParams"), + native_filter_params, + ) }, user_friendly=user_friendly, ) @@ -353,24 +357,51 @@ 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], + 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 {} + merged_params = self._merge_native_filters_into_url_params( + base_state.get("urlParams"), native_filter_params + ) + 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..40a0efdc9393 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,58 @@ 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: + """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 + 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,