eschutho opened a new pull request, #44986:
URL: https://github.com/apache/superset/pull/44986
### SUMMARY
**Root cause.** `POST /api/v1/chart/data` returns a 500 when a query object
has an `IN` / `NOT IN` filter with no `val` key (or `val: null`). For example:
```json
"filters": [
{"col": "created_at", "op": "TEMPORAL_RANGE", "val": "No filter"},
{"col": "region", "op": "IN"}
]
```
`ExploreMixin.filter_values_handler` returns `None` when `values is None`,
so `eq` is `None`. The IN/NOT IN branch of `ExploreMixin.get_sqla_query` then
hits the bare `assert isinstance(eq, (tuple, list))`. `SqlaTable` inherits both
methods; there are no overrides anywhere in `superset/`, so this is the live
path. The `AssertionError` is not a `QueryObjectValidationError`, so the
chart-data error handling does not catch it and the request fails with a 500.
**Fix.** The bare assert is replaced with a `QueryObjectValidationError`.
Its translated message names the column: `Filter value is required for the IN /
NOT IN operator on column region`. The filter is rejected, not silently
dropped, because dropping it would widen the result set. Behaviour for valid
filters does not change: a list value still reaches the existing empty-list
check, the mixed int/float normalisation and the NULL-handling paths exactly as
before.
**Sibling asserts / branches reviewed.** None of these hit the same bug, so
they are unchanged:
- The comparison operators (`>`, `<`, `>=`, `<=`), `LIKE` / `ILIKE` / `NOT
LIKE` / `NOT ILIKE` and `CONTAINS_ANY` / `CONTAINS_ALL` share the `else`
branch. That branch already raises `QueryObjectValidationError("Must specify a
value for filters with comparison operators")` when `eq is None`.
- The `LENGTH_*` operators already raise when `cast_to_num(eq)` is `None`.
- The only other `assert` in `get_sqla_query` (`assert isinstance(metric,
dict)`) is unrelated to filter values.
### Tradeoffs
- This changes how the failure surfaces: an unhandled `AssertionError` (a
500) becomes a typed `QueryObjectValidationError`.
- **Main query path:** `QueryContextProcessor.get_df_payload` catches
`QueryObjectValidationError` and returns it as a cached error, so the client
gets a 400 with the message above.
- **`ensure_totals_available` path** (the path in the observed stack trace):
this pre-query runs `get_query_result` without catching
`QueryObjectValidationError`. The exception propagates to
`ChartDataRestApi._get_data_response`. **This PR alone does not turn that path
into a 400.** It still depends on the complementary open PR #43081
(`fix(chart/data): handle QueryObjectValidationError in _get_data_response`).
After both PRs, that path also returns a 400 with a clear message. Until then,
this PR still replaces an opaque `AssertionError` with a typed, descriptive
exception.
- Clients that previously "worked around" the 500 will see a 4xx instead. No
client could have relied on a successful response here, because the request
always failed.
### Follow-ups
- Land #43081 so `QueryObjectValidationError` raised from the totals
pre-query maps to a 400.
- Find the client that sends an `IN` filter without `val`. The frontend
should not emit one; dropping or completing incomplete filters there would
avoid the error entirely.
- On multi-value (array) columns, the whole-array `IN` / `NOT IN` branch
runs before this one. With no value it builds `col IN (ARRAY[NULL])` instead of
erroring. It does not crash, so it is out of scope here, but it may deserve the
same validation.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only).
### TESTING INSTRUCTIONS
New unit tests in `tests/unit_tests/models/helpers_test.py`:
- `test_get_sqla_query_in_filter_without_value_raises_validation_error`:
parametrized over IN with no `val`, NOT IN with no `val`, IN with `val=None`
and NOT IN with `val=None`. Each case asserts a `QueryObjectValidationError`
that mentions the column. Before the fix, all four failed with `AssertionError`
at `superset/models/helpers.py`.
- `test_get_sqla_query_in_filter_with_value_builds_query`: regression test
for IN and NOT IN. A valid list still compiles to `region IN ('EMEA', 'APAC')`
/ `region NOT IN (...)`.
```
pytest tests/unit_tests/models/helpers_test.py -k in_filter
# before fix: 4 failed (AssertionError), 4 passed
# after fix: 8 passed
```
Full `tests/unit_tests/models/helpers_test.py` and
`tests/unit_tests/charts/data/malformed_adhoc_metric_test.py`: 190 passed, 2
skipped, and 1 failure. That failure
(`test_build_like_predicate_is_case_insensitive_and_escaped`) is a local
missing-module import error. It fails identically on `master`, so this change
did not cause it.
`pre-commit run` on the changed files (ruff, ruff-format, mypy, pylint): all
passed.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] 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
Related: #43081 (complementary; handles `QueryObjectValidationError` in
`_get_data_response`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
