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.

Sign up for GitHub

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

Jump to bottom

Wrap jackson exception on malformed json string #114445

Merged
merged 14 commits into from
Dec 5, 2024
Merged

Wrap jackson exception on malformed json string #114445

merged 14 commits into from
Dec 5, 2024

Conversation

henriquepaes1
Copy link
Contributor

This PR closes #114142, which observed a 500 response when sending a malformed query to the _search endpoint. I identified that the problem was due to an unchecked exception thrown by the Jackson JSON library when using the .getText( ) method. The solution was to catch the JsonParseException and throw an IllegalArgumentException that gives better information on where the error happened. Now the response looks like the image below:

image

@henriquepaes1 henriquepaes1 requested a review from a team as a code owner October 9, 2024 18:56
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Oct 9, 2024
@AI-IshanBhatt AI-IshanBhatt added the :Search Foundations/Search Catch all for Search Foundations label Oct 10, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed needs:triage Requires assignment of a team area label labels Oct 10, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@rjernst
Copy link
Member

rjernst commented Oct 11, 2024

Thanks for the change @henriquepaes1. Can you please adhere to the formatting style used by the rest of the code? Spaces between braces/parens and catch on same line as braces.

It would also be nice to have a test. It could go in JsonXContentTests

@rjernst rjernst added :Core/Infra/Core Core issues without another label and removed :Search Foundations/Search Catch all for Search Foundations labels Oct 11, 2024
@rjernst rjernst self-assigned this Oct 11, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team and removed Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Oct 11, 2024
@rjernst rjernst changed the title Return Bad Request with malformed _search Wrap jackson exception on malformed json string Oct 11, 2024
@rjernst rjernst added the >bug label Oct 11, 2024
@henriquepaes1
Copy link
Contributor Author

Hey @rjernst! Thanks for pointing these out! I'm struggling with the test. Should I write the test for the JsonXContent class or for the AbstractQueryBuilder, where the exception was observed in the issue? Also, should I squash the commits when I finish?

@rjernst
Copy link
Member

rjernst commented Oct 11, 2024

Should I write the test for the JsonXContent class or for the AbstractQueryBuilder

You could add the test to BaseXContentTestCase, which will be run for all xcontent types.

should I squash the commits when I finish?

Don't squash. I suggest you read https://github.com/elastic/elasticsearch/blob/main/CONTRIBUTING.md.

@henriquepaes1
Copy link
Contributor Author

Hey @rjernst! Are there any other things that I should correct in this PR?

@rjernst
Copy link
Member

rjernst commented Oct 24, 2024

@elasticsearchmachine ok to test

@rjernst
Copy link
Member

rjernst commented Oct 24, 2024

@elasticmachine test this please

@rjernst
Copy link
Member

rjernst commented Oct 24, 2024

@elasticsearchmachine update branch

@rjernst
Copy link
Member

rjernst commented Oct 24, 2024

@elasticmachine update branch

@rjernst
Copy link
Member

rjernst commented Oct 24, 2024

@elasticmachine test this please

@prdoyle
Copy link
Contributor

prdoyle commented Nov 7, 2024 •
edited
Loading

@henriquepaes1 at first glance, it looks like a failure unrelated to your change, so I've merged main and kicked off the tests again, hoping for an upstream fix. 🤞

@prdoyle
Copy link
Contributor

prdoyle commented Nov 7, 2024

@elasticmachine test this please

@henriquepaes1
Copy link
Contributor Author

at first glance, it looks like a failure unrelated to your change, so I've merged main and kicked off the tests again, hoping for an upstream fix. 🤞

Sure @prdoyle! Thanks for the heads up. Let me know if I need to fix something :)

@prdoyle prdoyle self-assigned this Nov 7, 2024
@henriquepaes1 @prdoyle
Update parser as suggested
1f69f3f 
Co-authored-by: Patrick Doyle <810052+prdoyle@users.noreply.github.com>
@prdoyle
Copy link
Contributor

prdoyle commented Nov 8, 2024

Don't squash. I suggest you read https://github.com/elastic/elasticsearch/blob/main/CONTRIBUTING.md.

@rjernst - FWIW that linked document says this:

As a final step before merging we will either ask you to squash all commits yourself or we'll do it for you.

To be clear @henriquepaes1 - we'll do it for you. 😄

@prdoyle
Copy link
Contributor

prdoyle commented Nov 8, 2024

@henriquepaes1 - I'd like to check with Ryan one more time before proceeding to merge this. That means it will have to wait till next week.

@henriquepaes1
Copy link
Contributor Author

I'd like to check with Ryan one more time before proceeding to merge this. That means it will have to wait till next week.

No problems! Thanks for the clarification and for the help in getting this PR going :)

@prdoyle
Copy link
Contributor

prdoyle commented Nov 8, 2024

@elasticmachine test this please

@rjernst
Copy link
Member

rjernst commented Nov 22, 2024

@elasticmachine update branch

@benwtrent
Copy link
Member

@elasticmachine update branch

@benwtrent
Copy link
Member

@elasticmachine test this please

@prdoyle prdoyle removed their assignment Dec 5, 2024
rjernst
rjernst approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Thanks @henriquepaes1 , LGTM

@rjernst rjernst added v8.18.0 auto-backport Automatically create backport pull requests when merged labels Dec 5, 2024
@rjernst rjernst merged commit 4740b02 into elastic:main Dec 5, 2024
17 checks passed
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Dec 5, 2024
@henriquepaes1 @rjernst
Wrap jackson exception on malformed json string (elastic#114445)
512c60e 
This commit hides the underlying Jackson parse exception when encountered while parsing string tokens.
@rjernst rjernst mentioned this pull request Dec 5, 2024
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.x

@benwtrent
Copy link
Member

🎉🎉🎉 @henriquepaes1 🎉🎉🎉

🎉🎉🎉 Thank you!!!!! 🎉🎉🎉

elasticsearchmachine pushed a commit that referenced this pull request Dec 5, 2024
@rjernst @henriquepaes1
Wrap jackson exception on malformed json string (#114445) (#118098)
51f1bc0 
This commit hides the underlying Jackson parse exception when encountered while parsing string tokens.

Co-authored-by: Henrique Paes <henriquee.paes1@gmail.com>
@henriquepaes1
Copy link
Contributor Author

Thanks for the guidance @benwtrent @rjernst! I'm really glad to contribute, hopefully it was the first of many!

@henriquepaes1 henriquepaes1 deleted the 114142-json-exception-handling branch December 5, 2024 19:39
@drempapis drempapis mentioned this pull request Dec 6, 2024
Closed
@Tim-Brooks Tim-Brooks mentioned this pull request Dec 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prdoyle prdoyle prdoyle approved these changes

@rjernst rjernst rjernst approved these changes

Assignees

@rjernst rjernst

Labels
auto-backport Automatically create backport pull requests when merged >bug :Core/Infra/Core Core issues without another label external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Core/Infra Meta label for core/infra team v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_search payload resulting in JsonParseException
7 participants
@henriquepaes1 @elasticsearchmachine @rjernst @prdoyle @benwtrent @AI-IshanBhatt @elasticmachine