diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 90f574e27cb..a59e13afaa5 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -99,6 +99,7 @@ class BaseReportState: self._scheduled_dttm = scheduled_dttm self._start_dttm = datetime.utcnow() self._execution_id = execution_id + self._filter_warnings: list[str] = [] def update_report_schedule_and_log( self, @@ -265,7 +266,11 @@ class BaseReportState: if ( dashboard_state := self._report_schedule.extra.get("dashboard") ) and feature_flag_manager.is_feature_enabled("ALERT_REPORT_TABS"): - native_filter_params = self._report_schedule.get_native_filters_params() + native_filter_params, filter_warnings = ( + self._report_schedule.get_native_filters_params() + ) + if filter_warnings: + self._filter_warnings.extend(filter_warnings) if anchor := dashboard_state.get("anchor"): try: anchor_list = json.loads(anchor) @@ -835,7 +840,13 @@ class ReportNotTriggeredErrorState(BaseReportState): ) return self.send() - self.update_report_schedule_and_log(ReportState.SUCCESS) + # Include filter warnings in the log if any were collected + warning_message = ( + ";".join(self._filter_warnings) if self._filter_warnings else None + ) + self.update_report_schedule_and_log( + ReportState.SUCCESS, error_message=warning_message + ) except (SupersetErrorsException, Exception) as first_ex: error_message = str(first_ex) if isinstance(first_ex, SupersetErrorsException): @@ -980,7 +991,13 @@ class ReportSuccessState(BaseReportState): try: self.send() - self.update_report_schedule_and_log(ReportState.SUCCESS) + # Include filter warnings in the log if any were collected + warning_message = ( + ";".join(self._filter_warnings) if self._filter_warnings else None + ) + self.update_report_schedule_and_log( + ReportState.SUCCESS, error_message=warning_message + ) except Exception as ex: # pylint: disable=broad-except try: self.update_report_schedule_and_log( diff --git a/superset/reports/models.py b/superset/reports/models.py index 4fa573d6f1e..e26c3fbdbf9 100644 --- a/superset/reports/models.py +++ b/superset/reports/models.py @@ -16,6 +16,7 @@ # under the License. """A collection of ORM sqlalchemy models for Superset""" +import logging from typing import Any, Optional import prison @@ -47,6 +48,8 @@ from superset.reports.types import ReportScheduleExtra from superset.utils.backports import StrEnum from superset.utils.core import MediumText +logger = logging.getLogger(__name__) + metadata = Model.metadata # pylint: disable=no-member @@ -187,24 +190,46 @@ class ReportSchedule(AuditMixinNullable, ExtraJSONMixin, Model): def crontab_humanized(self) -> str: return get_description(self.crontab) - def get_native_filters_params(self) -> str: + def get_native_filters_params(self) -> tuple[str, list[str]]: + """ + Generate native filter params for dashboard URL. + + Returns: + A tuple of (rison_encoded_params, list_of_warning_messages). + Warnings are returned so they can be surfaced to users in the + execution log. + """ params: dict[str, Any] = {} + warnings: list[str] = [] dashboard = self.extra.get("dashboard") if dashboard and dashboard.get("nativeFilters"): - for filter in dashboard.get("nativeFilters") or []: # type: ignore + native_filters = dashboard.get("nativeFilters") or [] + for native_filter in native_filters: # type: ignore + native_filter_id = native_filter.get("nativeFilterId") + filter_type = native_filter.get("filterType") + + if native_filter_id is None or filter_type is None: + warning_msg = ( + f"Skipping malformed native filter missing required " + f"fields: {native_filter}" + ) + warnings.append(warning_msg) + logger.warning(warning_msg) + continue + params = { **params, **self._generate_native_filter( - filter["nativeFilterId"], - filter["filterType"], - filter["columnName"], - filter["filterValues"], + native_filter_id, + filter_type, + native_filter.get("columnName") or "", + native_filter.get("filterValues") or [], ), } # hack(hughhh): workaround for escaping prison not handling quotes right rison = prison.dumps(params) rison = rison.replace("'", "%27") - return rison + return rison, warnings def _generate_native_filter( self, diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index e40a45d9bbe..f1f73bcf4bc 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -290,6 +290,10 @@ def test_get_dashboard_urls_with_multiple_tabs( "urlParams": None, } } + mock_report_schedule.get_native_filters_params.return_value = ( # type: ignore + "()", + [], + ) class_instance: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" @@ -333,6 +337,10 @@ def test_get_dashboard_urls_with_exporting_dashboard_only( "urlParams": None, } } + mock_report_schedule.get_native_filters_params.return_value = ( # type: ignore + "()", + [], + ) mock_run.return_value = "url1" class_instance: BaseReportState = BaseReportState( diff --git a/tests/unit_tests/reports/model_test.py b/tests/unit_tests/reports/model_test.py index 19e12e14072..a741a06bf70 100644 --- a/tests/unit_tests/reports/model_test.py +++ b/tests/unit_tests/reports/model_test.py @@ -14,7 +14,6 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -import pytest from superset.reports.models import ReportSchedule @@ -37,9 +36,14 @@ def test_get_native_filters_params(): } } - assert report_schedule.get_native_filters_params() == ( - "(filter_id:(extraFormData:(filters:!((col:column_name,op:IN,val:!(value1,value2)))),filterState:(label:column_name,validateStatus:!f,value:!(value1,value2)),id:filter_id,ownState:()))" + result, warnings = report_schedule.get_native_filters_params() + expected = ( + "(filter_id:(extraFormData:(filters:!((col:column_name,op:IN," + "val:!(value1,value2)))),filterState:(label:column_name," + "validateStatus:!f,value:!(value1,value2)),id:filter_id,ownState:()))" ) + assert result == expected + assert warnings == [] def test_get_native_filters_params_multiple_filters(): @@ -66,9 +70,17 @@ def test_get_native_filters_params_multiple_filters(): } } - assert report_schedule.get_native_filters_params() == ( - "(filter_id_1:(extraFormData:(filters:!((col:column_name_1,op:IN,val:!(value1,value2)))),filterState:(label:column_name_1,validateStatus:!f,value:!(value1,value2)),id:filter_id_1,ownState:()),filter_id_2:(extraFormData:(filters:!((col:column_name_2,op:IN,val:!(value3,value4)))),filterState:(label:column_name_2,validateStatus:!f,value:!(value3,value4)),id:filter_id_2,ownState:()))" + result, warnings = report_schedule.get_native_filters_params() + expected = ( + "(filter_id_1:(extraFormData:(filters:!((col:column_name_1,op:IN," + "val:!(value1,value2)))),filterState:(label:column_name_1," + "validateStatus:!f,value:!(value1,value2)),id:filter_id_1,ownState:())," + "filter_id_2:(extraFormData:(filters:!((col:column_name_2,op:IN," + "val:!(value3,value4)))),filterState:(label:column_name_2," + "validateStatus:!f,value:!(value3,value4)),id:filter_id_2,ownState:()))" ) + assert result == expected + assert warnings == [] def test_report_generate_native_filter_no_values(): @@ -99,9 +111,10 @@ def test_report_generate_native_filter_no_values(): } -def test_get_native_filters_params_invalid_structure(): +def test_get_native_filters_params_missing_filter_values(): """ - Test the ``get_native_filters_params`` method with invalid structure. + Test the ``get_native_filters_params`` method with missing filterValues. + Should handle gracefully by using empty list as default. """ report_schedule = ReportSchedule() report_schedule.extra = { @@ -111,29 +124,85 @@ def test_get_native_filters_params_invalid_structure(): "nativeFilterId": "filter_id", "columnName": "column_name", "filterType": "filter_select", - # Missing "filterValues" key + # Missing "filterValues" key - should default to [] } ] } } - with pytest.raises(KeyError, match="'filterValues'"): - report_schedule.get_native_filters_params() + # Should not raise, should handle gracefully with empty filterValues + result, warnings = report_schedule.get_native_filters_params() + assert "filter_id" in result + assert "column_name" in result + assert warnings == [] -# todo(hugh): how do we want to handle this case? -# def test_report_generate_native_filter_invalid_filter_id(): -# """ -# Test the ``_generate_native_filter`` method with invalid filter id. -# """ -# report_schedule = ReportSchedule() -# native_filter_id = None -# column_name = "column_name" -# values = ["value1", "value2"] +def test_get_native_filters_params_explicit_none_values(): + """ + Test the ``get_native_filters_params`` method with explicit None values. + Should handle gracefully by coercing None to empty string/list. + """ + report_schedule = ReportSchedule() + report_schedule.extra = { + "dashboard": { + "nativeFilters": [ + { + "nativeFilterId": "filter_id", + "columnName": None, # Explicit None + "filterType": "filter_select", + "filterValues": None, # Explicit None + } + ] + } + } -# assert report_schedule._generate_native_filter( -# native_filter_id, column_name, values -# ) == {} + # Should not raise TypeError, should handle gracefully + result, warnings = report_schedule.get_native_filters_params() + assert "filter_id" in result + assert warnings == [] + + +def test_get_native_filters_params_missing_required_fields(): + """ + Test the ``get_native_filters_params`` method with missing required fields. + Filters missing nativeFilterId or filterType should be skipped. + """ + report_schedule = ReportSchedule() + report_schedule.extra = { + "dashboard": { + "nativeFilters": [ + { + # Missing nativeFilterId - should be skipped + "filterType": "filter_select", + "columnName": "column_name", + "filterValues": ["value1"], + }, + { + # Missing filterType - should be skipped + "nativeFilterId": "filter_2", + "columnName": "column_name", + "filterValues": ["value2"], + }, + { + # Valid filter - should be processed + "nativeFilterId": "filter_3", + "filterType": "filter_select", + "columnName": "column_name", + "filterValues": ["value3"], + }, + ] + } + } + + result, warnings = report_schedule.get_native_filters_params() + # Only the valid filter should be in the result + assert "filter_3" in result + assert "filter_2" not in result + assert "value1" not in result + assert "value3" in result + # Two malformed filters should generate two warnings + assert len(warnings) == 2 + assert all("Skipping malformed native filter" in w for w in warnings) def test_report_generate_native_filter(): @@ -173,7 +242,9 @@ def test_get_native_filters_params_empty(): report_schedule = ReportSchedule() report_schedule.extra = {} - assert report_schedule.get_native_filters_params() == "()" + result, warnings = report_schedule.get_native_filters_params() + assert result == "()" + assert warnings == [] def test_get_native_filters_params_no_native_filters(): @@ -183,7 +254,9 @@ def test_get_native_filters_params_no_native_filters(): report_schedule = ReportSchedule() report_schedule.extra = {"dashboard": {"nativeFilters": []}} - assert report_schedule.get_native_filters_params() == "()" + result, warnings = report_schedule.get_native_filters_params() + assert result == "()" + assert warnings == [] def test_report_generate_native_filter_empty_values():