aminghadersohi commented on code in PR #45098: URL: https://github.com/apache/superset/pull/45098#discussion_r4226106701
########## 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: `""` is not only an empty default here. A TEMPORAL_RANGE filter `val` of `""` or `" "` passes this validator in the dataset and semantic_layer schemas. The parser then raises, and `query_dataset` reports it as "Unexpected error" with a traceback. Reject `""` in those filter validators. -- 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]
