aminghadersohi commented on code in PR #42144:
URL: https://github.com/apache/superset/pull/42144#discussion_r3624272483
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
+
+ def test_year_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[year]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_quarter_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[quarter]"}
+ )
+ assert req.time_range == "Last quarter"
+
+ def test_month_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[month]"}
+ )
+ assert req.time_range == "Last month"
+
+ def test_week_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[week]"}
+ )
+ assert req.time_range == "Last week"
+
+ def test_day_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[day]"}
+ )
+ assert req.time_range == "Last day"
+
+ def test_hour_bracket_normalized(self) -> None:
+ """'[hour]' maps to an explicit DATEADD/DATETIME expression.
+
+ 'Last hour' is deliberately not used: get_since_until() resolves its
+ since-expression against 'now' but its default until-expression
+ against 'today' (midnight), so since ends up after until and raises
+ a "From date cannot be larger than to date" error.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[hour]"}
+ )
+ assert req.time_range == "DATEADD(DATETIME('now'), -1, HOUR) :
DATETIME('now')"
+
+ def test_minute_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[minute]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, MINUTE) :
DATETIME('now')"
+ )
+
+ def test_second_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[second]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, SECOND) :
DATETIME('now')"
+ )
+
+ def test_bracket_uppercase_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[YEAR]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_bracket_with_whitespace_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " [year] "}
+ )
+ assert req.time_range == "Last year"
+
+ def test_valid_superset_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "Last 7
days"}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_iso_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "2024-01-01 : 2024-12-31",
+ }
+ )
+ assert req.time_range == "2024-01-01 : 2024-12-31"
+
+ def test_non_bracket_value_is_stripped(self) -> None:
+ """Non-bracket values must be trimmed too, not just the lookup key.
+
+ Otherwise leading/trailing whitespace around an otherwise valid
+ relative range (e.g. from an LLM) would propagate to
+ get_since_until() and could cause avoidable parse failures.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " Last 7
days "}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_none_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": None}
+ )
+ assert req.time_range is None
+
+
[email protected]
+async def test_query_dataset_bracket_year_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[year]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648: LLM clients passing '[year]' verbatim
+ triggered TimeRangeParseFailError because the raw token was forwarded to
+ parse_human_datetime() without normalization. The schema validator now
+ maps it to 'Last year' before the query context is built.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
Review Comment:
Confirmed — mocking ChartDataCommand.validate/.run and
QueryContextFactory.create meant get_since_until() (the function that actually
raises TimeRangeParseFailError, reached via
QueryObjectFactory/QueryContextProcessor) was never invoked in this test. The
tool-level assertions only proved the normalized value was forwarded through
the filter dict unchanged, which duplicates the schema-level unit tests. Added
a direct call to get_since_until() on the captured value so the test exercises
the real parser and would fail if it ever raised again. See 8e61783e80.
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
+
+ def test_year_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[year]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_quarter_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[quarter]"}
+ )
+ assert req.time_range == "Last quarter"
+
+ def test_month_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[month]"}
+ )
+ assert req.time_range == "Last month"
+
+ def test_week_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[week]"}
+ )
+ assert req.time_range == "Last week"
+
+ def test_day_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[day]"}
+ )
+ assert req.time_range == "Last day"
+
+ def test_hour_bracket_normalized(self) -> None:
+ """'[hour]' maps to an explicit DATEADD/DATETIME expression.
+
+ 'Last hour' is deliberately not used: get_since_until() resolves its
+ since-expression against 'now' but its default until-expression
+ against 'today' (midnight), so since ends up after until and raises
+ a "From date cannot be larger than to date" error.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[hour]"}
+ )
+ assert req.time_range == "DATEADD(DATETIME('now'), -1, HOUR) :
DATETIME('now')"
+
+ def test_minute_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[minute]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, MINUTE) :
DATETIME('now')"
+ )
+
+ def test_second_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[second]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, SECOND) :
DATETIME('now')"
+ )
+
+ def test_bracket_uppercase_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[YEAR]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_bracket_with_whitespace_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " [year] "}
+ )
+ assert req.time_range == "Last year"
+
+ def test_valid_superset_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "Last 7
days"}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_iso_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "2024-01-01 : 2024-12-31",
+ }
+ )
+ assert req.time_range == "2024-01-01 : 2024-12-31"
+
+ def test_non_bracket_value_is_stripped(self) -> None:
+ """Non-bracket values must be trimmed too, not just the lookup key.
+
+ Otherwise leading/trailing whitespace around an otherwise valid
+ relative range (e.g. from an LLM) would propagate to
+ get_since_until() and could cause avoidable parse failures.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " Last 7
days "}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_none_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": None}
+ )
+ assert req.time_range is None
+
+
[email protected]
+async def test_query_dataset_bracket_year_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[year]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648: LLM clients passing '[year]' verbatim
+ triggered TimeRangeParseFailError because the raw token was forwarded to
+ parse_human_datetime() without normalization. The schema validator now
+ maps it to 'Last year' before the query context is built.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory.create",
+ side_effect=capture_create,
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "query_dataset",
+ {
+ "request": {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "[year]",
+ }
+ },
+ )
+
+ data = json.loads(result.content[0].text)
+ # Must succeed (no error_type) and forward the normalized value
+ assert "error_type" not in data or data.get("error_type") is None
+ assert len(captured_queries) == 1
+ temporal_filters = [
+ f for f in captured_queries[0]["filters"] if f["op"] ==
"TEMPORAL_RANGE"
+ ]
+ assert len(temporal_filters) == 1
+ assert temporal_filters[0]["val"] == "Last year"
+
+
[email protected]
+async def test_query_dataset_bracket_hour_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[hour]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648. Unlike the other bracket shorthands,
+ '[hour]' does not normalize to 'Last hour': get_since_until() resolves
+ that expression's since-clause against 'now' but its until-clause
+ against 'today' (midnight), which raises "From date cannot be larger
+ than to date" for any sub-day unit. The schema validator instead maps
+ '[hour]' to an explicit DATEADD/DATETIME range that resolves both ends
+ against 'now'.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
Review Comment:
Same fix applied here — added a direct get_since_until() call on the
captured hour-range value so the real parser path is exercised, not just the
mocked tool-call pipeline. See 8e61783e80.
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
Review Comment:
Confirmed and fixed — the class docstring said all bracket shorthands map to
a canonical Last <unit> form, but second/minute/hour intentionally use explicit
DATEADD/DATETIME expressions (see the fix for the "Unusable time-range
mappings" thread above). Updated the docstring to describe both branches. See
8e61783e80.
--
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]
