From 9a7bec6e2caca02416dbdd809747df2f69172323 Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Fri, 3 Jul 2026 15:35:04 -0400 Subject: [PATCH 1/2] Rewrite trailing setters and field setters as mapper.rebuild() chains `MigrateMapperSettersToBuilder` previously left `mapper.rebuild()...build()` TODO comments on any setter it couldn't fold into a `Mapper.builder()...build()` chain, and silently skipped setters whose receiver was a field access (`this.mapper.setX(...)`). This change rewrites those setters as `receiver = receiver.rebuild().(args).build();` when the receiver is safely reassignable: - a non-final local declared in an enclosing block (method parameters are excluded because reassignment silently drops the caller's expected mutation), or - a `J.FieldAccess` with an identifier target and a non-final field. A `doAfterVisit` post-pass coalesces consecutive per-receiver rebuild assignments into a single chained `.rebuild()....build()`, so a run of N setters becomes one assignment rather than N. Setters that remain not-rewritable (final field, parameter, receiver behind a method chain) still get the existing TODO comment. --- .../MigrateMapperSettersToBuilder.java | 267 +++++++++++++++++- .../MigrateMapperSettersToBuilderTest.java | 197 +++++++++++++ 2 files changed, 462 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java b/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java index 84ed040..486551e 100644 --- a/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java +++ b/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java @@ -184,6 +184,7 @@ public J visitBlock(J.Block block, ExecutionContext ctx) { if (visited != block) { doAfterVisit(new UpdateSerializationInclusionConfiguration().getVisitor()); doAfterVisit(new UpdateAutoDetectVisibilityConfiguration().getVisitor()); + doAfterVisit(coalesceRebuildAssignments()); } return visited; } @@ -276,11 +277,14 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { } } - if (!(mi.getSelect() instanceof J.Identifier)) { + Expression select = mi.getSelect(); + if (!(select instanceof J.Identifier) && + !(select instanceof J.FieldAccess && + ((J.FieldAccess) select).getTarget() instanceof J.Identifier)) { return mi; } - String matchedMapper = matchingMapperType(mi.getSelect().getType()); + String matchedMapper = matchingMapperType(select.getType()); if (matchedMapper == null) { return mi; } @@ -290,6 +294,17 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { return mi; } + // Try to rewrite as `.rebuild().(...).build();` + // when the receiver is safely reassignable (non-final local declared in an + // enclosing block, or a non-final field). Parameters are excluded because + // reassignment would silently drop the caller's mutation. + if (isTopLevelStatement(getCursor()) && isReassignableReceiver(select, getCursor())) { + J rebuilt = rewriteAsRebuildAssignment(mi, select, matchedMapper, mapping, ctx); + if (rebuilt != null) { + return rebuilt; + } + } + // Not eligible for builder migration - add a TODO comment String simpleMapperName = matchedMapper.substring(matchedMapper.lastIndexOf('.') + 1); String commentText = String.format( @@ -696,10 +711,257 @@ private J applyBuilderTemplate(String mapperFqn, List setter } return chained; } + + /** + * Rewrites {@code receiver.setterName(args)} as + * {@code receiver = receiver.rebuild().builderName(args).build();} + * via {@link JavaTemplate}. The result is a {@link J.Assignment} that + * replaces the original method-invocation statement. + *

+ * Consecutive assignments produced by this method are coalesced into + * a single chained {@code .rebuild()....build()} by the post-pass + * enqueued via {@code doAfterVisit(coalesceRebuildAssignments())}. + */ + private @Nullable J rewriteAsRebuildAssignment(J.MethodInvocation mi, Expression select, + String mapperFqn, SetterToBuilderMapping mapping, + ExecutionContext ctx) { + String builderName = mapping.builderName; + // Match the special-case rename in appendBuilderCall so setDefaultPropertyInclusion + // resolves against the Jackson 2 classpath; a follow-up recipe rewrites it. + if (mapping == SetterToBuilderMapping.SET_DEFAULT_PROPERTY_INCLUSION && + mi.getArguments().size() == 1 && + !(mi.getArguments().get(0) instanceof J.Empty) && + TypeUtils.isAssignableTo("com.fasterxml.jackson.annotation.JsonInclude$Include", + mi.getArguments().get(0).getType())) { + builderName = "serializationInclusion"; + } + StringBuilder templateCode = new StringBuilder(); + templateCode.append("#{any(").append(mapperFqn).append(")} = #{any(") + .append(mapperFqn).append(")}.rebuild()\n.").append(builderName).append("("); + List templateArgs = new ArrayList<>(); + templateArgs.add(select); + templateArgs.add(select); + boolean first = true; + for (Expression arg : mi.getArguments()) { + if (arg instanceof J.Empty) { + continue; + } + if (!first) { + templateCode.append(", "); + } + first = false; + templateCode.append("#{any()}"); + templateArgs.add(arg); + } + templateCode.append(")\n.build()"); + + maybeAddImport(mapperFqn); + maybeAddImport(JSON_INCLUDE); + + JavaParser.Builder parser = JavaParser.fromJavaVersion() + .classpathFromResources(ctx, "jackson-annotations-2", "jackson-core-2", "jackson-databind-2") + .dependsOn(mapperStub(mapperFqn, emptyList())); + + return JavaTemplate.builder(templateCode.toString()) + .imports(mapperFqn, JSON_INCLUDE) + .javaParser(parser) + .build() + .apply(getCursor(), mi.getCoordinates().replace(), templateArgs.toArray()); + } } ); } + /** + * True when {@code cursor}'s value is a {@link J.MethodInvocation} whose parent tree + * cursor is a {@link J.Block} — i.e. the MI is a top-level statement in a block, + * which is where a rebuild-as-assignment rewrite is valid. + */ + private static boolean isTopLevelStatement(Cursor cursor) { + Object parent = cursor.getParentTreeCursor().getValue(); + return parent instanceof J.Block; + } + + /** + * True when the setter receiver is safely reassignable via + * {@code receiver = receiver.rebuild()....build()}. Accepts: + *

    + *
  • a {@link J.Identifier} for a non-final local declared in an enclosing block + * (parameters are excluded — reassigning them silently drops the caller's + * expected mutation), and
    • + *
  • a {@link J.FieldAccess} with an identifier target and a non-final field.
    • + *
+ */ + private static boolean isReassignableReceiver(Expression select, Cursor cursor) { + if (select instanceof J.Identifier) { + return isReassignableLocal((J.Identifier) select, cursor); + } + if (select instanceof J.FieldAccess) { + J.FieldAccess fa = (J.FieldAccess) select; + if (!(fa.getTarget() instanceof J.Identifier)) { + return false; + } + JavaType.Variable v = fa.getName().getFieldType(); + return v != null && !v.hasFlags(Flag.Final); + } + return false; + } + + private static boolean isReassignableLocal(J.Identifier ident, Cursor cursor) { + JavaType.Variable fieldType = ident.getFieldType(); + if (fieldType != null && fieldType.hasFlags(Flag.Final)) { + return false; + } + String name = ident.getSimpleName(); + Cursor c = cursor; + while (c != null) { + Object v = c.getValue(); + if (v instanceof J.Block) { + for (Statement stmt : ((J.Block) v).getStatements()) { + J.VariableDeclarations vd = extractVariableDeclarations(stmt); + if (vd == null) { + continue; + } + for (J.VariableDeclarations.NamedVariable nv : vd.getVariables()) { + if (name.equals(nv.getName().getSimpleName()) && + TypeUtils.isOfType(nv.getName().getType(), ident.getType())) { + return !vd.hasModifier(J.Modifier.Type.Final); + } + } + } + } + if (v instanceof J.MethodDeclaration) { + for (Statement p : ((J.MethodDeclaration) v).getParameters()) { + if (p instanceof J.VariableDeclarations) { + for (J.VariableDeclarations.NamedVariable nv : ((J.VariableDeclarations) p).getVariables()) { + if (name.equals(nv.getName().getSimpleName())) { + return false; + } + } + } + } + return false; + } + c = c.getParent(); + } + return false; + } + + /** + * Post-pass invoked via {@code doAfterVisit} that walks each block and coalesces + * runs of consecutive {@code = .rebuild().(...).build();} assignments + * with a semantically equal LHS into a single chained assignment. Purely a tree + * rewrite — no {@link JavaTemplate} — so it's safe to run at post-pass time. + */ + private static JavaIsoVisitor coalesceRebuildAssignments() { + return new JavaIsoVisitor() { + @Override + public J.Block visitBlock(J.Block block, ExecutionContext ctx) { + J.Block b = super.visitBlock(block, ctx); + List stmts = b.getStatements(); + if (stmts.size() < 2) { + return b; + } + List merged = new ArrayList<>(stmts.size()); + boolean changed = false; + int i = 0; + while (i < stmts.size()) { + RebuildParts head = tryParseRebuildAssignment(stmts.get(i)); + if (head == null) { + merged.add(stmts.get(i)); + i++; + continue; + } + List allChainCalls = new ArrayList<>(head.chainCalls); + int j = i + 1; + while (j < stmts.size()) { + RebuildParts next = tryParseRebuildAssignment(stmts.get(j)); + if (next == null || !SemanticallyEqual.areEqual(head.lhs, next.lhs)) { + break; + } + allChainCalls.addAll(next.chainCalls); + j++; + } + if (j == i + 1) { + merged.add(stmts.get(i)); + i++; + continue; + } + Statement coalesced = rebuildCoalescedAssignment(head, allChainCalls); + merged.add(coalesced); + changed = true; + i = j; + } + return changed ? b.withStatements(merged) : b; + } + }; + } + + /** + * Parsed shape of a rebuild-assignment statement: {@code = .rebuild().(...).(...).build();} + * The {@code chainCalls} are the method invocations between {@code .rebuild()} and the + * terminal {@code .build()}, in call order. + */ + private static class RebuildParts { + final J.Assignment assignment; + final Expression lhs; + final J.MethodInvocation buildCall; + final J.MethodInvocation rebuildCall; + final List chainCalls; + + RebuildParts(J.Assignment assignment, Expression lhs, J.MethodInvocation buildCall, + J.MethodInvocation rebuildCall, List chainCalls) { + this.assignment = assignment; + this.lhs = lhs; + this.buildCall = buildCall; + this.rebuildCall = rebuildCall; + this.chainCalls = chainCalls; + } + } + + private static @Nullable RebuildParts tryParseRebuildAssignment(Statement stmt) { + J.Assignment assignment; + if (stmt instanceof J.Assignment) { + assignment = (J.Assignment) stmt; + } else { + // Kotlin wraps assignments too; keep the Java-only path for now. + return null; + } + if (!(assignment.getAssignment() instanceof J.MethodInvocation)) { + return null; + } + J.MethodInvocation buildCall = (J.MethodInvocation) assignment.getAssignment(); + if (!"build".equals(buildCall.getName().getSimpleName())) { + return null; + } + // Walk down the chain until we find the .rebuild() call + List chainCalls = new ArrayList<>(); + Expression current = buildCall.getSelect(); + while (current instanceof J.MethodInvocation) { + J.MethodInvocation mi = (J.MethodInvocation) current; + if ("rebuild".equals(mi.getName().getSimpleName())) { + Collections.reverse(chainCalls); + return new RebuildParts(assignment, assignment.getVariable(), buildCall, mi, chainCalls); + } + chainCalls.add(mi); + current = mi.getSelect(); + } + return null; + } + + /** + * Reconstruct a coalesced {@code = .rebuild().<...allChainCalls>.build();} + * from the head assignment's LHS/build/rebuild nodes and the combined chain calls. + */ + private static Statement rebuildCoalescedAssignment(RebuildParts head, List allChainCalls) { + Expression newSelect = head.rebuildCall; + for (J.MethodInvocation chainCall : allChainCalls) { + newSelect = chainCall.withSelect(newSelect); + } + J.MethodInvocation newBuild = head.buildCall.withSelect(newSelect); + return head.assignment.withAssignment(newBuild); + } + /** * Returns the FQN of the mapper type matched by the given new class, or null if none match. */ @@ -822,6 +1084,7 @@ private static String mapperStub(String mapperFqn, List unkn .append(" extends com.fasterxml.jackson.databind.ObjectMapper {\n"); sb.append(" public ").append(simpleName).append("() {}\n"); sb.append(" public static Builder builder() { return null; }\n"); + sb.append(" public Builder rebuild() { return null; }\n"); sb.append(" public static class Builder extends ") .append("com.fasterxml.jackson.databind.cfg.MapperBuilder<") .append(simpleName).append(", Builder> {\n"); diff --git a/src/test/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilderTest.java b/src/test/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilderTest.java index 1aefb0b..dd9d90f 100644 --- a/src/test/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilderTest.java +++ b/src/test/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilderTest.java @@ -682,6 +682,203 @@ JsonMapper create() { } } + @Nested + class RewriteAsRebuild { + + @Test + void trailingSetterAfterGapOnLocal() { + rewriteRun( + java( + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + JsonMapper create() { + JsonMapper mapper = new JsonMapper(); + mapper.disable(SerializationFeature.INDENT_OUTPUT); + System.out.println(mapper); + mapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + return mapper; + } + } + """, + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + JsonMapper create() { + JsonMapper mapper = JsonMapper.builder() + .disable(SerializationFeature.INDENT_OUTPUT) + .build(); + System.out.println(mapper); + mapper = mapper.rebuild() + .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + .build(); + return mapper; + } + } + """ + ) + ); + } + + @Test + void trailingSettersCoalesceIntoOneRebuildChain() { + rewriteRun( + java( + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + JsonMapper create() { + JsonMapper mapper = new JsonMapper(); + mapper.disable(SerializationFeature.INDENT_OUTPUT); + System.out.println(mapper); + mapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + mapper.disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES); + return mapper; + } + } + """, + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + JsonMapper create() { + JsonMapper mapper = JsonMapper.builder() + .disable(SerializationFeature.INDENT_OUTPUT) + .build(); + System.out.println(mapper); + mapper = mapper.rebuild() + .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + .disable(DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES) + .build(); + return mapper; + } + } + """ + ) + ); + } + + @Test + void thisFieldSetterCoalescesIntoRebuild() { + rewriteRun( + java( + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + private JsonMapper mapper = new JsonMapper(); + + void configure() { + this.mapper.disable(SerializationFeature.INDENT_OUTPUT); + this.mapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + } + } + """, + """ + import com.fasterxml.jackson.databind.DeserializationFeature; + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + private JsonMapper mapper = new JsonMapper(); + + void configure() { + this.mapper = this.mapper.rebuild() + .disable(SerializationFeature.INDENT_OUTPUT) + .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + .build(); + } + } + """ + ) + ); + } + + @Test + void qualifiedFieldSetterRewritten() { + rewriteRun( + java( + """ + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + static class Holder { + JsonMapper mapper = new JsonMapper(); + } + + void configure(Holder holder) { + holder.mapper.disable(SerializationFeature.INDENT_OUTPUT); + } + } + """, + """ + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + static class Holder { + JsonMapper mapper = new JsonMapper(); + } + + void configure(Holder holder) { + holder.mapper = holder.mapper.rebuild() + .disable(SerializationFeature.INDENT_OUTPUT) + .build(); + } + } + """ + ) + ); + } + + @Test + void finalFieldStillGetsTodoComment() { + rewriteRun( + java( + """ + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + private final JsonMapper mapper = new JsonMapper(); + + void configure() { + this.mapper.disable(SerializationFeature.INDENT_OUTPUT); + } + } + """, + """ + import com.fasterxml.jackson.databind.SerializationFeature; + import com.fasterxml.jackson.databind.json.JsonMapper; + + class A { + private final JsonMapper mapper = new JsonMapper(); + + void configure() { + // TODO disable could not be folded to the builder of JsonMapper. Use mapper.rebuild().disable(...).build() or move to the mapper's instantiation site. + this.mapper.disable(SerializationFeature.INDENT_OUTPUT); + } + } + """ + ) + ); + } + } + @Nested class FluentChain { From db9c799f5f3ba5b7389570c5f609c71757f7519d Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Fri, 3 Jul 2026 15:41:57 -0400 Subject: [PATCH 2/2] Remove verbose comments --- .../MigrateMapperSettersToBuilder.java | 57 +------------------ 1 file changed, 3 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java b/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java index 486551e..04566cc 100644 --- a/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java +++ b/src/main/java/org/openrewrite/java/jackson/MigrateMapperSettersToBuilder.java @@ -294,10 +294,6 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) { return mi; } - // Try to rewrite as `.rebuild().(...).build();` - // when the receiver is safely reassignable (non-final local declared in an - // enclosing block, or a non-final field). Parameters are excluded because - // reassignment would silently drop the caller's mutation. if (isTopLevelStatement(getCursor()) && isReassignableReceiver(select, getCursor())) { J rebuilt = rewriteAsRebuildAssignment(mi, select, matchedMapper, mapping, ctx); if (rebuilt != null) { @@ -712,22 +708,10 @@ private J applyBuilderTemplate(String mapperFqn, List setter return chained; } - /** - * Rewrites {@code receiver.setterName(args)} as - * {@code receiver = receiver.rebuild().builderName(args).build();} - * via {@link JavaTemplate}. The result is a {@link J.Assignment} that - * replaces the original method-invocation statement. - *

- * Consecutive assignments produced by this method are coalesced into - * a single chained {@code .rebuild()....build()} by the post-pass - * enqueued via {@code doAfterVisit(coalesceRebuildAssignments())}. - */ private @Nullable J rewriteAsRebuildAssignment(J.MethodInvocation mi, Expression select, String mapperFqn, SetterToBuilderMapping mapping, ExecutionContext ctx) { String builderName = mapping.builderName; - // Match the special-case rename in appendBuilderCall so setDefaultPropertyInclusion - // resolves against the Jackson 2 classpath; a follow-up recipe rewrites it. if (mapping == SetterToBuilderMapping.SET_DEFAULT_PROPERTY_INCLUSION && mi.getArguments().size() == 1 && !(mi.getArguments().get(0) instanceof J.Empty) && @@ -772,26 +756,10 @@ private J applyBuilderTemplate(String mapperFqn, List setter ); } - /** - * True when {@code cursor}'s value is a {@link J.MethodInvocation} whose parent tree - * cursor is a {@link J.Block} — i.e. the MI is a top-level statement in a block, - * which is where a rebuild-as-assignment rewrite is valid. - */ private static boolean isTopLevelStatement(Cursor cursor) { - Object parent = cursor.getParentTreeCursor().getValue(); - return parent instanceof J.Block; + return cursor.getParentTreeCursor().getValue() instanceof J.Block; } - /** - * True when the setter receiver is safely reassignable via - * {@code receiver = receiver.rebuild()....build()}. Accepts: - *

    - *
  • a {@link J.Identifier} for a non-final local declared in an enclosing block - * (parameters are excluded — reassigning them silently drops the caller's - * expected mutation), and
    • - *
  • a {@link J.FieldAccess} with an identifier target and a non-final field.
    • - *
- */ private static boolean isReassignableReceiver(Expression select, Cursor cursor) { if (select instanceof J.Identifier) { return isReassignableLocal((J.Identifier) select, cursor); @@ -847,12 +815,6 @@ private static boolean isReassignableLocal(J.Identifier ident, Cursor cursor) { return false; } - /** - * Post-pass invoked via {@code doAfterVisit} that walks each block and coalesces - * runs of consecutive {@code = .rebuild().(...).build();} assignments - * with a semantically equal LHS into a single chained assignment. Purely a tree - * rewrite — no {@link JavaTemplate} — so it's safe to run at post-pass time. - */ private static JavaIsoVisitor coalesceRebuildAssignments() { return new JavaIsoVisitor() { @Override @@ -897,11 +859,6 @@ public J.Block visitBlock(J.Block block, ExecutionContext ctx) { }; } - /** - * Parsed shape of a rebuild-assignment statement: {@code = .rebuild().(...).(...).build();} - * The {@code chainCalls} are the method invocations between {@code .rebuild()} and the - * terminal {@code .build()}, in call order. - */ private static class RebuildParts { final J.Assignment assignment; final Expression lhs; @@ -920,13 +877,10 @@ private static class RebuildParts { } private static @Nullable RebuildParts tryParseRebuildAssignment(Statement stmt) { - J.Assignment assignment; - if (stmt instanceof J.Assignment) { - assignment = (J.Assignment) stmt; - } else { - // Kotlin wraps assignments too; keep the Java-only path for now. + if (!(stmt instanceof J.Assignment)) { return null; } + J.Assignment assignment = (J.Assignment) stmt; if (!(assignment.getAssignment() instanceof J.MethodInvocation)) { return null; } @@ -934,7 +888,6 @@ private static class RebuildParts { if (!"build".equals(buildCall.getName().getSimpleName())) { return null; } - // Walk down the chain until we find the .rebuild() call List chainCalls = new ArrayList<>(); Expression current = buildCall.getSelect(); while (current instanceof J.MethodInvocation) { @@ -949,10 +902,6 @@ private static class RebuildParts { return null; } - /** - * Reconstruct a coalesced {@code = .rebuild().<...allChainCalls>.build();} - * from the head assignment's LHS/build/rebuild nodes and the combined chain calls. - */ private static Statement rebuildCoalescedAssignment(RebuildParts head, List allChainCalls) { Expression newSelect = head.rebuildCall; for (J.MethodInvocation chainCall : allChainCalls) {