sadpandajoe commented on code in PR #45461:
URL: https://github.com/apache/superset/pull/45461#discussion_r4328281017
##########
tests/unit_tests/models/helpers_test.py:
##########
@@ -1084,6 +1085,123 @@ def
test_get_sqla_query_does_not_mutate_adhoc_orderby_with_jinja(
)
[email protected](
+ "time_range",
+ ["a : b", "2024-02-01 : 2024-01-01"],
+)
+def test_get_sqla_query_rejects_a_bad_range_in_a_second_temporal_filter(
+ database: Database, time_range: str
+) -> None:
+ """
+ Building the SQL parses every temporal filter, not just the one the query
+ context was built from, so a bad range in any of them is a bad request.
+ """
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.exceptions import QueryObjectValidationError
+
+ table = SqlaTable(
+ database=database,
+ schema=None,
+ table_name="t",
+ columns=[
+ TableColumn(column_name="a", type="INTEGER"),
+ TableColumn(column_name="ds", is_dttm=True, type="TIMESTAMP"),
+ TableColumn(column_name="other_ds", is_dttm=True,
type="TIMESTAMP"),
+ ],
+ )
+
+ with pytest.raises(QueryObjectValidationError) as excinfo:
+ table.get_sqla_query(
+ columns=["a"],
+ metrics=[],
+ granularity="ds",
+ is_timeseries=False,
+ row_limit=100,
+ filter=[
+ # The range the query context is built from, which is parsed
+ # and reported on well before this point.
+ {
+ "col": "ds",
+ "op": FilterOperator.TEMPORAL_RANGE,
+ "val": "2024-01-01 : 2024-12-31",
+ },
+ {
+ "col": "other_ds",
+ "op": FilterOperator.TEMPORAL_RANGE,
+ "val": time_range,
+ },
+ ],
+ )
+
+ assert "other_ds" in excinfo.value.message
+
+
[email protected](
+ "filters",
+ [
+ pytest.param(
+ [
+ {"col": "ds", "val": "2024-01-01 : 2024-12-31"},
+ {"col": "other_ds", "val": "a : b"},
+ ],
+ id="bad range in the last temporal filter",
Review Comment:
Both `processing_time_offsets` cases use `a : b`, which only raises the
marshmallow `ValidationError`. The `ValueError` branch of the new outer `except
(ValidationError, ValueError)` is never reached, so dropping `ValueError` there
would leave every new test green while a last temporal filter like `2024-02-01
: 2024-01-01` goes back to a 500. Could you add a case with that reversed range
as the last filter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
