From 2dfc770b0f71d73b65081a8c3473a6ddb62d0101 Mon Sep 17 00:00:00 2001 From: Jamile Celento Date: Wed, 4 Feb 2026 06:37:17 -0300 Subject: [PATCH] fix(native-filters): update TEMPORAL_RANGE filter subject when Time Column filter is applied (#36985) --- superset/utils/core.py | 66 ++- tests/integration_tests/explore/api_tests.py | 92 ++++ tests/unit_tests/utils/test_core.py | 508 +++++++++++++++++++ 3 files changed, 662 insertions(+), 4 deletions(-) diff --git a/superset/utils/core.py b/superset/utils/core.py index a5c69554559..23a3017bf2c 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1011,6 +1011,39 @@ def form_data_to_adhoc(form_data: dict[str, Any], clause: str) -> AdhocFilterCla return result +def _update_existing_temporal_filter( + temporal_filter: AdhocFilterClause, + granularity_sqla_override: str | None, + time_range: str | None, + chart_has_granularity_sqla: bool, +) -> None: + """Update an existing temporal filter with new subject/comparator.""" + if ( + granularity_sqla_override is not None + and temporal_filter.get("expressionType") == "SIMPLE" + ): + temporal_filter["subject"] = granularity_sqla_override + if time_range and not chart_has_granularity_sqla: + temporal_filter["comparator"] = time_range + + +def _create_temporal_filter( + granularity_sqla: str, + time_range: str, +) -> AdhocFilterClause: + """Create a new TEMPORAL_RANGE adhoc filter.""" + new_filter: AdhocFilterClause = { + "clause": "WHERE", + "expressionType": "SIMPLE", + "operator": FilterOperator.TEMPORAL_RANGE, + "subject": granularity_sqla, + "comparator": time_range, + "isExtra": True, + } + new_filter["filterOptionName"] = hash_from_dict(cast(dict[Any, Any], new_filter)) + return new_filter + + def merge_extra_form_data(form_data: dict[str, Any]) -> None: # noqa: C901 """ Merge extra form data (appends and overrides) into the main payload @@ -1059,10 +1092,35 @@ def merge_extra_form_data(form_data: dict[str, Any]) -> None: # noqa: C901 for fltr in append_filters if fltr ) - if form_data.get("time_range") and not form_data.get("granularity_sqla"): - for adhoc_filter in form_data.get("adhoc_filters", []): - if adhoc_filter.get("operator") == "TEMPORAL_RANGE": - adhoc_filter["comparator"] = form_data["time_range"] + + granularity_sqla_override = extra_form_data.get("granularity_sqla") + time_range = form_data.get("time_range") + chart_has_granularity_sqla = bool(form_data.get("granularity_sqla")) + + temporal_filters = [ + adhoc_filter + for adhoc_filter in adhoc_filters + if adhoc_filter.get("operator") == FilterOperator.TEMPORAL_RANGE + ] + + for temporal_filter in temporal_filters: + _update_existing_temporal_filter( + temporal_filter, + granularity_sqla_override, + time_range, + chart_has_granularity_sqla, + ) + + if ( + not temporal_filters + and granularity_sqla_override is not None + and time_range is not None + ): + new_temporal_filter = _create_temporal_filter( + granularity_sqla_override, + cast(str, time_range), + ) + adhoc_filters.append(new_temporal_filter) def merge_extra_filters(form_data: dict[str, Any]) -> None: # noqa: C901 diff --git a/tests/integration_tests/explore/api_tests.py b/tests/integration_tests/explore/api_tests.py index cc65870ca61..4c1d357462f 100644 --- a/tests/integration_tests/explore/api_tests.py +++ b/tests/integration_tests/explore/api_tests.py @@ -254,3 +254,95 @@ def test_get_url_params(test_client, login_as_admin, chart_id): "foo": "bar", "slice_id": str(chart_id), } + + +def test_granularity_sqla_override_updates_temporal_range_filter_subject( + test_client, login_as_admin, chart_id, admin_id, dataset +): + """ + Test that extra_form_data.granularity_sqla overrides TEMPORAL_RANGE filter subject. + + The flow is: + 1. Chart has TEMPORAL_RANGE adhoc filters on various columns + 2. Dashboard applies Time Column native filter selecting 'year' via extra_form_data + 3. Explore API processes form_data through merge_extra_filters/merge_extra_form_data + 4. All TEMPORAL_RANGE filter subjects should be updated to 'year' + """ + + form_data_with_temporal_filter = { + "datasource": f"{dataset.id}__{dataset.type}", + "viz_type": "country_map", + "time_range": "Last week", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "No filter", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "some_other_time_col", + }, + { + "clause": "WHERE", + "comparator": "foo", + "expressionType": "SIMPLE", + "operator": "==", + "subject": "non_temporal_col", + }, + { + "clause": "WHERE", + "comparator": "Last year", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "another_time_col", + }, + ], + "extra_form_data": { + "granularity_sqla": "year", + "time_range": "Last month", + }, + } + + test_form_data_key = f"test_granularity_override_key_{chart_id}_{dataset.id}" + entry: TemporaryExploreState = { + "owner": admin_id, + "datasource_id": dataset.id, + "datasource_type": dataset.type, + "chart_id": chart_id, + "form_data": json.dumps(form_data_with_temporal_filter), + } + cache_manager.explore_form_data_cache.set(test_form_data_key, entry) + + try: + resp = test_client.get( + f"api/v1/explore/?form_data_key={test_form_data_key}" + f"&datasource_id={dataset.id}&datasource_type={dataset.type}" + ) + assert resp.status_code == 200 + + data = json.loads(resp.data.decode("utf-8")) + result = data.get("result") + form_data = result["form_data"] + + adhoc_filters = form_data.get("adhoc_filters", []) + temporal_range_filters = [ + f + for f in adhoc_filters + if f.get("operator") == "TEMPORAL_RANGE" + and f.get("expressionType") == "SIMPLE" + ] + + assert len(temporal_range_filters) == 2, "Expected two TEMPORAL_RANGE filters" + for temporal_filter in temporal_range_filters: + assert temporal_filter["subject"] == "year", ( + "Time Column native filter (granularity_sqla) should override " + "TEMPORAL_RANGE filter subject for all matching filters" + ) + + non_temporal_filters = [f for f in adhoc_filters if f.get("operator") == "=="] + assert len(non_temporal_filters) == 1 + assert non_temporal_filters[0]["subject"] == "non_temporal_col" + + assert form_data["time_range"] == "Last month" + assert form_data.get("granularity") == "year" + finally: + cache_manager.explore_form_data_cache.delete(test_form_data_key) diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py index fe1b3311ef2..463ee32180a 100644 --- a/tests/unit_tests/utils/test_core.py +++ b/tests/unit_tests/utils/test_core.py @@ -31,6 +31,7 @@ from superset.utils.core import ( cast_to_boolean, check_is_safe_zip, DateColumn, + FilterOperator, generic_find_constraint_name, generic_find_fk_constraint_name, get_datasource_full_name, @@ -39,6 +40,7 @@ from superset.utils.core import ( get_user_agent, is_test, merge_extra_filters, + merge_extra_form_data, merge_request_params, normalize_dttm_col, parse_boolean_string, @@ -1089,6 +1091,512 @@ def test_merge_extra_filters_when_applied_time_extras_predefined(): } +def test_merge_extra_form_data_updates_temporal_range_subject(): + """ + Test that when extra_form_data contains granularity_sqla, it should update + the subject of any TEMPORAL_RANGE adhoc filter to use the new time column. + """ + form_data = { + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "No filter", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + } + ], + "extra_form_data": { + "granularity_sqla": "event_date", + "time_range": "Last week", + }, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["subject"] == "event_date" + assert form_data["time_range"] == "Last week" + assert form_data["granularity"] == "event_date" + assert "extra_form_data" not in form_data + + +def test_time_column_filter_with_multiple_temporal_range_filters(): + """ + Test that Time Column native filter updates ALL TEMPORAL_RANGE filters + in the adhoc_filters list, but leaves non-TEMPORAL_RANGE filters unchanged. + """ + form_data = { + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "Last week", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "default_time_col", + }, + { + "clause": "WHERE", + "comparator": "foo", + "expressionType": "SIMPLE", + "operator": "==", + "subject": "some_column", + }, + { + "clause": "WHERE", + "comparator": "Last month", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "another_time_col", + }, + ], + "extra_form_data": { + "granularity_sqla": "selected_time_col", + }, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["subject"] == "selected_time_col" + assert form_data["adhoc_filters"][2]["subject"] == "selected_time_col" + assert form_data["adhoc_filters"][1]["subject"] == "some_column" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_skips_sql_expression_filters(): + """ + Test that SQL expression type filters are not modified when granularity_sqla + is provided. Only SIMPLE expression type filters should have their subject updated. + """ + form_data = { + "adhoc_filters": [ + { + "clause": "WHERE", + "expressionType": "SQL", + "operator": "TEMPORAL_RANGE", + "sqlExpression": "created_at > '2020-01-01'", + }, + { + "clause": "WHERE", + "comparator": "Last week", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + }, + ], + "extra_form_data": { + "granularity_sqla": "event_date", + }, + } + merge_extra_form_data(form_data) + + assert "subject" not in form_data["adhoc_filters"][0] + assert form_data["adhoc_filters"][1]["subject"] == "event_date" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_time_range_without_granularity_sqla(): + """ + Test that when form_data has time_range but no granularity_sqla, + the TEMPORAL_RANGE filter comparator is updated to match time_range. + """ + form_data = { + "time_range": "Last month", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "No filter", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + } + ], + "extra_form_data": {}, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["comparator"] == "Last month" + assert form_data["adhoc_filters"][0]["subject"] == "created_at" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_no_subject_update_when_granularity_override_none(): + """ + Test that when granularity_sqla_override is None, the TEMPORAL_RANGE filter + subject should NOT be updated, even if expressionType is SIMPLE. + """ + original_subject = "original_time_col" + form_data = { + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "Last week", + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": original_subject, + } + ], + "extra_form_data": {}, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["subject"] == original_subject + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_update_comparator_when_time_range_is_falsy(): + """ + Test that when time_range is falsy (None, empty string, or False), + the TEMPORAL_RANGE filter comparator should NOT be updated. + """ + original_comparator = "Original time range" + form_data = { + "time_range": None, + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": original_comparator, + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + } + ], + "extra_form_data": {}, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["comparator"] == original_comparator + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_no_comparator_update_when_time_range_empty(): + """ + Test that when time_range is an empty string (falsy), the TEMPORAL_RANGE + filter comparator should NOT be updated. + """ + original_comparator = "Original time range" + form_data = { + "time_range": "", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": original_comparator, + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + } + ], + "extra_form_data": {}, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["comparator"] == original_comparator + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_update_comparator_when_has_granularity_sqla(): + """ + Test that when form_data has granularity_sqla (truthy), the TEMPORAL_RANGE + filter comparator should NOT be updated, even if time_range exists. + + When granularity_sqla is present in form_data, it indicates a legacy chart + where granularity_sqla and time_range are separate form_data attributes. + """ + original_comparator = "Original time range" + form_data = { + "time_range": "Last month", + "granularity_sqla": "event_date", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": original_comparator, + "expressionType": "SIMPLE", + "operator": "TEMPORAL_RANGE", + "subject": "created_at", + } + ], + "extra_form_data": {}, + } + merge_extra_form_data(form_data) + + assert form_data["adhoc_filters"][0]["comparator"] == original_comparator + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_removes_extra_form_data(): + """ + Test that merge_extra_form_data removes extra_form_data from form_data + after processing (it calls form_data.pop("extra_form_data", {})). + """ + form_data = { + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "event_date", + "time_range": "Last week", + }, + } + merge_extra_form_data(form_data) + + assert "extra_form_data" not in form_data + assert form_data["granularity"] == "event_date" + assert form_data["time_range"] == "Last week" + + +def test_merge_extra_form_data_creates_temporal_filter_when_none_exists(): + """ + Test that when granularity_sqla_override and time_range are provided + but no TEMPORAL_RANGE filter exists, a new temporal filter is created. + """ + form_data = { + "time_range": "2022-01-22 : 2025-01-22", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "foo", + "expressionType": "SIMPLE", + "operator": "==", + "subject": "some_column", + } + ], + "extra_form_data": { + "granularity_sqla": "updated_at", + }, + } + merge_extra_form_data(form_data) + + temporal_filters = [ + f + for f in form_data["adhoc_filters"] + if f.get("operator") == FilterOperator.TEMPORAL_RANGE + ] + assert len(temporal_filters) == 1 + assert temporal_filters[0]["subject"] == "updated_at" + assert temporal_filters[0]["comparator"] == "2022-01-22 : 2025-01-22" + assert temporal_filters[0]["expressionType"] == "SIMPLE" + assert temporal_filters[0]["clause"] == "WHERE" + assert temporal_filters[0]["isExtra"] is True + assert "filterOptionName" in temporal_filters[0] + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_creates_temporal_filter_with_empty_adhoc_filters(): + """ + Test that a temporal filter is created when adhoc_filters is empty + and granularity_sqla_override and time_range are provided. + """ + form_data = { + "time_range": "Last month", + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "created_at", + }, + } + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == 1 + assert form_data["adhoc_filters"][0]["operator"] == FilterOperator.TEMPORAL_RANGE + assert form_data["adhoc_filters"][0]["subject"] == "created_at" + assert form_data["adhoc_filters"][0]["comparator"] == "Last month" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_creates_temporal_filter_with_missing_adhoc_filters(): + """ + Test that a temporal filter is created when adhoc_filters key doesn't exist + and granularity_sqla_override and time_range are provided. + """ + form_data = { + "time_range": "2024-01-01 : 2024-03-01", + "extra_form_data": { + "granularity_sqla": "event_date", + }, + } + merge_extra_form_data(form_data) + + assert "adhoc_filters" in form_data + assert len(form_data["adhoc_filters"]) == 1 + assert form_data["adhoc_filters"][0]["operator"] == FilterOperator.TEMPORAL_RANGE + assert form_data["adhoc_filters"][0]["subject"] == "event_date" + assert form_data["adhoc_filters"][0]["comparator"] == "2024-01-01 : 2024-03-01" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_create_filter_when_granularity_missing(): + """ + Test that no temporal filter is created when granularity_sqla_override + is missing, even if time_range is provided. + """ + form_data = { + "time_range": "Last week", + "adhoc_filters": [], + "extra_form_data": {}, + } + original_filters_count = len(form_data["adhoc_filters"]) + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == original_filters_count + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_create_filter_when_time_range_missing(): + """ + Test that no temporal filter is created when time_range is missing, + even if granularity_sqla_override is provided. + """ + form_data = { + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "updated_at", + }, + } + original_filters_count = len(form_data["adhoc_filters"]) + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == original_filters_count + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_create_filter_when_time_range_none(): + """ + Test that no temporal filter is created when time_range is None, + even if granularity_sqla_override is provided. + """ + form_data = { + "time_range": None, + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "created_at", + }, + } + original_filters_count = len(form_data["adhoc_filters"]) + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == original_filters_count + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_does_not_create_filter_when_granularity_none(): + """ + Test that no temporal filter is created when granularity_sqla_override + is None, even if time_range is provided. + """ + form_data = { + "time_range": "Last month", + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": None, + }, + } + original_filters_count = len(form_data["adhoc_filters"]) + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == original_filters_count + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_creates_filter_with_different_temporal_columns(): + """ + Test creating temporal filters for different temporal columns + (event_date, created_at, updated_at) to ensure the fix works + across multiple column types. + """ + temporal_columns = ["event_date", "created_at", "updated_at"] + time_range = "2022-01-22 : 2025-01-22" + + for column in temporal_columns: + form_data = { + "time_range": time_range, + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": column, + }, + } + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == 1 + assert form_data["adhoc_filters"][0]["subject"] == column + assert form_data["adhoc_filters"][0]["comparator"] == time_range + assert ( + form_data["adhoc_filters"][0]["operator"] == FilterOperator.TEMPORAL_RANGE + ) + + +def test_merge_extra_form_data_does_not_create_duplicate_when_filter_exists(): + """ + Test that when a TEMPORAL_RANGE filter already exists, a new one + is NOT created. Instead, the existing filter is updated. + """ + form_data = { + "time_range": "Last week", + "adhoc_filters": [ + { + "clause": "WHERE", + "comparator": "No filter", + "expressionType": "SIMPLE", + "operator": FilterOperator.TEMPORAL_RANGE, + "subject": "original_column", + } + ], + "extra_form_data": { + "granularity_sqla": "new_column", + }, + } + merge_extra_form_data(form_data) + + temporal_filters = [ + f + for f in form_data["adhoc_filters"] + if f.get("operator") == FilterOperator.TEMPORAL_RANGE + ] + assert len(temporal_filters) == 1 + assert temporal_filters[0]["subject"] == "new_column" + assert temporal_filters[0]["comparator"] == "Last week" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_date_format_with_timestamp_column(): + """ + Test that date format time_range (YYYY-MM-DD) works correctly + when applied to a timestamp column. + """ + form_data = { + "time_range": "2022-01-22 : 2025-01-22", + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "created_at", + }, + } + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == 1 + temporal_filter = form_data["adhoc_filters"][0] + assert temporal_filter["operator"] == FilterOperator.TEMPORAL_RANGE + assert temporal_filter["subject"] == "created_at" + assert temporal_filter["comparator"] == "2022-01-22 : 2025-01-22" + assert temporal_filter["expressionType"] == "SIMPLE" + assert "extra_form_data" not in form_data + + +def test_merge_extra_form_data_timestamp_format_with_date_column(): + """ + Test that timestamp format time_range (YYYY-MM-DDTHH:MM:SS) works correctly + when applied to a date column. + """ + form_data = { + "time_range": "2022-01-22T00:00:00 : 2025-01-22T23:59:59", + "adhoc_filters": [], + "extra_form_data": { + "granularity_sqla": "event_date", + }, + } + merge_extra_form_data(form_data) + + assert len(form_data["adhoc_filters"]) == 1 + temporal_filter = form_data["adhoc_filters"][0] + assert temporal_filter["operator"] == FilterOperator.TEMPORAL_RANGE + assert temporal_filter["subject"] == "event_date" + assert temporal_filter["comparator"] == "2022-01-22T00:00:00 : 2025-01-22T23:59:59" + assert temporal_filter["expressionType"] == "SIMPLE" + assert "extra_form_data" not in form_data + + def test_merge_request_params_when_url_params_undefined(): form_data = {"since": "2000", "until": "now"} url_params = {"form_data": form_data, "dashboard_ids": "(1,2,3,4,5)"}