Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fixes serialization issues with Exception#cause [rework] #3229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6ec238
Fixes serialization issues with Exception#cause
May 10, 2023
e56ff6a
fixup! Fixes serialization issues with Exception#cause
May 10, 2023
b3f6050
Skip exception
May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions rewrite-core/src/main/java/org/openrewrite/internal/ExceptionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,4 @@ public static String sanitizeStackTrace(Throwable t, Class<?> until) {
}
return sanitized.toString();
}

public static boolean containsCircularReferences(Throwable exception) {
Set<Throwable> causes = Collections.newSetFromMap(new IdentityHashMap<>());
causes.add(exception);
boolean containsACircularReference = false;
while (exception != null && exception.getCause() != null) {
Throwable exceptionToFind = exception.getCause();
if (exceptionToFind != null) {

if (!causes.add(exceptionToFind)) {
containsACircularReference = true;
break;
} else {
exception = exceptionToFind;
}
} else {
exception = null;
}
}
return containsACircularReference;
}
}
29 changes: 19 additions & 10 deletions rewrite-core/src/main/java/org/openrewrite/marker/Markup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,28 @@ default String print(Cursor cursor, UnaryOperator<String> commentWrapper, boolea
}

static <T extends Tree> T error(T t, Throwable throwable) {
if (ExceptionUtils.containsCircularReferences(throwable)) {
throwable = new Exception(throwable.getMessage());
throwable.setStackTrace(throwable.getStackTrace());
}
return markup(t, new Markup.Error(randomId(), throwable));
return markup(t, new Markup.Error(randomId(), exceptionWithoutSelfReferences(throwable)));
}

static <T extends Tree> T warn(T t, Throwable throwable) {
if (ExceptionUtils.containsCircularReferences(throwable)) {
throwable = new Exception(throwable.getMessage());
throwable.setStackTrace(throwable.getStackTrace());
}
return markup(t, new Markup.Warn(randomId(), throwable));
return markup(t, new Markup.Warn(randomId(), exceptionWithoutSelfReferences(throwable)));
}

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;
}
Comment on lines +60 to 75
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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!


static <T extends Tree> T info(T t, String message) {
Expand Down
34 changes: 0 additions & 34 deletions rewrite-java-test/src/test/java/org/openrewrite/java/RecipeMarkupDemonstrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,11 @@
*/
package org.openrewrite.java;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.openrewrite.ExecutionContext;
import org.openrewrite.java.tree.JavaSourceFile;
import org.openrewrite.marker.Markup;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.test.RewriteTest.toRecipe;

class RecipeMarkupDemonstrationTest implements RewriteTest {

Expand All @@ -45,33 +40,4 @@ class Test {
)
);
}

@Test
void exceptionsWithoutCircularReferences() {
rewriteRun(
spec -> spec
.recipe(toRecipe(r -> new JavaIsoVisitor<>() {
@Override
public JavaSourceFile visitJavaSourceFile(JavaSourceFile cu, ExecutionContext p) {
Exception exception = new RuntimeException("this the parent of a circular exception");
Exception exception2 = new RuntimeException("this the child of a circular exception", exception);
exception.initCause(exception2);
JavaSourceFile sf = Markup.error(cu, exception);
Markup.Error marker = sf.getMarkers().findFirst(Markup.Error.class).get();
assert marker.getException().getCause() == null;
//Otherwise, there is an error if the exception is serialized.
return sf;
}
})),
java(
"""
class Test {
}
""",
"""
/*~~(this the parent of a circular exception)~~>*/class Test {
}
""")
);
}
}