-
Notifications
You must be signed in to change notification settings - Fork 364
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
Fixes serialization issues with Exception#cause [rework] #3229
Conversation
static Throwable exceptionWithoutSelfReferences(Throwable throwable) { | ||
// remove self-references from the cause chain to avoid serialization problems from the Moderne plugins | ||
Throwable temp = throwable; | ||
do { | ||
if (temp.getCause() == null) { | ||
try { | ||
temp.initCause(null); | ||
} catch (IllegalStateException ignored) { | ||
//ignore, the cause was already initialized to null | ||
} | ||
break; | ||
} | ||
} while ((temp = temp.getCause()) != null); | ||
|
||
return throwable; | ||
} |
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.
Isn't this a rendering issue, not an issue in the Markup.Warn
creation problem? This seems like something that should be fixed in rendering or serialization, not by messing with the exception that gets returned by the AST.
Basically, this looks like you're fixing the wrong problem
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.
AFAICT the serialization problem we've seen is due to FasterXML/jackson-databind#1986. I don't know if I understand FasterXML/jackson-databind#1986 (comment) correctly, but also looking at FasterXML/jackson-databind#877 it seems like it should be possible to define and register a mixin similar to this:
public abstract class ThrowableMixin {
@JsonIgnore private Throwable cause;
@JsonIgnore public abstract Throwable getCause();
}
I don't know if that will work or if it will have the effect of not serializing any of the exception causes.
This being said, I agree with you that it would be better if we could get Jackson to properly serialize exceptions without having to muck with them like this.
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 think that you are right. We can create instead a jackson module to serialize exceptions :)
Closing the PR for now.
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.
If you find a way to do this, that would be great!
Context: #3199
I merged assuming that GH Actions run in the previous commit, but there were tests errors. So I reverted the commit and this PR fixes the tests errors.