-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Return better messages for JSON errors in API. #18367
Return better messages for JSON errors in API. #18367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I have some comments.
The new error message no longer includes any type information about which class the issue is about.
Example:
Cannot construct instance of `org.graylog.plugins.views.search.searchtypes.events.EventList$Builder`
That information doesn't seem to be part of the response anymore. I was using the information as a starting point for debugging.
Can we bring that information back?
changelog/unreleased/pr-18367.toml
Outdated
@@ -0,0 +1,4 @@ | |||
type = "a" | |||
message = "Improving API error responses for JSON parsing failures.." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double full stop.
message = "Improving API error responses for JSON parsing failures.." | |
message = "Improving API error responses for JSON parsing failures." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
graylog2-server/src/main/java/org/graylog2/plugin/rest/RequestError.java
Show resolved
Hide resolved
...rver/src/main/java/org/graylog2/shared/rest/exceptionmappers/JsonMappingExceptionMapper.java
Show resolved
Hide resolved
changelog/unreleased/pr-18367.toml
Outdated
@@ -0,0 +1,4 @@ | |||
type = "a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it a c
because it's a change in the API error responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
…sponses-for-json-errors
…erties in a structured way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
* Return better messages for JSON errors in API. * Improving structure once again. * Include more attributes in specialized error class. * Adding license header. * Adding changelog snippet. * Handling missing target type. * Including reference path if present. * Fixing up changelog, adding upgrade notes. * Adding test for JSON parsing error responses. * Constructing reference path for missing required property. * Handling JSON parsing error on root level. * Consolidating `JacksonPropertyExceptionMapper` to handle invalid properties in a structured way. * Adjusting test.
Description
Motivation and Context
This PR is aiming to improve error messages returned by the API in case of JSON mapping errors. Previously, error messages caused by deserialization failures were of little help, because a) they were using Java fqcn's and b) they were too noisy. This PR is changing this to return a) the location of the error in the JSON input and b) the path to the property that failed to deserialize in a structured and concise way.
How Has This Been Tested?
Screenshots (if appropriate):
For unexpected type:
Before:
After:
For generic JSON parsing (superfluous
,
):Before:
After:
Types of changes
Checklist: