Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: async validation errors #22351

Merged
merged 2 commits into from
May 21, 2024
Merged

fix: async validation errors #22351

merged 2 commits into from
May 21, 2024

Conversation

aspicer
Copy link
Contributor

@aspicer aspicer commented May 18, 2024

Problem

Async user surfaceable queries don't have user friendly errors
#21270

Changes

Screenshot 2024-05-18 at 12 34 28 PM

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Tested locally

@aspicer aspicer requested a review from a team May 18, 2024 09:37
Copy link
Contributor

github-actions bot commented May 18, 2024

Size Change: +172 B (+0.02%)

Total Size: 1.05 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.05 MB +172 B (+0.02%)

compressed-size-action

@@ -329,6 +329,10 @@ export const insightVizDataLogic = kea<insightVizDataLogicType>([
validationError: [
(s) => [s.insightDataError],
(insightDataError): string | null => {
// Handle Async Queries here
if (insightDataError?.data?.error_message) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question: does this only apply to async HogQL queries, or HogQL queries overall? Because if the shape of insightDataError is different for async ones specifically, that sounds like a minefield

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InsightDataError is different for sync and async. See below:

Sync
image

Async
image

The "status" field does work correctly, but the error message is in a different place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so they're pretty significantly different. That's a bit annyoing. But let's maybe keep as is for now, and only fix the problem at hand.

@aspicer aspicer merged commit 0999920 into master May 21, 2024
83 checks passed
@aspicer aspicer deleted the aspicer/async_validation_errors branch May 21, 2024 13:56
timgl pushed a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants