Harden handleError() against secondary exceptions#7387
Conversation
… insertion Wrap the FindRecipeRunException re-traversal in a try-catch so that if the tree is too broken for node-level marker insertion, we fall back to marking the entire file with Markup.error. This ensures the error is always visible even when the tree has invariant violations (e.g. null collection fields).
DockerPrinter.visitLabel() and visitEnv() used for-each loops directly on getPairs() which throws NPE when the list is null. Added null guards matching the existing pattern used for other nullable list fields like getFlags().
| visit(pair.getKey(), p); | ||
| if (pair.isHasEquals()) { | ||
| p.append("="); | ||
| if (env.getPairs() != null) { |
There was a problem hiding this comment.
I think Merlin would have fixed this in his AST write PR. Not sure if we'd need this still then, as I would not expect the field to be null ever, but haven't checked (on mobile on plane)
There was a problem hiding this comment.
https://github.com/openrewrite/rewrite/blob/main/rewrite-docker/src/main/java/org/openrewrite/docker/DockerVisitor.java#L133 is null safe, but this one is not.
I will let you decide if we want to have this "defensive" fix in or not :)
There was a problem hiding this comment.
Ah I now see neither field had a safe default, so yes then this makes sense, thanks.
Might even want to mark those as nullable then, unless I misunderstood. Still reading from a small screen.
| try { | ||
| RecipeRunException vt = (RecipeRunException) t; | ||
| after = (SourceFile) new FindRecipeRunException(vt).visitNonNull(after, 0); | ||
| } catch (Throwable ignored) { |
github-project-automation
Bot
moved this from In Progress
to Ready to Review
in OpenRewrite
This reverts commit f4b3872.
2 participants
Summary
FindRecipeRunExceptionre-traversal inRecipeRunCycle.handleError()with a try-catchMarkup.error()on the entire fileContext
When a recipe throws on a tree with broken invariants,
handleError()attempts to re-traverse the tree viaFindRecipeRunExceptionto attach an error marker to the specific node that failed. If that re-traversal itself throws (because the tree is too broken to traverse), the secondary exception could escapehandleError(). This change catches that secondary exception and falls back to a file-level error marker instead.Test plan
./gradlew :rewrite-core:testpasses