Skip to content

Allows Jackson2 encoders to log Throwable reason for not being able to serialize or deserialize #25892

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

Closed
wants to merge 1 commit into from

Conversation

Anusien
Copy link

@Anusien Anusien commented Oct 9, 2020

I have a @RestController @RequestBody that Jackson should be able
to deserialize, but it can't because I made an error in my
serialization setup. All I get in my application's logs is

org.springframework.web.server.UnsupportedMediaTypeStatusException:
415 UNSUPPORTED_MEDIA_TYPE "Content type 'application/json' not
supported for bodyType=CLASS_NAME"

Meanwhile there's a very helpful IllegalArgumentException from within
Jackson that gets swallowed. Jackson actually propagates the exception
all the way up to ObjectMapper#canDeserialize(JavaType), where it
sees that you haven't asked for that exception and swallows it. If we
instead called
ObjectMapper#canSerialize(Class<T>, AtomicReference<Throwable>),
the consumer could do something with the exception. And the same is
true of ObjectMapper#canDeserialize.

This changes Jackson2Encoder and Jackson2Decoder to call the
canSerialize/canDeserialize methods that propagate the Throwable. It
adds a hook where users can receive that Throwable and act on it if
they want.

-----------------------------------------------------------------------
I have a `@RestController` `@RequestBody` that Jackson *should* be able
to deserialize, but it can't because I made an error in my
serialization setup. All I get in my application's logs is
> org.springframework.web.server.UnsupportedMediaTypeStatusException:
415 UNSUPPORTED_MEDIA_TYPE "Content type 'application/json' not
supported for bodyType=CLASS_NAME"

Meanwhile there's a very helpful `IllegalArgumentException` from within
Jackson that gets swallowed. Jackson actually propagates the exception
all the way up to `ObjectMapper#canDeserialize(JavaType)`, where it
sees that you haven't asked for that exception and swallows it. If we
instead called
`ObjectMapper#canSerialize(Class<T>, AtomicReference<Throwable>)`,
the consumer could do something with the exception. And the same is
true of `ObjectMapper#canDeserialize`.

This changes Jackson2Encoder and Jackson2Decoder to call the
canSerialize/canDeserialize methods that propagate the Throwable. It
adds a hook where users can receive that Throwable and act on it if
they want.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 10, 2020
@Anusien
Copy link
Author

Anusien commented Oct 13, 2020

If you're interested in this, I'd love to get it marked hacktoberfest_accepted.

Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

While the added protected method is slightly more convenient I don't see a big difference vs overriding canDecode and the overhead of create that AtomicReference instances goes to waste if not needed or when not debugging.

What do you plan to do with the information in the protected method? If it is just logging, we could consider making this all conditional on DEBUG level logging and automatically logging any exception at DEBUG level. That way it's useful out of the box and has no impact in production which may be good for what looks like a debugging feature. However I'm not sure how much noise it might add or if all exceptions should be logged?

@rstoyanchev
Copy link
Contributor

It seems we have a similar mechanism for HttpMessageConverter so we should make the two consistent.

@rstoyanchev rstoyanchev self-assigned this Nov 2, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 2, 2020
@rstoyanchev rstoyanchev added this to the 5.3.1 milestone Nov 2, 2020
@rstoyanchev rstoyanchev changed the title Allows Jackson2 encoders to propagate Throwables Allows Jackson2 encoders to log Throwable reason for not being able to serialize or deserialize Nov 2, 2020
@Anusien
Copy link
Author

Anusien commented Nov 2, 2020

I was concerned about the overhead of creating a new AtomicReference each time, but I wasn't confident enough about the way this thing was called to try and reuse an instance variable.

My understanding, which is admittedly limited, is that Jackson will only throw an exception if it thinks it should be able to deserialize and it can't. I'd want to log this at a higher level than DEBUG because it likely presents an actual problem. But I'll implement it this way.

One question though: why do we check for logger.isDebugEnabled() before deciding whether to log the actual exception (versus just using string concatenation)? CC @jhoeller who made this commit;

@jhoeller
Copy link
Contributor

jhoeller commented Nov 2, 2020

That special debug level check for a warn statement is a pattern that we're using in a few places. The goal is to only log a single line in case of just warn level, while a full stacktrace is being added to the warn level log statement if debug level is active as well. It's a compromise to not log an extra debug log statement with the full stacktrace, and to keep the plain warn log as concise as possible.

@rstoyanchev
Copy link
Contributor

I've added similar logic to the Jackson Encoder/Decoder but mainly enabled as long as the DEBUG level is on. This saves us from creating and using an AtomicReference at runtime for what is essentially a debugging feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants