Skip to content

Fix: Correctly show error message in DQL and PPL query editor #9589

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ruchidh
Copy link
Contributor

@ruchidh ruchidh commented Mar 21, 2025

Description

This PR fix a bug that causing error message in query editor to show Cannot read properties of undefined (reading 'hasOwnProperty') and message[object object] instead of the correct error message and also added a toaster at bottom for detailed error message.

There are 3 major changes in this PR:

Correctly throw error from fetch result
Properly handle error message base on current error structure
Add toaster for query editor dispaly error

Reference from pr by @Maosaic - #9586

Issues Resolved

Fix a issue that cause error message in query editor to show Cannot read properties of undefined (reading 'hasOwnProperty')
Fix message [object object] issue

Screenshot

Before
image

Screenshot 2025-03-21 at 1 45 22 PM

After
image

Screenshot 2025-03-21 at 3 45 00 PM

Changelog

  • fix: Correctly show error message in DQL and PPL query editor
  • feat: Add toaster for query errors

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@ruchidh ruchidh changed the title Fix error handling in discover query editor Fix: Correctly show error message in DQL and PPL query editor Mar 21, 2025
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Ruchi Sharma <[email protected]>
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

2 similar comments
Copy link
Contributor

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

Copy link
Contributor

❌ Changelog Entry Missing Hyphen

Changelog entries must begin with a hyphen (-).

Copy link

codecov bot commented Mar 22, 2025

Codecov Report

Attention: Patch coverage is 41.37931% with 17 lines in your changes missing coverage. Please review.

Project coverage is 61.79%. Comparing base (411e108) to head (f888102).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...ic/application/view_components/utils/use_search.ts 0.00% 8 Missing ⚠️
...query_string/language_service/lib/query_result.tsx 60.00% 2 Missing and 2 partials ⚠️
src/plugins/query_enhancements/common/utils.ts 66.66% 2 Missing and 1 partial ⚠️
...lic/application/view_components/canvas/top_nav.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #9589    +/-   ##
========================================
  Coverage   61.78%   61.79%            
========================================
  Files        3807     3797    -10     
  Lines       91933    91777   -156     
  Branches    14597    14570    -27     
========================================
- Hits        56802    56713    -89     
+ Misses      31452    31440    -12     
+ Partials     3679     3624    -55     
Flag Coverage Δ
Linux_1 28.89% <0.00%> (+0.04%) ⬆️
Linux_2 ?
Linux_3 39.32% <30.00%> (-0.01%) ⬇️
Linux_4 28.81% <20.68%> (+0.05%) ⬆️
Windows_1 28.90% <0.00%> (+0.04%) ⬆️
Windows_2 56.26% <ø> (ø)
Windows_3 39.32% <30.00%> (-0.01%) ⬇️
Windows_4 28.81% <20.68%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@@ -329,6 +350,19 @@ export const useSearch = (services: DiscoverViewServices) => {
}
}

// Currently error message is sent as encoded JSON string, which requires extra parsing
// TODO: Confirm error contract
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You can create an issue to track the "Todo: Confirm error contract" and mention it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood , thank you

LDrago27
LDrago27 previously approved these changes Mar 22, 2025
Comment on lines +47 to +61
let errorMessage = response.data.body?.message ?? response.data.body ?? response.data;

// Check if errorMessage is an object and handle Error objects
if (typeof errorMessage === 'object') {
if (errorMessage instanceof Error) {
// If errorMessage is an instance of Error, extract its message
errorMessage = errorMessage.message;
} else if (errorMessage.message) {
// If errorMessage has a message property, extract that message
errorMessage = JSON.stringify(errorMessage.message);
} else {
// If errorMessage is a plain object, stringify it
errorMessage = JSON.stringify(errorMessage);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but looks like there's some more digging we can do to figure out how we can have some sort of response interface here. Right now it looks we're in the wild west 🤠

Copy link
Collaborator

@angle943 angle943 left a comment

Choose a reason for hiding this comment

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

could we add some unit tests?

@Maosaic
Copy link
Collaborator

Maosaic commented Mar 31, 2025

can you remove the code changes included in #9586 ? I have made some change on that PR, to avoid conflicts and keep thing clean, I think we should remove the duplicate change here.

Signed-off-by: Ruchi Sharma <[email protected]>
@ruchidh ruchidh requested a review from wanglam as a code owner April 2, 2025 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants