-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix #4603 (Keep stacktrace when re-throwing exception with JsonMappingException) #5054
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
Conversation
*<p> | ||
* Feature is enabled by default. | ||
*/ | ||
UNWRAP_ROOT_CAUSE(true) |
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.
Since this is for 3.0, I think we can default it to false
.
Going further down this route: do we even need a MapperFeature
? I am ok with unwrapping being removed for this specific case (of setter/getter/field access).
@stevenschlansker WDYT?
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.
I personally don't see the value of having this configurable in the first place - for me, I would always be OK with nested exceptions where Jackson adds helpful descriptions like JacksonMappingException: couldn't write field 'foo' from type 'bar'
but always preserves the cause caused by: IllegalAccessError: your module doesn't read the other module
or whatever it is.
I would always keep the ultimate cause myself, even if it is not "friendly". But I can understand how other people might have different expectations, like Spring Boot will try to hide the failure details from you behind some friendly message - but I hate it ;)
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.
Thanks @stevenschlansker. I'll need to think about this a bit... I am not married to the unwrapping feature even tho I originally added it in the first place.
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.
+1 on having preserving the ultimaite root cause
. Seems like there is no harm in it?
*<p> | ||
* Feature is enabled by default. | ||
*/ | ||
UNWRAP_ROOT_CAUSE(true) |
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.
Question: are we happy with the name? For now this specifically applies to setter/getter/field access handling, and we could try to make it more specific.
(naming only matters if we decide to keep configurability, of course)
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.
Hmmm, I was thinking since MapperFeature being global, would apply to wherever we can.
So yes, name being generic is okay.
@JooHyukKim I think we should change default to EDIT: Actually.... let's make things even simpler and simply remove wrapping without new |
Sorry late reply. Will do! |
Fixes #4603