SEPURI-SAI-KRISHNA commented on code in PR #45461:
URL: https://github.com/apache/superset/pull/45461#discussion_r4367540882
##########
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:
Confirmed, and the gap was where you put it: dropping `ValueError` from the
outer `except` (`helpers.py:2936`) left all 288 tests green, while the other
three arms were already pinned.
A reversed range arrives there as a bare `ValueError: From date cannot be
larger
than to date`, `a : b` as `TimeRangeParseFailError`. Added as a third case on
`test_processing_time_offsets_rejects_a_bad_temporal_range`, which
parametrizes
the expected message too, and renamed the two existing ids to "unreadable" so
the three read apart. Each arm has its own named case.
The same shape one site over, at the place my description claimed was already
safe. `is_valid_date_range` splits an offset on `:` while the parser wants
`" : "`, so `time_offsets: ["2024-01-01:2024-12-31"]` passes the check and
then
fails as a `ValidationError`, and the handler around that loop
(`helpers.py:3029`) caught only `ValueError`, so it escaped as a 500. Needs
`DATE_RANGE_TIMESHIFTS_ENABLED`. Widened it, added a test, and corrected the
description.
That handler's `ValueError` half has no separating test, since the inner
`except`
at `:2968` catches reversed bounds first. That case is in for the message
rather
than as a gate.
--
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]
