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

Entities that can't be serialized now return a 500 #1347

Merged
merged 1 commit into from Nov 20, 2015

Conversation

Projects
None yet
4 participants
@nickbabcock
Contributor

nickbabcock commented Nov 20, 2015

Jackson is good about serializing types that conform to the Bean spec, but
needs help serializing and deserializing types that don't (normally with
the use of annotations). Previous behavior would return a 400 (Bad
Request) when the server would attempt to return an entity that couldn't
be serialized. This is misleading because it gives the illusion that the
fault lies with the client instead of the server.

Entities that can't be serialized return a 500
Jackson is good about serializing types that conform to the Bean spec, but
needs help serializing and deserializing types that don't (normally with
the use of annotations). Previous behavior would return a 400 (Bad
Request) when the server would attempt to return an entity that couldn't
be serialized. This is misleading because it gives the illusion that the
fault lies with the client instead of the server.

@jplock jplock added this to the 1.0.0 milestone Nov 20, 2015

@jplock jplock added the improvement label Nov 20, 2015

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Nov 20, 2015

Member

I think this is a good idea and I don't know if someone would be relying on this or whatnot, so it should be fine to break BC in 1.0.0

Member

jplock commented Nov 20, 2015

I think this is a good idea and I don't know if someone would be relying on this or whatnot, so it should be fine to break BC in 1.0.0

jplock added a commit that referenced this pull request Nov 20, 2015

Merge pull request #1347 from nickbabcock/outbound-json
Entities that can't be serialized now return a 500

@jplock jplock merged commit 304e48b into dropwizard:master Nov 20, 2015

@brentryan

This comment has been minimized.

Show comment
Hide comment
@brentryan

brentryan Nov 21, 2015

I'm not sure I agree entirely here. The error could still be the clients issue, right? If they send in bad data then a 400 is appropriate.

brentryan commented Nov 21, 2015

I'm not sure I agree entirely here. The error could still be the clients issue, right? If they send in bad data then a 400 is appropriate.

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Nov 21, 2015

Contributor

That should be covered by the returnsA400ForNonDeserializableRequestEntities test, but if you can find an edge case, I'd be happy to re-evaluate my implementation.

Contributor

nickbabcock commented Nov 21, 2015

That should be covered by the returnsA400ForNonDeserializableRequestEntities test, but if you can find an edge case, I'd be happy to re-evaluate my implementation.

@brentryan

This comment has been minimized.

Show comment
Hide comment
@brentryan

brentryan Nov 22, 2015

Na, I just read the conversation above and not the code. It looks good to me and passed my test locally. Thanks!

brentryan commented Nov 22, 2015

Na, I just read the conversation above and not the code. It looks good to me and passed my test locally. Thanks!

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Nov 23, 2015

Contributor

No problem, great to have additional eyes for these topics!

Contributor

nickbabcock commented Nov 23, 2015

No problem, great to have additional eyes for these topics!

@mikewatt

This comment has been minimized.

Show comment
Hide comment
@mikewatt

mikewatt Apr 19, 2016

I've come come across a scenario where this causes a 500, which should probably be a 4xx. I believe it is related to this change, although I'm not sure where the appropriate fix is.

The JSR310 module re-throws parser exceptions as JsonMappingExceptions (https://github.com/FasterXML/jackson-datatype-jsr310/blob/7eccc43bbaac9e742f80b3897256563e28812345/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/JSR310DeserializerBase.java#L60), so if the client request body contains a value that is not parseable into the necessary instance type, a 500 is generated.

Is this fix to handle this in the exception mapper, or should the JSR310 module throw a different exception, or something else entirely?

mikewatt commented Apr 19, 2016

I've come come across a scenario where this causes a 500, which should probably be a 4xx. I believe it is related to this change, although I'm not sure where the appropriate fix is.

The JSR310 module re-throws parser exceptions as JsonMappingExceptions (https://github.com/FasterXML/jackson-datatype-jsr310/blob/7eccc43bbaac9e742f80b3897256563e28812345/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/JSR310DeserializerBase.java#L60), so if the client request body contains a value that is not parseable into the necessary instance type, a 500 is generated.

Is this fix to handle this in the exception mapper, or should the JSR310 module throw a different exception, or something else entirely?

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Apr 19, 2016

Contributor

Thank you for the report mike. I can confirm that this is undesirable behavior and will have a fix out shortly.

Contributor

nickbabcock commented Apr 19, 2016

Thank you for the report mike. I can confirm that this is undesirable behavior and will have a fix out shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment