fix(chart API): Do not duplicate Jinja-applied filters with filters_dashboard_id (#41131)

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:

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

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