codeant-ai-for-open-source[bot] commented on code in PR #42283:
URL: https://github.com/apache/superset/pull/42283#discussion_r3692249282
##########
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:
**Suggestion:** The validator only checks the dedicated `time_range` field;
`QueryDatasetFilter.val` remains `Any`, so callers can submit `{"op":
"TEMPORAL_RANGE", "val": "banana"}` through `filters` and bypass this
validation. `query_dataset` forwards those filter values directly to the query,
preserving the silent full-table behavior this change is intended to prevent.
Validate `TEMPORAL_RANGE` filter values as well, or reject that operator from
the generic filter list when it is not validated. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Generic `query_dataset` temporal filters bypass validation.
- ❌ Invalid values can return unfiltered historical results.
- ⚠️ Responses may report successful but incorrectly scoped queries.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eba9f0ffd5ff434e8606a3d6eafb8d3d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eba9f0ffd5ff434e8606a3d6eafb8d3d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 803:806
**Comment:**
*Incomplete Implementation: The validator only checks the dedicated
`time_range` field; `QueryDatasetFilter.val` remains `Any`, so callers can
submit `{"op": "TEMPORAL_RANGE", "val": "banana"}` through `filters` and bypass
this validation. `query_dataset` forwards those filter values directly to the
query, preserving the silent full-table behavior this change is intended to
prevent. Validate `TEMPORAL_RANGE` filter values as well, or reject that
operator from the generic filter list when it is not validated.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42283&comment_hash=fbc38719747b786935d2d26475ebe5cbd179bfcaa0e15e3750e9ea87ed5995d1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42283&comment_hash=fbc38719747b786935d2d26475ebe5cbd179bfcaa0e15e3750e9ea87ed5995d1&reaction=dislike'>👎</a>
##########
superset/mcp_service/common/time_range_validation.py:
##########
@@ -0,0 +1,193 @@
+# 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.
+
+"""
+Shared ``time_range`` validation for MCP tools that forward a free-form,
+model-generated string into a ``TEMPORAL_RANGE`` filter or a native filter's
+``default_time_range``.
+
+``superset.utils.date_parser.get_since_until()`` only rewrites a
+separator-less ``time_range`` into a bounded range when it recognizes one of
+a handful of prefixes (``Last``, ``Next``, ``previous calendar ...``,
+``Current ...``, ``first ... of ...``). Anything else -- ``banana``,
+``this month``, lowercase ``last week``, ``[decade]`` -- silently falls
+through to an unbounded ``(None, today)`` result: no error, no warning, and
+the query returns the entire table.
+
+That silent behavior is unfixable at the ``get_since_until()`` level without
+risking regressions across the whole chart/dashboard query path (see
+``jinja_context.py`` and ``semantic_layers/mapper.py``, which also call it).
+The MCP tools are the surface that accepts free-form, model-generated
+strings, so the guard lives here instead: reject anything
+``get_since_until()`` would silently discard, with a message that lists the
+accepted forms so the caller (an LLM) can self-correct.
+"""
+
+from __future__ import annotations
+
+import re
+
+from superset.constants import NO_TIME_RANGE
+
+# Bracket shorthands (e.g. "[year]", "[quarter]") are not a Superset
+# time-range grammar -- they appear when an LLM copies a grain token from a
+# dashboard filter context. Map them to an equivalent form that
+# get_since_until() resolves correctly.
+#
+# "[second]"/"[minute]"/"[hour]" map to explicit DATEADD/DATETIME
+# expressions rather than "Last second"/"Last minute"/"Last hour": bare
+# "Last <sub-day unit>" pairs a since-expression resolved against "now"
+# with a default until-expression resolved against "today" (midnight), so
+# since ends up after until and get_since_until() raises "From date cannot
+# be larger than to date" (or, for "hour" specifically, the literal-string
+# fallback parser resolves it to a nonsensical timestamp that trips the
+# same check -- "hour" isn't in get_since_until()'s scope+unit regex, see
+# _SUB_DAY_LAST_PATTERN below). Explicit DATEADD/DATETIME expressions
+# sidestep that mismatch by resolving both ends against "now".
+BRACKET_SHORTHAND_TO_TIME_RANGE: dict[str, str] = {
+ "[second]": "DATEADD(DATETIME('now'), -1, SECOND) : DATETIME('now')",
+ "[minute]": "DATEADD(DATETIME('now'), -1, MINUTE) : DATETIME('now')",
+ "[hour]": "DATEADD(DATETIME('now'), -1, HOUR) : DATETIME('now')",
+ "[day]": "Last day",
+ "[week]": "Last week",
+ "[month]": "Last month",
+ "[quarter]": "Last quarter",
+ "[year]": "Last year",
+}
+
+# Bare "Last <n>? <second|minute|hour>[s]" values hit the same since/until
+# mismatch as the bracket shorthands above -- normalize them the same way,
+# to an explicit DATEADD/DATETIME range resolved against "now" on both
+# ends, instead of rejecting them outright. "Next <second|minute|hour>"
+# does not need the same treatment: get_since_until() pairs it with a
+# "today" (midnight) *since*, so since <= until always holds.
+_SUB_DAY_LAST_PATTERN =
re.compile(r"^Last\s+(?:(\d+)\s+)?(second|minute|hour)s?$")
+_SUB_DAY_UNIT_TO_DATEADD_UNIT = {
+ "second": "SECOND",
+ "minute": "MINUTE",
+ "hour": "HOUR",
+}
+
+_SEPARATOR = " : "
+
+# Mirrors the exact `startswith` prefixes get_since_until() checks (in
+# that order) before it will rewrite a separator-less time_range into a
+# " : "-bounded range. Case-sensitive to match get_since_until() exactly --
+# e.g. "Last week" parses, "last week" does not.
+_PREVIOUS_CALENDAR_PREFIXES = (
+ "previous calendar week",
+ "previous calendar month",
+ "previous calendar quarter",
+ "previous calendar year",
+)
+_CURRENT_PREFIXES = (
+ "Current day",
+ "Current week",
+ "Current month",
+ "Current quarter",
+ "Current year",
+)
+
+# Mirrors date_parser.get_since_until()'s nth_subunit_pattern, the one
+# separator-less grammar handled by a regex rather than a literal prefix
+# (e.g. "first week of this year"). Kept byte-for-byte in sync with that
+# pattern; the existing date_parser test suite is the guard against drift.
+_NTH_SUBUNIT_PATTERN = re.compile(
+ r"^(first|1st)\s{1,5}"
+ r"(week|month|quarter)\s{1,5}of\s{1,5}"
+ r"(?:(this|last|next|prior)\s{1,5})?"
+ r"(?:the\s{1,5})?"
+ r"(week|month|quarter|year)$",
+ re.IGNORECASE,
+)
+
+
+def _normalize_sub_day_last(value: str) -> str | None:
+ """Rewrite a bare "Last <n>? <second|minute|hour>[s]" value into an
+ explicit DATEADD/DATETIME range resolved against "now" on both ends.
+ Returns ``None`` if ``value`` isn't a sub-day "Last ..." value."""
+ if (match := _SUB_DAY_LAST_PATTERN.match(value)) is None:
+ return None
+ delta = int(match.group(1)) if match.group(1) else 1
+ unit = _SUB_DAY_UNIT_TO_DATEADD_UNIT[match.group(2)]
+ return f"DATEADD(DATETIME('now'), -{delta}, {unit}) : DATETIME('now')"
+
+
+def _has_recognized_bare_prefix(value: str) -> bool:
+ """Whether get_since_until() rewrites this separator-less value into a
+ bounded range, rather than silently discarding it.
+
+ Callers must check ``_normalize_sub_day_last()`` first: a bare "Last
+ <second|minute|hour>" value matches ``startswith("Last")`` here but
+ needs rewriting, not pass-through, so it isn't re-admitted as-is.
+ """
+ if value.startswith("Last") or value.startswith("Next"):
+ return True
Review Comment:
**Suggestion:** These broad prefix checks accept malformed values such as
`Last nonsense`, `Next nonsense`, or `Last` even though `get_since_until()`
only recognizes specific relative-time grammars. Such values are passed through
to downstream parsing, where they can raise a lower-level parse/database error
instead of the intended MCP `ValidationError`. Match the actual
`get_since_until()` grammar rather than treating every string beginning with
these prefixes as valid. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Malformed relative ranges reach semantic-layer parsing.
- ⚠️ `query_dataset` returns backend parse errors.
- ⚠️ LLM callers lose the accepted-format correction guidance.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4c679ed54c344705ba32de0d1dc22ffd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4c679ed54c344705ba32de0d1dc22ffd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/common/time_range_validation.py
**Line:** 138:139
**Comment:**
*Incorrect Condition Logic: These broad prefix checks accept malformed
values such as `Last nonsense`, `Next nonsense`, or `Last` even though
`get_since_until()` only recognizes specific relative-time grammars. Such
values are passed through to downstream parsing, where they can raise a
lower-level parse/database error instead of the intended MCP `ValidationError`.
Match the actual `get_since_until()` grammar rather than treating every string
beginning with these prefixes as valid.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42283&comment_hash=1c32bdbcd3826d26648a269c5dcd74782fd75c9e94c2052d0fa4f302f39e27be&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42283&comment_hash=1c32bdbcd3826d26648a269c5dcd74782fd75c9e94c2052d0fa4f302f39e27be&reaction=dislike'>👎</a>
--
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]
