SEPURI-SAI-KRISHNA opened a new pull request, #44953:
URL: https://github.com/apache/superset/pull/44953
### SUMMARY
A malformed `post_processing` option returned HTTP 500 where the contract
pinned by `test_chart_data_invalid_post_processing` is 400. Fuzzing found 39
distinct sites that escape to `app.errorhandler(Exception)`; this PR converts
the 38 that are bad requests and leaves the 1 that is genuinely a server-side
condition. Background and the full measurement in #44952.
#44463 widened the guard at `superset/models/helpers.py` so raw pandas
`TypeError` and `pandas.errors.DataError` become `QueryObjectValidationError`,
and #44502 did the same for `superset/semantic_layers/models.py`. `ValueError`,
`KeyError` and `AttributeError` escape the same way: not caught there, not
caught by `get_df_payload` (which handles only `QueryObjectValidationError`),
and `ChartDataRestApi.data` carries no `@safe`, so they land in
`app.errorhandler(Exception)` whose `json_error_response` defaults to
`status=500`.
This PR handles those three at both call sites, following #44463's pattern:
```python
except InvalidPostProcessingError as ex:
raise QueryObjectValidationError(ex.message) from ex
-except (TypeError, pd.errors.DataError) as ex:
+except KeyError as ex:
+ raise QueryObjectValidationError(
+ _(
+ "Post-processing references a column or level that is "
+ "not in the query result: %(name)s",
+ name=ex.args[0] if ex.args else ex,
+ )
+ ) from ex
+except (
+ TypeError,
+ ValueError,
+ AttributeError,
+ pd.errors.DataError,
+) as ex:
+ logger.warning(
+ "Post-processing failed and was reported as a bad request",
+ exc_info=True,
+ )
raise QueryObjectValidationError(str(ex)) from ex
```
`KeyError` gets its own arm because `str(KeyError("x"))` is just `"'x'"`, so
reusing the shared message would answer a request with `Error:
'does_not_exist'`. All ten `KeyError` sites name a column or MultiIndex level —
`boxplot(metrics)`, `rank(metric)`, `rank(group_by)`, `histogram(column)`,
`histogram(groupby)`, `flatten(drop_levels)`, `geohash_decode(geohash)`,
`geohash_encode(latitude)`, `geohash_encode(longitude)`,
`geodetic_parse(geodetic)` — so a specific message is accurate for every one.
Measured end to end:
| options | before | after |
|---|---|---|
| `rank` `metric: "does_not_exist"` | HTTP 500 | `400 Error: Post-processing
references a column or level that is not in the query result: does_not_exist` |
| `diff` `periods: 1.5` | HTTP 500 | `400 Error: periods must be an integer`
|
No operator internals are modified and no option is newly rejected — only
the status and message of a request that already failed.
**`ImportError` is deliberately left out.** The one case is `rolling` with a
`win_type`, which needs scipy; a missing optional dependency is a deployment
matter rather than a bad request, so 500 remains correct there. That is the
single site this PR does not convert, and it is called out in a comment.
**Why these values reach pandas at all:**
`ChartDataPostProcessingOperationSchema.options` is a bare `fields.Dict`
(schemas.py:1123), so no option is validated before dispatch. The 11
`ChartData*OptionsSchema` classes are referenced only from `CHART_SCHEMAS` for
OpenAPI output and validate nothing, and 8 operations have no options schema at
all. Wiring those up would be the root-cause fix, but it risks rejecting
payloads that work today, so it is left out of this PR; the `except` boundary
is where #44463 already set the precedent.
**The trade-off, stated plainly.** `ValueError` and `AttributeError` are
broad: an `AttributeError` reading `'NoneType' object has no attribute ...` is
the classic signature of a genuine bug, and under this change it answers 400
rather than 500. Two things keep that acceptable, and I would rather reviewers
weigh them explicitly than discover them later:
- The pre-existing handler already makes exactly this trade for `TypeError`,
which is no narrower. This PR extends an established boundary rather than
introducing a new one.
- The masking risk is that `get_df_payload` records `str(ex)` on the
response and never logs a traceback, so a mis-classified internal fault would
vanish silently. The handler therefore logs with `exc_info=True` before
re-raising, so the stack trace still reaches operators. The `KeyError` arm does
not log: a missing column is unambiguous and would only add noise.
If reviewers would rather keep `AttributeError` as a 500, dropping it from
the tuple is a one-line change and costs 7 of the 38 converted sites.
**Translations.** The `KeyError` arm adds one new translatable string, so
`superset/translations/messages.pot` is regenerated to match (`+4/-0`, the
entry plus its `python-format` flag). Without that the required `babel-extract`
check fails on `scripts/translations/check_pot_drift.py`, and the string would
be untranslatable in every language. No `.po` catalogue is touched, matching
how other string-adding PRs land.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A: backend only. The error response for an already-failing request changes
from an opaque 500 to a 400 carrying the underlying message.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/models/helpers_test.py
tests/unit_tests/semantic_layers/models_test.py
```
Seven cases are added, parametrized over the three exception types at both
call sites (the `KeyError` cases assert the specific message, not just the key):
-
`helpers_test.py::test_get_query_result_wraps_post_processing_request_errors` —
four cases driving **real** operations through `SqlaTable.get_query_result`:
`diff` with `periods: 1.5` (ValueError from pandas), `boxplot` with
`percentiles: [10, 200]` (ValueError from numpy), `rank` with an unknown metric
(KeyError), and `aggregate` with a string `aggregates` (AttributeError).
-
`models_test.py::test_semantic_view_get_query_result_wraps_post_processing_request_errors`
— three cases for the semantic-layer call site.
All seven **fail on master** with the raw exception propagating, and pass
with this change. No existing test changed behaviour:
`tests/unit_tests/{pandas_postprocessing,models,semantic_layers,common,charts}`
run clean at 2197 passed, 2 xfailed (both pre-existing). `ruff check`, `ruff
format --check` and `mypy` are clean, and `pylint` rates both changed source
files 10.00/10.
To size the change I fuzzed every operation's options one at a time away
from a known-good baseline — 910 calls across all 19 operations — classifying
each failure by whether the guard catches it. Before: **39 distinct
unhandled-exception sites** across 14 of the 19 operations (ValueError 21,
KeyError 10, AttributeError 7, ImportError 1). After: **1**, the scipy
`ImportError` above. That sweep is not exhaustive over every possible option
value, so it is a lower bound on what this fixes rather than a complete census.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #44952
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
### Note on the duplicated guard
The block is now identical in `superset/models/helpers.py` and
`superset/semantic_layers/models.py`. That duplication is why the two sites
were fixed eleven days apart (#44502, then #44463), and #44463's description
flags the second as a follow-up. I have kept both inline here to match the
existing style and keep the diff reviewable, rather than extracting a shared
constant — `superset/exceptions.py` is the only module both import at runtime
and it deliberately avoids a pandas import. Happy to consolidate if reviewers
prefer.
--
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]
