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

Added tests for details of HttpMessageNotReadableException #65

Merged
merged 3 commits into from
Sep 13, 2016

Conversation

whiskeysierra
Copy link
Collaborator

@whiskeysierra whiskeysierra commented Sep 12, 2016

Fixes #1
Fixes #56

Willi Schönborn added 2 commits September 12, 2016 23:37
This removes the burden of implementations to declare it as a checked exception in their
throws clause.

Fixes #56
@lukasniemeier-zalando
Copy link
Member

regarding #56 👍
regarding #1: 8 month ago the idea was to expose further fields on the problem including structured info about the location or path of the error (if applicable). If we want to back up from this idea 👍

@@ -26,6 +26,32 @@ public void missingRequestBody() throws Exception {
.andExpect(jsonPath("$.detail", containsString("request body is missing")));
}

// TODO: jackson stuff tests
Copy link
Member

@lukasniemeier-zalando lukasniemeier-zalando Sep 13, 2016

Choose a reason for hiding this comment

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

other common exception causes:

  • InvalidFormatException (e.g. have BigDecimal field but foobar on the wire)
  • JsonMappingException with no further cause due to No suitable constructor found

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the first one is a

Specialized sub-class of {@link JsonMappingException}

and Throwables.getRootCause will return this if no cause was found. I think we should be safe here. I'll add two tests though.

Copy link
Collaborator Author

@whiskeysierra whiskeysierra Sep 13, 2016

Choose a reason for hiding this comment

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

and Throwables.getRootCause will return

Which I'm not using anymore in the current version... 😄

@whiskeysierra
Copy link
Collaborator Author

I looked into the location/path but it will only contain what's part of the message anyway. Adding a new property would in theory require to register a new problem type. Feels like overkill to me, especially since nobody should handle that problem programmatically. It's almost certainly a bug.

@lukasniemeier-zalando
Copy link
Member

👍

@whiskeysierra whiskeysierra merged commit cafebb3 into master Sep 13, 2016
@whiskeysierra whiskeysierra deleted the feature/json-error branch September 13, 2016 07:27
@kmandeville
Copy link

Were there no changes to help improve this issue? I have the same problem.

@whiskeysierra
Copy link
Collaborator Author

@kmandeville We cross-linked a couple of issues here. Can you be more specific?

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.

3 participants