codeant-ai-for-open-source[bot] commented on code in PR #42144:
URL: https://github.com/apache/superset/pull/42144#discussion_r3616215793
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -865,6 +891,14 @@ class QueryDatasetRequest(QueryCacheControl):
description="Maximum number of rows to return (default 1000, max
50000).",
)
+ @field_validator("time_range")
+ @classmethod
+ def normalize_time_range(cls, v: str | None) -> str | None:
+ if v is None:
+ return v
+ canonical = _BRACKET_SHORTHAND_TO_TIME_RANGE.get(v.strip().lower())
+ return canonical if canonical is not None else v
Review Comment:
**Suggestion:** The validator trims `time_range` only for bracket-shorthand
lookup but returns the original untrimmed value when no shorthand match is
found. This can propagate leading/trailing whitespace (for example, around
otherwise valid relative ranges) into downstream time parsing and cause
avoidable parse failures; return the trimmed value for the non-mapped path as
well. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Query dataset MCP tool may misparse user time_range inputs.
- ⚠️ LLM-generated time ranges with whitespace may fail normalization.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a `QueryDatasetRequest` instance (class at
`superset/mcp_service/dataset/schemas.py:833-893`) with a non-bracket time
range
containing surrounding whitespace, e.g. `time_range=" Last 7 days "` and
valid `metrics`
or `columns` so the model is accepted.
2. During model initialization, Pydantic runs `normalize_time_range`
(`@field_validator("time_range")` at lines 894-900). The validator receives
`v=" Last 7
days "`, computes `v.strip().lower()` as `"last 7 days"`, and looks this up
in
`_BRACKET_SHORTHAND_TO_TIME_RANGE` (lines 821-830), which returns `None`
because it is not
a bracket shorthand.
3. Because `canonical` is `None`, the validator executes `return canonical
if canonical is
not None else v` at line 900, so the stored `time_range` on the
`QueryDatasetRequest`
instance remains `" Last 7 days "` with leading/trailing spaces preserved.
4. That untrimmed `time_range` string is then passed along the normal
query_dataset
execution path (not shown in the diff, but comments at lines 810-813
explicitly state that
the strings are ultimately interpreted by `get_since_until()`), so any
downstream parser
that expects canonical Superset shorthands without extra whitespace may
misinterpret or
reject the value, potentially causing time-range parsing failures for
otherwise valid
inputs.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29646c9bba734e2dbffef8ea90ccbf35&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=29646c9bba734e2dbffef8ea90ccbf35&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:** 899:900
**Comment:**
*Logic Error: The validator trims `time_range` only for
bracket-shorthand lookup but returns the original untrimmed value when no
shorthand match is found. This can propagate leading/trailing whitespace (for
example, around otherwise valid relative ranges) into downstream time parsing
and cause avoidable parse failures; return the trimmed value for the non-mapped
path as well.
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%2F42144&comment_hash=3b650d3cbae0ecf58cd823584d56a26fbbf4a8d2654bb1a5f6c60e4a60cac613&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42144&comment_hash=3b650d3cbae0ecf58cd823584d56a26fbbf4a8d2654bb1a5f6c60e4a60cac613&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]
