bodapatisaikrishna commented on PR #44211: URL: https://github.com/apache/superset/pull/44211#issuecomment-5653881556
Closing this myself after digging further — turns out this exact behavior is already documented and deliberately not fixed at this level. `superset/mcp_service/common/time_range_validation.py` (which I hadn't seen when I opened this) explains why: fixing case-sensitivity in `get_since_until()` directly was considered and rejected as too risky, since it has ~14 callers across the chart/dashboard query path, migrations, and the semantic layer (see the module's docstring, and the `SC-114824` reference). The guard was deliberately placed at the MCP tool boundary instead, and there's a test (`TestValidateTimeRangeRejectsSilentFailures::test_previously_silent_values_now_raise[last week]`) that explicitly locks in "last week" as a value that should keep failing at that layer rather than start resolving here — which is exactly what my change broke. My fix is correct for `get_since_until()` in isolation, but I only verified its own unit tests, not the other 13 call sites this touches. Better to close than push past a decision that's already been made with more context than I had. Sorry for the noise. -- 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]
