Skip to content

Harden handleError() against secondary exceptions #7387

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

Merged
Jenson3210 merged 3 commits into from
Apr 16, 2026
Merged

Harden handleError() against secondary exceptions #7387

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8abf885
Harden handleError() against secondary exceptions during error marker…
Jenson3210 Apr 16, 2026
f4b3872
Fix DockerPrinter NPE when Label or Env pairs are null
Jenson3210 Apr 16, 2026
2437936
Revert "Fix DockerPrinter NPE when Label or Env pairs are null"
Jenson3210 Apr 16, 2026
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
10 changes: 8 additions & 2 deletions rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.openrewrite.config.DeclarativeRecipe;
import org.openrewrite.internal.ExceptionUtils;
import org.openrewrite.internal.FindRecipeRunException;
import org.openrewrite.marker.Markup;
import org.openrewrite.internal.RecipeRunException;
import org.openrewrite.internal.StringUtils;
import org.openrewrite.marker.*;
Expand Down Expand Up @@ -718,8 +719,13 @@ private void recordSourceFileResult(@Nullable String beforePath, @Nullable Strin
ctx.getOnError().accept(t);

if (t instanceof RecipeRunException && after != null) {
RecipeRunException vt = (RecipeRunException) t;
after = (SourceFile) new FindRecipeRunException(vt).visitNonNull(after, 0);
try {
RecipeRunException vt = (RecipeRunException) t;
after = (SourceFile) new FindRecipeRunException(vt).visitNonNull(after, 0);
} catch (Throwable ignored) {
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 16, 2026

Choose a reason for hiding this comment

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

Good catch!

// Tree is too broken for node-level marker — fall back to marking the whole file
after = Markup.error(after, t);
}
}

// Use the original source file to record the error, not the one that may have been modified by the visitor.
Expand Down
28 changes: 16 additions & 12 deletions rewrite-docker/src/main/java/org/openrewrite/docker/internal/DockerPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,15 @@ public Docker visitArg(Docker.Arg arg, PrintOutputCapture<P> p) {
public Docker visitEnv(Docker.Env env, PrintOutputCapture<P> p) {
beforeSyntax(env, p);
p.append(env.getKeyword());
for (Docker.Env.EnvPair pair : env.getPairs()) {
visitSpace(pair.getPrefix(), p);
visit(pair.getKey(), p);
if (pair.isHasEquals()) {
p.append("=");
if (env.getPairs() != null) {
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 16, 2026

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@Jenson3210 Jenson3210 Apr 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 16, 2026

Choose a reason for hiding this comment

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

Ah I now see neither field had a safe default, so yes then this makes sense, thanks.

rewrite/rewrite-docker/src/main/java/org/openrewrite/docker/tree/Docker.java

Line 368 in 4eb011c

List<EnvPair> pairs;

Might even want to mark those as nullable then, unless I misunderstood. Still reading from a small screen.

for (Docker.Env.EnvPair pair : env.getPairs()) {
visitSpace(pair.getPrefix(), p);
visit(pair.getKey(), p);
if (pair.isHasEquals()) {
p.append("=");
}
visit(pair.getValue(), p);
}
visit(pair.getValue(), p);
}
afterSyntax(env, p);
return env;
Expand All @@ -173,13 +175,15 @@ public Docker visitEnv(Docker.Env env, PrintOutputCapture<P> p) {
public Docker visitLabel(Docker.Label label, PrintOutputCapture<P> p) {
beforeSyntax(label, p);
p.append(label.getKeyword());
for (Docker.Label.LabelPair pair : label.getPairs()) {
visitSpace(pair.getPrefix(), p);
visit(pair.getKey(), p);
if (pair.isHasEquals()) {
p.append("=");
if (label.getPairs() != null) {
for (Docker.Label.LabelPair pair : label.getPairs()) {
visitSpace(pair.getPrefix(), p);
visit(pair.getKey(), p);
if (pair.isHasEquals()) {
p.append("=");
}
visit(pair.getValue(), p);
}
visit(pair.getValue(), p);
}
afterSyntax(label, p);
return label;
Expand Down
⚡