Skip to content

fix(mcp): reject unparseable time_range instead of silently matching full table#42283

Draft
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:aminghadersohi/mcp-time-range-validation
Draft

fix(mcp): reject unparseable time_range instead of silently matching full table#42283
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:aminghadersohi/mcp-time-range-validation

Conversation

@aminghadersohi

Copy link
Copy Markdown
Contributor

SUMMARY

MCP tools that accept a free-form time_range string (query_dataset, get_table, manage_native_filters) forward it straight into a TEMPORAL_RANGE filter or native filter default with no validation.

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 with success: true.

Verified live against a local Superset MCP server (Postgres metadata DB, examples data, dataset 28 cleaned_sales_data, 2823 rows total):

get_table(dataset_id=28, metrics=["count"], time_range="banana")   -> 2823 rows, success: true, warnings: []
get_table(dataset_id=28, metrics=["count"], time_range="[year]")   -> 2823 rows
get_table(dataset_id=28, metrics=["count"], time_range="Last year") -> 0 rows (correct)
get_table(dataset_id=28, metrics=["count"], time_range="2003-01-01 : 2004-01-01") -> 1000 rows (correct)

This PR adds a shared validator (superset.mcp_service.common.time_range_validation) that:

  • Normalizes bracket shorthands ([second] through [year]) to a form get_since_until() resolves correctly, following the pattern from fix(mcp): normalize bracket-shorthand time ranges in query_dataset #42144 (which scoped this normalization to query_dataset only).
  • Rejects any other separator-less value that get_since_until() would silently discard, with a ValidationError listing the accepted forms so an LLM caller can self-correct instead of receiving a full-table result with no error signal.

Wired into QueryDatasetRequest.time_range, GetTableRequest.time_range, FilterTimeSpec.default_time_range, and NativeFilterUpdateSpec.default_time_range — closing the gap left open by #42144, which explicitly scoped get_table and manage_native_filters out (see discussion).

Why the guard lives in the MCP schema layer, not in get_since_until() itself: that function is also called from jinja_context.py and semantic_layers/mapper.py across the whole chart/dashboard query path. Making it raise there is a much larger blast radius and a separate discussion about whether silently-ignored time ranges are load-bearing anywhere. The MCP tools are the surface that accepts free-form, model-generated strings.

BREAKING CHANGE

Values like this month or lowercase last week that previously silently returned an unfiltered, full-table result now raise a ValidationError. This is the intended fix — callers previously had no signal that their filter was ignored, which is worse than a loud error for an LLM caller that could otherwise self-correct.

TESTING INSTRUCTIONS

  • pytest tests/unit_tests/mcp_service/common/test_time_range_validation.py
  • pytest tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py
  • pytest tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py
  • pytest tests/unit_tests/mcp_service/dashboard/tool/test_manage_native_filters.py
  • pytest tests/unit_tests/utils/date_parser_tests.py — confirms the shared validator's accepted-prefix grammar doesn't regress the existing get_since_until() test suite (this PR does not modify date_parser.py)
  • Manually: call get_table / query_dataset with time_range set to banana, this month, last week (lowercase), and [decade] against a dataset with a temporal column, and confirm each now returns a ValidationError instead of an unfiltered result.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 48.97959% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.14%. Comparing base (60a7c18) to head (188876f).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
...perset/mcp_service/common/time_range_validation.py 32.25% 21 Missing ⚠️
superset/mcp_service/dashboard/schemas.py 77.77% 2 Missing ⚠️
superset/mcp_service/dataset/schemas.py 66.66% 1 Missing ⚠️
superset/mcp_service/semantic_layer/schemas.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42283      +/-   ##
==========================================
- Coverage   65.15%   65.14%   -0.01%     
==========================================
  Files        2789     2790       +1     
  Lines      157543   157584      +41     
  Branches    35864    35872       +8     
==========================================
+ Hits       102644   102662      +18     
- Misses      52931    52953      +22     
- Partials     1968     1969       +1     
Flag Coverage Δ
hive 38.75% <48.97%> (+<0.01%) ⬆️
mysql 57.65% <48.97%> (-0.01%) ⬇️
postgres 57.70% <48.97%> (-0.01%) ⬇️
presto 40.66% <48.97%> (+<0.01%) ⬆️
python 59.08% <48.97%> (-0.01%) ⬇️
sqlite 57.31% <48.97%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aminghadersohi
aminghadersohi marked this pull request as ready for review July 22, 2026 03:31
@dosubot dosubot Bot added the risk:breaking-change Issues or PRs that will introduce breaking changes label Jul 22, 2026
…the full table

MCP tools that accept a free-form time_range string (query_dataset,
get_table, manage_native_filters) forward it straight into a
TEMPORAL_RANGE filter or native filter default with no validation.
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 with success: true.

Add a shared validator (superset.mcp_service.common.time_range_validation)
that normalizes bracket shorthands (e.g. [year] -> Last year, following
the pattern from apache#42144) and rejects any other separator-less value that
get_since_until() would silently discard, with a ValidationError listing
the accepted forms so an LLM caller can self-correct. Wired into
QueryDatasetRequest.time_range, GetTableRequest.time_range,
FilterTimeSpec.default_time_range, and
NativeFilterUpdateSpec.default_time_range.

The guard lives in the MCP schema layer rather than in
get_since_until() itself: that function is also called from
jinja_context.py and semantic_layers/mapper.py across the whole
chart/dashboard query path, where making it raise is a separate,
larger-blast-radius discussion. The MCP tools are the surface that
accepts free-form, model-generated strings.

BREAKING CHANGE: values like 'this month' or lowercase 'last week' that
previously silently returned an unfiltered, full-table result now raise
a ValidationError. This is the intended fix -- callers previously had
no signal that their filter was ignored.
@aminghadersohi
aminghadersohi force-pushed the aminghadersohi/mcp-time-range-validation branch from 4559795 to 188876f Compare July 22, 2026 03:39
@aminghadersohi
aminghadersohi marked this pull request as draft July 22, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review:draft risk:breaking-change Issues or PRs that will introduce breaking changes size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants