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

JSON parse failures should be 4xx codes #112703

Merged
merged 10 commits into from
Sep 11, 2024

Conversation

benwtrent
Copy link
Member

It seemed if there wasn't any text to parse, this is not an internal issue but instead an argument issue.

I simply changed the exception thrown. If we don't agree with this, I can adjust query parsing directly, but this seemed like the better choice.

closes: #112296

@benwtrent benwtrent added >bug :Core/Infra/Core Core issues without another label :Search/Search Search-related issues that do not fall into other categories test-full-bwc Trigger full BWC version matrix tests v8.16.0 labels Sep 10, 2024
@benwtrent benwtrent requested a review from a team as a code owner September 10, 2024 12:53
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team Team:Search Meta label for search team labels Sep 10, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @benwtrent, I've created a changelog YAML for you.

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

We discussed with @cbuescher whether IllegalArgumentException was a good choice on #112605. I'm fine with either IllegalArgumentException or ValidationException, but I think that whatever we choose it's better to be consistent. Wdyt?

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.

Looks good, I have one suggestion to make the error message easier to understand

@benwtrent
Copy link
Member Author

@ldematte @cbuescher I read the discussion, IAE or an xcontent parsing exception makes sense to me here as we are parsing values input values.

The PR linked has to do with the content type not even being known. That to me is a horse of a different color. While, so, I would prefer an IAE there as well as the input is the REST request we are handling and thus an illegal argument was given, the poorly formed request.

@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent added v8.15.2 auto-backport-and-merge auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Sep 11, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @benwtrent, I've updated the changelog YAML for you.

@elasticsearchmachine elasticsearchmachine merged commit 281ee04 into elastic:main Sep 11, 2024
15 checks passed
@benwtrent benwtrent deleted the json-failures-4xx branch September 11, 2024 14:16
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Sep 11, 2024
It seemed if there wasn't any text to parse, this is not an internal
issue but instead an argument issue.

I simply changed the exception thrown. If we don't agree with this, I
can adjust `query` parsing directly, but this seemed like the better
choice.

closes: elastic#112296
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.15

benwtrent added a commit that referenced this pull request Sep 11, 2024
It seemed if there wasn't any text to parse, this is not an internal
issue but instead an argument issue.

I simply changed the exception thrown. If we don't agree with this, I
can adjust `query` parsing directly, but this seemed like the better
choice.

closes: #112296

Co-authored-by: Elastic Machine <[email protected]>
davidkyle pushed a commit that referenced this pull request Sep 12, 2024
It seemed if there wasn't any text to parse, this is not an internal
issue but instead an argument issue.

I simply changed the exception thrown. If we don't agree with this, I
can adjust `query` parsing directly, but this seemed like the better
choice.

closes: #112296
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >bug :Core/Infra/Core Core issues without another label :Search/Search Search-related issues that do not fall into other categories Team:Core/Infra Meta label for core/infra team Team:Search Meta label for search team test-full-bwc Trigger full BWC version matrix tests v8.15.2 v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IllegalStateExceptions caused by JsonParseExceptions should return 4xx status codes
5 participants