diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 56c0611bc67..c6e2b693b96 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -2399,7 +2399,7 @@ class ExploreMixin: # pylint: disable=too-many-public-methods time_grain: Optional[str] = None, label: Optional[str] = "__time", template_processor: Optional[BaseTemplateProcessor] = None, - ) -> ColumnElement: + ) -> Optional[ColumnElement]: col = ( time_col.get_timestamp_expression( time_grain=time_grain, @@ -2427,6 +2427,8 @@ class ExploreMixin: # pylint: disable=too-many-public-methods self.dttm_sql_literal(end_dttm, time_col) ) ) + if not l: + return None return and_(True, *l) def values_for_column( # pylint: disable=too-many-locals @@ -2945,14 +2947,14 @@ class ExploreMixin: # pylint: disable=too-many-public-methods and self.main_dttm_col != dttm_col.column_name and self.main_dttm_col not in removed_filters ): - time_filters.append( - self.get_time_filter( - time_col=columns_by_name[self.main_dttm_col], - start_dttm=from_dttm, - end_dttm=to_dttm, - template_processor=template_processor, - ) + _main_dttm_filter = self.get_time_filter( + time_col=columns_by_name[self.main_dttm_col], + start_dttm=from_dttm, + end_dttm=to_dttm, + template_processor=template_processor, ) + if _main_dttm_filter is not None: + time_filters.append(_main_dttm_filter) # Check if time filter should be skipped because it was handled in template. # Check both the actual column name and __timestamp alias @@ -2968,7 +2970,8 @@ class ExploreMixin: # pylint: disable=too-many-public-methods end_dttm=to_dttm, template_processor=template_processor, ) - time_filters.append(time_filter_column) + if time_filter_column is not None: + time_filters.append(time_filter_column) # Always remove duplicates by column name, as sometimes `metrics_exprs` # can have the same name as a groupby column (e.g. when users use @@ -3205,16 +3208,16 @@ class ExploreMixin: # pylint: disable=too-many-public-methods time_shift=time_shift, extras=extras, ) - target_clause_list.append( - self.get_time_filter( - time_col=col_obj, - start_dttm=_since, - end_dttm=_until, - time_grain=flt_grain, - label=sqla_col.key, - template_processor=template_processor, - ) + _temporal_filter = self.get_time_filter( + time_col=col_obj, + start_dttm=_since, + end_dttm=_until, + time_grain=flt_grain, + label=sqla_col.key, + template_processor=template_processor, ) + if _temporal_filter is not None: + target_clause_list.append(_temporal_filter) else: raise QueryObjectValidationError( _("Invalid filter operation type: %(op)s", op=op) @@ -3301,14 +3304,14 @@ class ExploreMixin: # pylint: disable=too-many-public-methods inner_time_filter = [] if dttm_col and not db_engine_spec.time_groupby_inline: - inner_time_filter = [ - self.get_time_filter( - time_col=dttm_col, - start_dttm=inner_from_dttm or from_dttm, - end_dttm=inner_to_dttm or to_dttm, - template_processor=template_processor, - ) - ] + _inner_filter = self.get_time_filter( + time_col=dttm_col, + start_dttm=inner_from_dttm or from_dttm, + end_dttm=inner_to_dttm or to_dttm, + template_processor=template_processor, + ) + if _inner_filter is not None: + inner_time_filter = [_inner_filter] subq = subq.where(and_(*(where_clause_and + inner_time_filter))) subq = subq.group_by(*inner_groupby_exprs) diff --git a/tests/unit_tests/models/test_no_filter_time_range.py b/tests/unit_tests/models/test_no_filter_time_range.py new file mode 100644 index 00000000000..09139920897 --- /dev/null +++ b/tests/unit_tests/models/test_no_filter_time_range.py @@ -0,0 +1,307 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests that 'No filter' temporal range produces no WHERE 1 = 1 clause.""" + +from __future__ import annotations + +from datetime import datetime, timezone + +from flask import Flask +from pytest_mock import MockerFixture + +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn +from superset.models.core import Database +from superset.superset_typing import QueryObjectDict + + +def _make_dataset(mocker: MockerFixture) -> SqlaTable: + database = Database( + id=1, + database_name="test_db", + sqlalchemy_uri="sqlite://", + ) + columns = [ + TableColumn(column_name="time_start", is_dttm=1, type="TIMESTAMP"), + TableColumn(column_name="value", type="INTEGER"), + ] + dataset = SqlaTable( + table_name="test_table", + columns=columns, + main_dttm_col="time_start", + database=database, + metrics=[SqlMetric(metric_name="count", expression="COUNT(*)")], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.is_guest_user", + return_value=False, + ) + return dataset + + +def test_get_time_filter_returns_none_when_no_bounds( + mocker: MockerFixture, + app: Flask, +) -> None: + """get_time_filter returns None when both start_dttm and end_dttm are None.""" + dataset = _make_dataset(mocker) + time_col = dataset.columns[0] + + with app.test_request_context(): + result = dataset.get_time_filter( + time_col=time_col, + start_dttm=None, + end_dttm=None, + ) + + assert result is None + + +def test_get_time_filter_returns_clause_with_partial_bounds( + mocker: MockerFixture, + app: Flask, +) -> None: + """get_time_filter returns a clause when at least one bound is set.""" + dataset = _make_dataset(mocker) + time_col = dataset.columns[0] + + with app.test_request_context(): + result_start_only = dataset.get_time_filter( + time_col=time_col, + start_dttm=datetime(2024, 1, 1, tzinfo=timezone.utc), + end_dttm=None, + ) + result_end_only = dataset.get_time_filter( + time_col=time_col, + start_dttm=None, + end_dttm=datetime(2024, 1, 31, tzinfo=timezone.utc), + ) + result_both = dataset.get_time_filter( + time_col=time_col, + start_dttm=datetime(2024, 1, 1, tzinfo=timezone.utc), + end_dttm=datetime(2024, 1, 31, tzinfo=timezone.utc), + ) + + assert result_start_only is not None + assert result_end_only is not None + assert result_both is not None + + +def test_no_filter_temporal_range_omits_where_1_equals_1( + mocker: MockerFixture, + app: Flask, +) -> None: + """Generating SQL with 'No filter' time range must not produce WHERE 1 = 1.""" + dataset = _make_dataset(mocker) + + # "No filter" is represented as None/None for from_dttm/to_dttm + query_obj: QueryObjectDict = { + "granularity": "time_start", + "from_dttm": None, + "to_dttm": None, + "is_timeseries": False, + "filter": [ + { + "col": "time_start", + "op": "TEMPORAL_RANGE", + "val": "No filter", + } + ], + "metrics": ["count"], + "columns": [], + } + + with app.test_request_context(): + sqla_query = dataset.get_query_str_extended(query_obj, mutate=False) + generated_sql = sqla_query.sql + + assert "1 = 1" not in generated_sql, ( + f"Expected no '1 = 1' in generated SQL when 'No filter' is selected, " + f"but got: {generated_sql}" + ) + + +def test_no_filter_with_other_filters_preserved( + mocker: MockerFixture, + app: Flask, +) -> None: + """'No filter' time range + regular filter => WHERE has only the regular filter.""" + dataset = _make_dataset(mocker) + + query_obj: QueryObjectDict = { + "granularity": "time_start", + "from_dttm": None, + "to_dttm": None, + "is_timeseries": False, + "filter": [ + { + "col": "time_start", + "op": "TEMPORAL_RANGE", + "val": "No filter", + }, + { + "col": "value", + "op": "==", + "val": "42", + }, + ], + "metrics": ["count"], + "columns": [], + } + + with app.test_request_context(): + sqla_query = dataset.get_query_str_extended(query_obj, mutate=False) + generated_sql = sqla_query.sql + + assert "1 = 1" not in generated_sql, ( + f"Expected no '1 = 1' with 'No filter' + regular filter, got: {generated_sql}" + ) + # Regular filter should still be present + assert "value" in generated_sql.lower(), ( + f"Regular filter on 'value' should still appear in SQL, got: {generated_sql}" + ) + + +def test_actual_from_to_dttm_produces_time_filter( + mocker: MockerFixture, + app: Flask, +) -> None: + """Generating SQL with actual from/to dates applies a WHERE time filter.""" + dataset = _make_dataset(mocker) + + query_obj: QueryObjectDict = { + "granularity": "time_start", + "from_dttm": datetime(2024, 1, 1, tzinfo=timezone.utc), + "to_dttm": datetime(2024, 1, 31, tzinfo=timezone.utc), + "is_timeseries": False, + "filter": [], + "metrics": ["count"], + "columns": [], + } + + with app.test_request_context(): + sqla_query = dataset.get_query_str_extended(query_obj, mutate=False) + generated_sql = sqla_query.sql + + assert "1 = 1" not in generated_sql, ( + f"Expected no '1 = 1' in SQL with actual date bounds, got: {generated_sql}" + ) + assert "time_start" in generated_sql.lower(), ( + f"Expected time_start column in WHERE clause, got: {generated_sql}" + ) + + +def test_series_limit_with_time_filter_includes_time_in_inner_subquery( + mocker: MockerFixture, + app: Flask, +) -> None: + """Series limit subquery includes time filter when actual dates are provided. + + This covers the branch at helpers.py where inner_time_filter is set to + [_inner_filter] when get_time_filter returns a non-None clause inside the + series-limit subquery block. + """ + database = Database( + id=1, + database_name="test_db", + sqlalchemy_uri="sqlite://", + ) + columns = [ + TableColumn(column_name="time_start", is_dttm=1, type="TIMESTAMP"), + TableColumn(column_name="category", type="VARCHAR(100)"), + TableColumn(column_name="value", type="INTEGER"), + ] + dataset = SqlaTable( + table_name="test_table", + columns=columns, + main_dttm_col="time_start", + database=database, + metrics=[SqlMetric(metric_name="count", expression="COUNT(*)")], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.is_guest_user", + return_value=False, + ) + + query_obj: QueryObjectDict = { + "granularity": "time_start", + "from_dttm": datetime(2024, 1, 1, tzinfo=timezone.utc), + "to_dttm": datetime(2024, 1, 31, tzinfo=timezone.utc), + "is_timeseries": True, + "groupby": ["category"], + "series_limit": 5, + "filter": [], + "metrics": ["count"], + "columns": [], + } + + with app.test_request_context(): + sqla_query = dataset.get_query_str_extended(query_obj, mutate=False) + generated_sql = sqla_query.sql + + assert "1 = 1" not in generated_sql, ( + f"Expected no '1 = 1' in generated SQL with series_limit and actual dates, " + f"but got: {generated_sql}" + ) + assert "time_start" in generated_sql.lower(), ( + f"Expected time_start to appear in SQL (inner subquery WHERE), " + f"but got: {generated_sql}" + ) + + +def test_temporal_range_filter_with_actual_dates_produces_time_filter( + mocker: MockerFixture, + app: Flask, +) -> None: + """TEMPORAL_RANGE filter with actual dates applies a WHERE time filter.""" + dataset = _make_dataset(mocker) + + query_obj: QueryObjectDict = { + "granularity": None, + "from_dttm": None, + "to_dttm": None, + "is_timeseries": False, + "filter": [ + { + "col": "time_start", + "op": "TEMPORAL_RANGE", + "val": "2024-01-01T00:00:00 : 2024-01-31T00:00:00", + } + ], + "metrics": ["count"], + "columns": [], + } + + with app.test_request_context(): + sqla_query = dataset.get_query_str_extended(query_obj, mutate=False) + generated_sql = sqla_query.sql + + assert "time_start" in generated_sql.lower(), ( + f"Expected time_start in WHERE clause for TEMPORAL_RANGE with dates, " + f"got: {generated_sql}" + ) + assert "1 = 1" not in generated_sql, ( + f"Expected no '1 = 1' for TEMPORAL_RANGE with actual dates, " + f"got: {generated_sql}" + )