eschutho opened a new pull request, #45521:
URL: https://github.com/apache/superset/pull/45521
### SUMMARY
`POST /api/v1/query/stop` returned a 500 (after ~5 retries) when the request
body did not include `client_id`.
**Root cause**
- `StopQuerySchema.client_id` was declared as `fields.String()` without
`required=True`, so `stop_query_schema.load(request.json)` accepted a body
without `client_id`, and the next line,
`QueryDAO.stop_query(body["client_id"])`, raised `KeyError: 'client_id'`.
- `QueryRestApi.stop_query` is decorated with
`@backoff.on_exception(backoff.constant, Exception, interval=1, max_tries=5)`.
Any exception raised inside the function, including that `KeyError`, is retried
five times, rolling back the session between attempts, before the outer `@safe`
decorator turns it into a 500 and logs an ERROR.
**Fix**
1. Make `StopQuerySchema.client_id` `fields.String(required=True)`, so a
missing or wrongly typed `client_id` raises a marshmallow `ValidationError` at
load time.
2. Catch `ValidationError` in `stop_query` and return
`self.response_400(message=error.messages)`, following the pattern used in
`superset/databases/api.py`. The catch goes inside the function body on
purpose. The backoff decorator catches `Exception`, so a `ValidationError` that
escaped the function would be retried five times like the original `KeyError`.
The OpenAPI docstring already lists a `400` response for this endpoint, so
the documented contract does not change.
### Tradeoffs
- **Behavior change:** a request with a missing or invalid (non-string)
`client_id` returns a **400** with the marshmallow validation messages (e.g.
`{"message": {"client_id": ["Missing data for required field."]}}`). It no
longer retries five times (~seconds of latency plus repeated session rollbacks)
and returns a 500.
- No other semantic change. Valid requests go through the same code path as
before, and the backoff retry still applies to real runtime errors from the DAO
/ DB engine cancel path.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API-only change).
| Request body | Before | After |
| --- | --- | --- |
| `{}` | `KeyError` retried 5x, then 500 | 400, `client_id: Missing data for
required field.` |
| `{"client_id": 123}` | `ValidationError` retried 5x, then 500 | 400,
`client_id: Not a valid string.` |
| `{"client_id": "<valid>"}` | unchanged | unchanged |
### TESTING INSTRUCTIONS
New integration tests in `tests/integration_tests/queries/api_tests.py`:
- `test_stop_query_missing_client_id`: POST `{}` returns 400 with the
required-field message.
- `test_stop_query_invalid_client_id`: POST `{"client_id": 123}` returns 400
with the type message.
Both tests spy on `QueryRestApi.stop_query_schema.load` and assert it is
called **exactly once**, which shows the request was not retried by `backoff`.
They also assert that `QueryDAO.stop_query` is never called.
Results (SQLite metadata DB + Redis, test config
`tests.integration_tests.superset_test_config`):
- `pytest tests/integration_tests/queries/api_tests.py`: **17 passed**,
including the existing `test_stop_query`, `test_stop_query_not_found` and
`test_stop_query_already_finished`.
- With the source changes reverted, both new tests fail with `assert 500 ==
400`, which confirms they reproduce the bug.
- `ruff check`, `ruff format`, and `pre-commit run` are clean on the changed
files.
Manual check:
```bash
curl -X POST http://localhost:8088/api/v1/query/stop \
-H 'Content-Type: application/json' -H "Authorization: Bearer $TOKEN" \
-d '{}'
# -> 400 {"message": {"client_id": ["Missing data for required field."]}}
```
### 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
🤖 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]
