aminghadersohi commented on code in PR #42283:
URL: https://github.com/apache/superset/pull/42283#discussion_r3692637710
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -823,12 +802,8 @@ class QueryDatasetRequest(QueryCacheControl):
@field_validator("time_range")
@classmethod
- def normalize_time_range(cls, v: str | None) -> str | None:
- if v is None:
- return v
- stripped = v.strip()
- canonical = _BRACKET_SHORTHAND_TO_TIME_RANGE.get(stripped.lower())
- return canonical if canonical is not None else stripped
+ def _validate_time_range(cls, v: str | None) -> str | None:
+ return validate_time_range(v)
Review Comment:
Correct, and a good catch — this was the same bug the PR exists to fix,
reachable through a field I hadn't covered. Fixed in 90df755.
Reproduced it exactly as described:
```python
QueryDatasetRequest.model_validate({
"dataset_id": 1, "metrics": ["count"],
"filters": [{"col": "ts", "op": "TEMPORAL_RANGE", "val": "banana"}],
})
# -> accepted unvalidated; get_since_until("banana") -> (None, 2026-07-31)
```
`query_dataset.py:214` forwards `request.filters` into the query verbatim,
and `TEMPORAL_RANGE` resolves through `get_since_until()` just like the
dedicated field, so this reproduced the silent full-table match with `success:
true`.
Took the "validate the filter values" option rather than rejecting the
operator — `TEMPORAL_RANGE` in `filters` is a legitimate way to express a time
filter, and rejecting it would be a real capability loss. `QueryDatasetFilter`
and `GetTableFilter` (same `val: Any` shape, same exposure via `get_table`) now
run `TEMPORAL_RANGE` string values through `validate_time_range()`, so the
longhand spelling gets identical treatment to `time_range` — including the
sub-day normalization:
```
{"op": "TEMPORAL_RANGE", "val": "banana"} -> ValidationError
{"op": "TEMPORAL_RANGE", "val": "Last hour"} -> DATEADD(DATETIME('now'), -1,
HOUR) : DATETIME('now')
{"op": "==", "val": "banana"} -> untouched
```
Non-temporal operators are left alone — `banana` is a fine value to compare
a text column against — as are non-string values.
--
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]
