fix(querying): Use appropriate HTTP status for async queries

[Twixes]

Problem

In the experimental async queries system, we were always returning a 200 even if things went wrong, and we were unsafely always returning the error when things went wrong. Related to ZEN-12040

Changes

We now use HTTP status codes appropriately, and are more deliberate with errors.

How did you test this code?

Updated API tests.

[github-actions]

Size Change: 0 B

Total Size: 824 kB

ℹ️ View Unchanged

Filename	Size
frontend/dist/toolbar.js	824 kB

_{compressed-size-action}

[posthog-bot]

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

• chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
• webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

[posthog-bot]

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

• chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
• webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

[webjunkie]

Reviewing after merge:

Mhm... the reasoning here was that the GET/retrieve call mostly successfully retrieves the status of the async query.

Imagine as frontend, you are querying the status, get 200 means you got the status, then checking whether the query erred is on a different level. Returning a bad request or server errors means we cannot distinguish from the "get status" failing vs. the actual query having failed.

Was there no way to look into the error inside the status instead?

[Twixes]

I agree what you're describing definitely makes sense as one way of handling status @webjunkie Here the immediate issue is that all of our pre-existing API request code has handling for HTTP errors, so by reusing that, we can avoid that new handling to also check the query status body.
HTTP status codes do also make for a standard obvious experience – I'd say 200 vs. 202 is a must-have – though I agree 400 and 500 are one area that's a bit gray. They do refer to the request itself more than the resource… Would leave them as is for the existing error handling though.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ TypeError: Property.init() missing 1 required positional argument: 'key' posthog.tasks.tasks.process_query_task View Issue
• ‼️ CHQueryErrorNoCommonType: Code: 386. posthog.tasks.tasks.process_query_task View Issue
• ‼️ TypeError: Property.init() missing 1 required positional argument: 'key' posthog.tasks.tasks.process_query_task View Issue
• ‼️ CHQueryErrorUnknownIdentifier: Code: 47. posthog.tasks.tasks.process_query_task View Issue
• ‼️ CHQueryErrorNoCommonType: Code: 386. posthog.tasks.tasks.process_query_task View Issue

_{Did you find this useful? React with a 👍 or 👎}