Reuse existing transaction managers and keep code compiling for `AddTransactionManagerToTaskletAndChunk` by myhau · Pull Request #1189 · openrewrite/rewrite-spring

myhau · 2026-10-19T08:32:00Z

AddTransactionManagerToTaskletAndChunk added PlatformTransactionManager transactionManager to every method calling a one-argument tasklet() or chunk(), even when a transaction manager was already there, and SpringBatch5To6Migration also runs it on code that is already on Spring Batch 6. It now reuses an existing transaction manager parameter and leaves chains that already call transactionManager(..) alone.

Example issues solved

Transaction manager subtype parameter, set later in the chain (gs-batch-processing)

Before

Step step1(DataSourceTransactionManager transactionManager, ItemReader<String> reader, ItemWriter<String> writer) {
    return stepBuilderFactory.get("step1")
            .<String, String>chunk(3)
            .reader(reader)
            .writer(writer)
            .transactionManager(transactionManager)
            .build();
}

After (problem)

Step step1(DataSourceTransactionManager transactionManager, ItemReader<String> reader, ItemWriter<String> writer,
           PlatformTransactionManager transactionManager) { // duplicate parameter, does not compile
    return stepBuilderFactory.get("step1")
            .<String, String>chunk(3)
            .reader(reader)
            .writer(writer)
            .transactionManager(transactionManager)
            .build();
}

After (fixed, unchanged)

Step step1(DataSourceTransactionManager transactionManager, ItemReader<String> reader, ItemWriter<String> writer) {
    return stepBuilderFactory.get("step1")
            .<String, String>chunk(3)
            .reader(reader)
            .writer(writer)
            .transactionManager(transactionManager)
            .build();
}

Transaction manager from a field, set later in the chain

Before

class MyJobConfig {
    StepBuilderFactory stepBuilderFactory;
    PlatformTransactionManager txManager;

    Step myStep(Tasklet tasklet) {
        return stepBuilderFactory.get("myStep")
                .tasklet(tasklet)
                .allowStartIfComplete(true)
                .transactionManager(txManager)
                .build();
    }
}

After (problem)

class MyJobConfig {
    StepBuilderFactory stepBuilderFactory;
    PlatformTransactionManager txManager;

    Step myStep(Tasklet tasklet, PlatformTransactionManager transactionManager) { // unused parameter
        return stepBuilderFactory.get("myStep")
                .tasklet(tasklet)
                .allowStartIfComplete(true)
                .transactionManager(txManager)
                .build();
    }
}

After (fixed, unchanged)

class MyJobConfig {
    StepBuilderFactory stepBuilderFactory;
    PlatformTransactionManager txManager;

    Step myStep(Tasklet tasklet) {
        return stepBuilderFactory.get("myStep")
                .tasklet(tasklet)
                .allowStartIfComplete(true)
                .transactionManager(txManager)
                .build();
    }
}

Transaction manager subtype parameter with another name

Before

Step myStep(Tasklet tasklet, DataSourceTransactionManager txManager) {
    return stepBuilderFactory.get("myStep")
            .tasklet(tasklet)
            .build();
}

After (problem)

Step myStep(Tasklet tasklet, DataSourceTransactionManager txManager, PlatformTransactionManager transactionManager) { // second transaction manager
    return stepBuilderFactory.get("myStep")
            .tasklet(tasklet, transactionManager)
            .build();
}

After (fixed)

Step myStep(Tasklet tasklet, DataSourceTransactionManager txManager) {
    return stepBuilderFactory.get("myStep")
            .tasklet(tasklet, txManager)
            .build();
}

…steps `AddTransactionManagerToTaskletAndChunk` added `PlatformTransactionManager transactionManager` to every method calling a one-argument `tasklet(..)` or `chunk(..)`, even when - a parameter of a `PlatformTransactionManager` subtype was already present, which produced a duplicate parameter name or a second transaction manager, or - a later `.transactionManager(..)` in the same chain already configured the step, which left the new parameter unused. Reuse an existing subtype parameter, skip chains that already set the transaction manager, and leave calls outside a method (constructors, initializers) unchanged.

…gerToTaskletAndChunk Instead of running a nested visitor over every method body, the one-argument `tasklet(..)` or `chunk(..)` call marks its enclosing method, which adds the parameter after its body has been visited. The three argument templates only differed in the type of the first argument, so they now share one template. A call in a method of a local or anonymous class now adds the parameter to that method, instead of an unused one to the outer method.

The recipe is now a single visitor. A one-argument `tasklet(..)` or `chunk(..)` call takes the enclosing method's transaction manager parameter, or creates one that is appended to the method once its body has been visited, so each builder chain is walked once. - The method type gets the new parameter's name as well as its type, and the kept argument goes through an untyped placeholder. - A call in a method of a local or anonymous class uses the parameter of the method that declares the class instead of changing the inner method's signature; an anonymous class in a field initializer is left unchanged. - Methods without a method type are skipped like constructors. - Tests for lambda and anonymous tasklets, calls inside lambdas, local and anonymous classes, constructors and field initializers; the cases that expect no change are grouped in a nested NoChange class.

myhau · 2026-10-20T08:25:56Z

-        return statement instanceof J.VariableDeclarations &&
-                TypeUtils.isOfClassType(((J.VariableDeclarations) statement).getType(),
-                        "org.springframework.transaction.PlatformTransactionManager");
+    private static J.VariableDeclarations newTransactionManagerParameter(Cursor declaration, ExecutionContext ctx) {


(minor)

Do you think these static helpers should move into the visitor?

Hard to say (now). I tend to have to nudge Claude to inline small methods only used once. For anything beyond that it's nice if they can be static, but start with inlining small single use methods and see what's left.

myhau · 2026-10-20T08:25:58Z

+        Cursor classBody = method.getParentTreeCursor();
+        boolean localOrAnonymous = classBody.getParentTreeCursor().getValue() instanceof J.NewClass ||
+                classBody.firstEnclosing(J.MethodDeclaration.class) != null;
+        return localOrAnonymous ? enclosingMethod(method) : method;


Do you think recursion is ok here or do you prefer loops in general?

We tend to prefer loops; they're more explicit, easier to follow, and don't lead to deep stacks.

myhau · 2026-10-20T08:25:59Z

+        if (chained == null || chained.getSelect() != cursor.getValue()) {
+            return false;
        }
+        return TRANSACTION_MANAGER_MATCHER.matches(chained) || chainConfiguresTransactionManager(parent);


are there any "rules" around recursion in Open Rewrite recipes?

here the depth is the number of calls chained after tasklet(..) or chunk(..). Happy to use a loop instead.

Loops preferred indeed. See also https://github.com/openrewrite/rewrite-spring/pull/1189/changes#r4336753234

…hunk

myhau · 2026-10-20T09:22:32Z

-import static java.util.Collections.singletonList;
+import static java.util.Objects.requireNonNull;

 public class AddTransactionManagerToTaskletAndChunk extends Recipe {


I'm also thinking about adding some kind of "only migrate if we are on Spring Batch 4" precondition here:

rewrite-spring/src/main/resources/META-INF/rewrite/spring-batch-5.0.yml

Lines 21 to 29 in 39acb32

description: Migrate applications built on Spring Batch 4.3 to the latest Spring Batch 5.0 release.

tags:

- spring

- batch

preconditions:

- org.openrewrite.Singleton

recipeList:

- org.openrewrite.java.dependencies.UpgradeDependencyVersion:

groupId: org.springframework.batch

How do you approach this usually?

What's the motivation of adding the precondition of being on Spring Batch 4, which is the older version here?

Typically we write our recipes to be idempotent, and selectively matching, such that a recipe targeting a method on v4 would be a no-op when run on v5.

I'm still familiarizing myself with the changes you're proposing so hard to say if that applies here; I'm reading & commenting as I move from the top down.

…oTaskletAndChunk - The parameter template moves into the `computeMessageIfAbsent` lambda, and the `Tasklet` and `CompletionPolicy` type names into their matchers. - `addParameter` is now `withParameter`, the enclosing method cursor is `methodCursor`, and the matched call flag is `taskletOrChunk`. - The call-inside-lambda test is folded into the local and anonymous class test.

myhau · 2026-10-20T09:54:53Z

I'm putting it back to draft state, I need to double-check some things.

myhau · 2026-10-20T12:48:57Z

              }
-              """
+              """,
+            spec -> spec.afterRecipe(cu -> {


Here I wanted to check that the recipe updates the type information along with the printed code, so later recipes that match by type still find the new call

(this one would fail on main)

Any utility I could reuse instead (or existing test that does it better - for inspiration)?

This seems fine; thanks for calling it out.

myhau · 2026-10-20T12:52:35Z

@timtebeek

I re-opened it now.

I could probably leave a ton of more inline questions, but I will try to infer what good looks like from your changes applied on top (or responses to my existing comments).

myhau · 2026-10-20T12:53:56Z

+                            if (md.isConstructor() || md.getMethodType() == null) {
+                                return null;
+                            }
+                            parameter = methodCursor.computeMessageIfAbsent(MSG_NEW_PARAMETER, k ->


I'm not sure about this one.

We can walk the body just once, but I haven't seen Cursor messages used in such a way much (new LST element constructed through JavaTemplate). I keep coming back to this one feeling this might not be the best idea.

Please let me know what you think.

I'm indeed surprised to see an element added to a cursor message, considering that we need to provide a cursor when applying a template that's then potentially different where this is retrieved and swapped in. Makes the code somewhat hard to follow and fragile to changes. Did you consider or explore alternatives?

myhau · 2026-10-20T12:55:30Z

+                        if (transactionManager == null) {
+                            return mi;
+                        }
+                        return JavaTemplate.builder("#{any()}, #{any(" + PLATFORM_TRANSACTION_MANAGER + ")}")


I had a version of this without any(), but with something like:

JavaTemplate.builder("#{any(" + typeExtractedFromTaskletChunkArgument + ")}, #{any(" + PLATFORM_TRANSACTION_MANAGER + ")}")

Which one would you prefer?

So the type arguments in the template string help avoid ambiguity, or missed attribution where the isolated Java fragment that we compile can't attribute types otherwise. In general a more specific Java template string is preferred for that reason, especially when dealing with types from third party libraries; it can be omitted in cases where JDK types are used.

myhau · 2026-10-20T13:09:28Z

+                              .build();
+                      return stepBuilderFactory.get("unconfigured")
+                              .tasklet(tasklet, transactionManager)
                              .build();


Note:

This recipe on main does the same.

In theory, we could try to fix this one too (use existingManager instead of transactionManager), but here I started thinking about all kinds of other edge cases we could run into and decided to leave as is. Let me know if we should fix this one too.

What hesitations did you have to reuse the field from the wider scope? Because seeing a parameter introduced where there's a field in scope would surprise me as a user of the recipe.

Hard to say how prevalent this would be to be worth the trouble though; so perhaps best to scan if this pattern of having a field is likely before spending considerable time.

timtebeek · 2026-10-21T12:36:41Z

I've done a quick walk through looking mostly at your comments only; once those are addressed I'll do a deeper pass through the code. I've not pushed up changes this time since you're asking to learn.

myhau added 2 commits October 19, 2026 10:16

moderne-meeseeks Bot added this to OpenRewrite Oct 19, 2026

github-project-automation Bot moved this to In Progress in OpenRewrite Oct 19, 2026

moderne-meeseeks Bot assigned myhau Oct 19, 2026