From 37c4a36fdb01ffe807e683ead82fc63ae5da3e56 Mon Sep 17 00:00:00 2001 From: Alexandru Soare <37236580+alexandrusoare@users.noreply.github.com> Date: Wed, 25 Mar 2026 01:06:44 +0200 Subject: [PATCH] fix(report): raise warning when filter type not recognized (#38676) --- superset/reports/models.py | 189 ++++++++++++++----------- tests/unit_tests/reports/model_test.py | 81 +++++++++-- 2 files changed, 183 insertions(+), 87 deletions(-) diff --git a/superset/reports/models.py b/superset/reports/models.py index e26c3fbdbf9..66f37eefddd 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -217,15 +217,15 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): logger.warning(warning_msg) continue - params = { - **params, - **self._generate_native_filter( - native_filter_id, - filter_type, - native_filter.get("columnName") or "", - native_filter.get("filterValues") or [], - ), - } + filter_config, filter_warning = self._generate_native_filter( + native_filter_id, + filter_type, + native_filter.get("columnName") or "", + native_filter.get("filterValues") or [], + ) + if filter_warning: + warnings.append(filter_warning) + params = {**params, **filter_config} # hack(hughhh): workaround for escaping prison not handling quotes right rison = prison.dumps(params) rison = rison.replace("'", "%27") @@ -237,62 +237,85 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): filter_type: str, column_name: str, values: list[Optional[str]], - ) -> dict[str, Any]: + ) -> tuple[dict[str, Any], Optional[str]]: + """ + Generate a native filter configuration for the given filter type. + + Returns: + A tuple of (filter_config, warning_message). If the filter type is + unrecognized, returns an empty dict and a warning message. + """ if filter_type == "filter_time": # For select filters, we need to use the "IN" operator - return { - native_filter_id or "": { - "id": native_filter_id or "", - "extraFormData": {"time_range": values[0]}, - "filterState": {"value": values[0]}, - "ownState": {}, - } - } - elif filter_type == "filter_timegrain": - return { - native_filter_id or "": { - "id": native_filter_id or "", - "extraFormData": { - "time_grain_sqla": values[0], # grain - }, - "filterState": { - # "label": "30 second", # grain_label - "value": values # grain - }, - "ownState": {}, - } - } + return ( + { + native_filter_id or "": { + "id": native_filter_id or "", + "extraFormData": {"time_range": values[0]}, + "filterState": {"value": values[0]}, + "ownState": {}, + } + }, + None, + ) + if filter_type == "filter_timegrain": + return ( + { + native_filter_id or "": { + "id": native_filter_id or "", + "extraFormData": { + "time_grain_sqla": values[0], # grain + }, + "filterState": { + # "label": "30 second", # grain_label + "value": values # grain + }, + "ownState": {}, + } + }, + None, + ) - elif filter_type == "filter_timecolumn": - return { - native_filter_id or "": { - "extraFormData": { - "granularity_sqla": values[0] # column_name - }, - "filterState": { - "value": values # column_name - }, - } - } + if filter_type == "filter_timecolumn": + return ( + { + native_filter_id or "": { + "extraFormData": { + "granularity_sqla": values[0] # column_name + }, + "filterState": { + "value": values # column_name + }, + } + }, + None, + ) - elif filter_type == "filter_select": - return { - native_filter_id or "": { - "id": native_filter_id or "", - "extraFormData": { - "filters": [ - {"col": column_name or "", "op": "IN", "val": values or []} - ] - }, - "filterState": { - "label": column_name or "", - "validateStatus": False, - "value": values or [], - }, - "ownState": {}, - } - } - elif filter_type == "filter_range": + if filter_type == "filter_select": + return ( + { + native_filter_id or "": { + "id": native_filter_id or "", + "extraFormData": { + "filters": [ + { + "col": column_name or "", + "op": "IN", + "val": values or [], + } + ] + }, + "filterState": { + "label": column_name or "", + "validateStatus": False, + "value": values or [], + }, + "ownState": {}, + } + }, + None, + ) + if filter_type == "filter_range": # For range filters, values should be [min, max] or [value] for single value min_val = values[0] if len(values) > 0 else None max_val = values[1] if len(values) > 1 else None @@ -303,25 +326,33 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): if max_val is not None: filters.append({"col": column_name or "", "op": "<=", "val": max_val}) - return { - native_filter_id or "": { - "id": native_filter_id or "", - "extraFormData": {"filters": filters}, - "filterState": { - "value": [min_val, max_val], - "label": f"{min_val} ≤ x ≤ {max_val}" - if min_val and max_val - else f"x ≥ {min_val}" - if min_val - else f"x ≤ {max_val}" - if max_val - else "", - }, - "ownState": {}, - } - } + return ( + { + native_filter_id or "": { + "id": native_filter_id or "", + "extraFormData": {"filters": filters}, + "filterState": { + "value": [min_val, max_val], + "label": f"{min_val} ≤ x ≤ {max_val}" + if min_val and max_val + else f"x ≥ {min_val}" + if min_val + else f"x ≤ {max_val}" + if max_val + else "", + }, + "ownState": {}, + } + }, + None, + ) - return {} + warning_msg = ( + f"Skipping native filter with unrecognized filter type '{filter_type}' " + f"(filter_id: {native_filter_id})" + ) + logger.warning(warning_msg) + return {}, warning_msg class ReportRecipients(Model, AuditMixinNullable): diff --git a/tests/unit_tests/reports/model_test.py b/tests/unit_tests/reports/model_test.py index a741a06bf70..33ad3461dbf 100644 --- a/tests/unit_tests/reports/model_test.py +++ b/tests/unit_tests/reports/model_test.py @@ -93,9 +93,10 @@ def test_report_generate_native_filter_no_values(): filter_type = "filter_select" values = None - assert report_schedule._generate_native_filter( + result, warning = report_schedule._generate_native_filter( native_filter_id, filter_type, column_name, values - ) == { + ) + assert result == { "filter_id": { "id": "filter_id", "extraFormData": { @@ -109,6 +110,7 @@ def test_report_generate_native_filter_no_values(): "ownState": {}, } } + assert warning is None def test_get_native_filters_params_missing_filter_values(): @@ -215,9 +217,10 @@ def test_report_generate_native_filter(): column_name = "column_name" values = ["value1", "value2"] - assert report_schedule._generate_native_filter( + result, warning = report_schedule._generate_native_filter( native_filter_id, filter_type, column_name, values - ) == { + ) + assert result == { "filter_id": { "extraFormData": { "filters": [ @@ -233,6 +236,7 @@ def test_report_generate_native_filter(): "ownState": {}, } } + assert warning is None def test_get_native_filters_params_empty(): @@ -269,9 +273,10 @@ def test_report_generate_native_filter_empty_values(): column_name = "column_name" values = [] - assert report_schedule._generate_native_filter( + result, warning = report_schedule._generate_native_filter( native_filter_id, filter_type, column_name, values - ) == { + ) + assert result == { "filter_id": { "extraFormData": { "filters": [{"col": "column_name", "op": "IN", "val": []}] @@ -285,6 +290,7 @@ def test_report_generate_native_filter_empty_values(): "ownState": {}, } } + assert warning is None def test_report_generate_native_filter_no_column_name(): @@ -297,9 +303,10 @@ def test_report_generate_native_filter_no_column_name(): column_name = "" values = ["value1", "value2"] - assert report_schedule._generate_native_filter( + result, warning = report_schedule._generate_native_filter( native_filter_id, filter_type, column_name, values - ) == { + ) + assert result == { "filter_id": { "extraFormData": { "filters": [{"col": "", "op": "IN", "val": ["value1", "value2"]}] @@ -313,3 +320,61 @@ def test_report_generate_native_filter_no_column_name(): "ownState": {}, } } + assert warning is None + + +def test_report_generate_native_filter_unknown_filter_type(): + """ + Test the ``_generate_native_filter`` method with an unknown filter type. + Should return empty dict and a warning message. + """ + report_schedule = ReportSchedule() + native_filter_id = "filter_id" + filter_type = "filter_unknown" + column_name = "column_name" + values = ["value1", "value2"] + + result, warning = report_schedule._generate_native_filter( + native_filter_id, filter_type, column_name, values + ) + assert result == {} + assert warning is not None + assert "unrecognized filter type" in warning + assert "filter_unknown" in warning + assert "filter_id" in warning + + +def test_get_native_filters_params_unknown_filter_type(): + """ + Test the ``get_native_filters_params`` method with an unknown filter type. + Should skip the filter and include a warning. + """ + report_schedule = ReportSchedule() + report_schedule.extra = { + "dashboard": { + "nativeFilters": [ + { + "nativeFilterId": "filter_1", + "filterType": "filter_unknown_type", + "columnName": "column_name", + "filterValues": ["value1"], + }, + { + "nativeFilterId": "filter_2", + "filterType": "filter_select", + "columnName": "column_name", + "filterValues": ["value2"], + }, + ] + } + } + + result, warnings = report_schedule.get_native_filters_params() + # The unknown filter should be skipped, valid filter should be present + assert "filter_2" in result + assert "filter_1" not in result + assert "value2" in result + # Should have one warning for the unknown filter type + assert len(warnings) == 1 + assert "unrecognized filter type" in warnings[0] + assert "filter_unknown_type" in warnings[0]