diff --git a/superset/charts/data/api.py b/superset/charts/data/api.py index 3cbeccde271..4f6bc10ec4e 100644 --- a/superset/charts/data/api.py +++ b/superset/charts/data/api.py @@ -32,6 +32,7 @@ from superset.async_events.async_query_manager import AsyncQueryTokenException from superset.charts.api import ChartRestApi from superset.charts.client_processing import apply_client_processing from superset.charts.data.dashboard_filter_context import ( + apply_dashboard_filter_context, DashboardFilterContext, get_dashboard_filter_context, ) @@ -50,11 +51,7 @@ from superset.commands.chart.exceptions import ( ) from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType from superset.connectors.sqla.models import BaseDatasource -from superset.constants import ( - CACHE_DISABLED_TIMEOUT, - EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS, - EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS, -) +from superset.constants import CACHE_DISABLED_TIMEOUT from superset.daos.exceptions import DatasourceNotFound from superset.exceptions import QueryObjectValidationError, SupersetSecurityException from superset.extensions import event_logger @@ -204,41 +201,16 @@ class ChartDataRestApi(ChartRestApi): except SupersetSecurityException: return self.response_403() - if dashboard_filter_context.extra_form_data: - efd = dashboard_filter_context.extra_form_data - extra_filters = efd.get("filters", []) + if efd := dashboard_filter_context.extra_form_data: + # Note: this helper currently mutates `json_body` and `efd` in place. + # Changes won't persist as these are dicts detached from the ORM state, + # but highlighting in case they're further used (mind the changes). + apply_dashboard_filter_context(json_body, efd) - for query in json_body.get("queries", []): - if extra_filters: - existing = query.get("filters") or [] - query["filters"] = existing + [ - {**f, "isExtra": True} for f in extra_filters - ] - - extras = query.get("extras") or {} - for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS: - if key in efd: - extras[key] = efd[key] - if extras: - query["extras"] = extras - - for ( - src_key, - target_key, - ) in EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS.items(): - if src_key in efd: - query[target_key] = efd[src_key] - - query["extra_form_data"] = efd - - # We need to apply the form data to the global context as jinja - # templating pulls form data from the request globally, so this - # fallback ensures it has the filters and extra_form_data applied - # when used in get_sqla_query which constructs the final query. - - # Jinja macros like metric() resolve dataset context from g.form_data - # when not given an explicit dataset_id. For GET requests there is no - # JSON body, so we must always expose the saved query context here. + # We need to apply the form data to the global context as jinja + # templating pulls form data from the request globally, so this + # fallback ensures it has the filters and extra_form_data applied + # when used in get_sqla_query which constructs the final query. g.form_data = json_body try: diff --git a/superset/charts/data/dashboard_filter_context.py b/superset/charts/data/dashboard_filter_context.py index 50b6ef4244e..fa4227a9c04 100644 --- a/superset/charts/data/dashboard_filter_context.py +++ b/superset/charts/data/dashboard_filter_context.py @@ -314,3 +314,39 @@ def get_dashboard_filter_context( ) return context + + +def apply_dashboard_filter_context( + query_context: dict[str, Any], + extra_form_data: dict[str, Any], +) -> None: + """ + Apply dashboard filter context. + + Filters are removed from ``extra_form_data`` to avoid duplicated values when + using ``filter_values()`` macro. + + :param query_context: The chart's query context (mutated in place) + :param extra_form_data: The dashboard's merged extra_form_data to apply. It's + also mutated in place. + """ + extra_filters = extra_form_data.pop("filters", []) + for query in query_context.get("queries", []): + if extra_filters: + existing_filters = query.get("filters") or [] + query["filters"] = existing_filters + [ + {**flt, "isExtra": True} for flt in extra_filters + ] + + extras = query.get("extras") or {} + for key in EXTRA_FORM_DATA_OVERRIDE_EXTRA_KEYS: + if key in extra_form_data: + extras[key] = extra_form_data[key] + if extras: + query["extras"] = extras + + for src_key, target_key in EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS.items(): + if src_key in extra_form_data: + query[target_key] = extra_form_data[src_key] + + query["extra_form_data"] = extra_form_data diff --git a/tests/unit_tests/charts/test_chart_data_api.py b/tests/unit_tests/charts/test_chart_data_api.py index 15b987c5e09..eea180f01da 100644 --- a/tests/unit_tests/charts/test_chart_data_api.py +++ b/tests/unit_tests/charts/test_chart_data_api.py @@ -16,12 +16,20 @@ # under the License. from __future__ import annotations +from typing import Any, TYPE_CHECKING from unittest.mock import MagicMock from flask import Flask, g +from superset.charts.data.dashboard_filter_context import ( + apply_dashboard_filter_context, +) +from superset.jinja_context import ExtraCache from superset.utils import json +if TYPE_CHECKING: + from superset.app import SupersetApp + def test_get_data_sets_g_form_data_without_dashboard_filter() -> None: """ @@ -73,6 +81,38 @@ def test_get_data_sets_g_form_data_without_dashboard_filter() -> None: assert g.form_data["queries"][0]["columns"] == ["col1"] +def test_apply_dashboard_filter_context_does_not_duplicate_filters( + app: SupersetApp, +) -> None: + """ + Regression test for the ``filters_dashboard_id`` parameter. + + A dashboard's filters must not be present in both query["filters"] and + query["extra_form_data"]["filters"]. Previously the same filter existed in both, + so Jinja's filter_values() read each value twice and produced SQL such as + ``country in ('USA', 'USA')``. + """ + query_context_json: dict[str, Any] = { + "datasource": {"id": 1, "type": "table"}, + "queries": [{"filters": [{"col": "year", "op": "IN", "val": [2004]}]}], + } + extra_form_data = {"filters": [{"col": "country", "op": "IN", "val": ["USA"]}]} + + apply_dashboard_filter_context(query_context_json, extra_form_data) + + query = query_context_json["queries"][0] + assert query["filters"] == [ + {"col": "year", "op": "IN", "val": [2004]}, + {"col": "country", "op": "IN", "val": ["USA"], "isExtra": True}, + ] + assert "filters" not in query["extra_form_data"] + + # filter_values() therefore returns the dashboard value exactly once. + with app.test_request_context("/api/v1/chart/1/data/"): + g.form_data = query_context_json + assert ExtraCache().filter_values("country") == ["USA"] + + def _extract_filename(form_value: str) -> str | None: """Run _extract_export_params_from_request with a form filename value.""" from superset.charts.data.api import ChartDataRestApi