eschutho opened a new pull request, #43517: URL: https://github.com/apache/superset/pull/43517
### SUMMARY Chart data requests that resolve to a reversed date range (`since > until`) currently return an unhandled **HTTP 500** instead of a 400. This is a chronic error in production error tracking: - Sentry: [SUPERSET-PYTHON-NA4](https://preset-inc.sentry.io/issues/5539178946/) — `ValueError: From date cannot be larger than to date` - **71,501 events all-time**, chronic since 2024-06-26, still firing at ~30–40/day - Culprit: `ChartDataRestApi.data` - Shortcut story: [sc-118279](https://app.shortcut.com/preset/story/118279) **Root cause** `superset/utils/date_parser.py::get_since_until()` raises a bare `ValueError(_("From date cannot be larger than to date"))` when a caller supplies a time range where `since > until` (e.g. a user picks a reversed custom date range on a chart). `ChartDataRestApi._create_query_context_from_form()` calls `ChartDataQueryContextSchema().load(form_data)`, which invokes `get_since_until()` transitively through the schema's `make_query_context` `@post_load` hook (`schemas.py` → `query_context_factory.py` → `query_object_factory.py` → `time_range_utils.py` → `date_parser.py`). That helper already converts one lower-level exception for exactly this reason: ```python try: return ChartDataQueryContextSchema().load(form_data) except KeyError as ex: raise ValidationError("Request is incorrect") from ex ``` …but had no equivalent `except ValueError`. All three callers of `_create_query_context_from_form` (`ChartDataRestApi.data`, `.data_from_cache`, and the dashboard-filter-context GET path) catch `marshmallow.ValidationError` and return `response_400`, but **not** `ValueError` — so it propagated unhandled through Flask (`full_dispatch_request`) and surfaced as a 500. This matches the Sentry stack trace: `flask/app.py:full_dispatch_request` → … → `charts/data/api.py in data` → `_create_query_context_from_form` → `marshmallow/schema.py` load/post_load chain → `date_parser.py get_since_until`. **Fix rationale** The fix is a two-line addition at the API boundary that already establishes this precedent — a `ValueError` arm next to the existing `KeyError` arm — re-raising as `marshmallow.ValidationError` so the request returns a **400**: ```python try: return ChartDataQueryContextSchema().load(form_data) except KeyError as ex: raise ValidationError("Request is incorrect") from ex except ValueError as ex: raise ValidationError(str(ex)) from ex ``` The raised type of `get_since_until()` is **deliberately left unchanged**. It is called from several other places (`superset/views/api.py`, `superset/mcp_service/common/time_range_validation.py`) that explicitly catch `ValueError` today; changing its raised type would break those call sites. The correct, minimal fix point is the API boundary that already has this exact conversion pattern. ### Tradeoffs This converts **any** `ValueError` raised anywhere during `ChartDataQueryContextSchema().load()` — not only the date-range one — from an unhandled 500 into a handled 400. This is scoped to constructing a query context from user-submitted form data, so a `ValueError` there is inherently validation-shaped (the same reasoning that already justifies the pre-existing `KeyError` arm). It is nonetheless a failure-mode semantics change and is disclosed here explicitly rather than left implicit: a code defect that happened to raise `ValueError` inside this specific `load()` call would now be reported to the client as a 400 rather than surfacing as a 500. ### BEFORE/AFTER - **Before:** reversed date range → unhandled `ValueError` → HTTP 500 (unhandled server error, noisy in error tracking). - **After:** reversed date range → `marshmallow.ValidationError` → HTTP 400 with the message `From date cannot be larger than to date`. ### TESTING INSTRUCTIONS Added tests: 1. **Unit test** — `tests/unit_tests/charts/test_chart_data_api.py::test_create_query_context_from_form_converts_value_error_to_400`: patches `ChartDataQueryContextSchema.load` to raise `ValueError("From date cannot be larger than to date")` and asserts `_create_query_context_from_form` re-raises it as `marshmallow.ValidationError` carrying that message. 2. **Integration test** — `tests/integration_tests/charts/data/api_tests.py::TestPostChartDataApi::test_with_reversed_time_range__400`: POSTs to `api/v1/chart/data` with `queries[0].time_range = "2024-01-01T00:00:00 : 2020-01-01T00:00:00"` (an explicit ISO `since : until` pair with `since` after `until`) and asserts `status_code == 400`. What was actually run in the dev environment: - `ruff check` and `ruff format --check` on all three changed files — **passed**. - `pre-commit run --files <the 3 files>` — mypy, ruff, ruff-format, docstring/whitespace hooks **passed** (pylint hook was not runnable in this environment: `pylint: command not found`). - Unit test — **passed** (`1 passed`). Verified it **fails against pre-fix code** (via `git stash` of the `api.py` change, the `ValueError` propagates and the test fails), and **passes** with the fix in place. - The integration test's containing class `TestPostChartDataApi` is currently `@pytest.mark.skip`-ped on `master` (unrelated pre-existing TODO about DuckDB example-data format), so it cannot execute yet; it collects cleanly. To exercise the real end-to-end path anyway, I ran a standalone script that builds the app context and calls `ChartDataRestApi()._create_query_context_from_form(...)` with the reversed `time_range` above, exercising the **real** `get_since_until()` → schema `@post_load` chain (not a mock). Result: it raised `marshmallow.ValidationError: ['From date cannot be larger than to date']` — confirming the fix converts the real 500 path into a 400. ### ADDITIONAL INFORMATION - [x] Has associated issue: Fixes SUPERSET-PYTHON-NA4 (sc-118279) - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
