mikebridge commented on code in PR #45098: URL: https://github.com/apache/superset/pull/45098#discussion_r4227048279
########## superset/mcp_service/common/time_range_validation.py: ########## @@ -15,33 +15,11 @@ # 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. - -Acceptance is decided by *calling* ``get_since_until()`` and checking what -comes back, not by re-implementing its grammar here. Mirroring the grammar -would drift from the real parser and force a choice between rejecting valid -freeform values (``Last Monday``, ``Last year to date``) and admitting -malformed ones (``Last nonsense``, a bare ``Last``). Delegating gets both -right by construction. +"""Normalize MCP time ranges and expose shared parser validation as field errors. + +The shared parser rejects malformed separator-less ranges. MCP additionally +accepts bracket shorthands and empty defaults, and provides accepted-format Review Comment: Agreed, fixed in a408ede94e. Both filter validators (`QueryDatasetFilter` and `GetTableFilter`) now reject empty and whitespace-only TEMPORAL_RANGE values at request validation, and the error tells you the accepted forms. Optional top-level `time_range` fields still accept the empty default. The new tests cover both request schemas, blank and whitespace values, the preserved top-level default, and valid ranges (`No filter`, `Last week`, `<start> : <end>`). The focused suites pass. -- 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]
