Deprecated StepBuilderFactory/JobBuilderFactory types left alone when it's not used in same Java file

[dcsekar]

While using the "org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_4", we expected that it will remove StepBuilderFactory and JobBuilderFactory or change it to StepBuilder/JobBuilder. When the deprecated StepBuilderFactory/JobBuilderFactory instances are defined and if it's not used in the same Java file, then it's not getting removed.
But when the same java file has some function using that instance say "Job myJob(Step step) { return this.jobBuilderFactory.get ... }" , then it's getting changed to "Job myJob(Step step, JobRepository jobRepository)" and these instances are removed.

What problem are you trying to solve?

After the run of UpgradeSpringBoot_3_4 the StepBuilderFactory and JobBuilderFactory instances should be removed.

What version of OpenRewrite are you using?

org.openrewrite.recipe:rewrite-spring latest version

How are you running OpenRewrite?

Using Maven multi-module project.

What is the smallest, simplest way to reproduce the problem?

import org.springframework.batch.core.configuration.annotation.JobBuilderFactory;
import org.springframework.batch.core.configuration.annotation.StepBuilderFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Configuration;

public class StepConfig {
    protected StepBuilderFactory stepBuilderFactory;

    protected JobBuilderFactory jobBuilderFactory;
}

What did you expect to see?

Basically the content is not changed as expected. Either it can remove the stepBuilderFactory/jobBuilderFactory and change type to StepBuilder/JobBuilder

What did you see instead?

import org.springframework.batch.core.configuration.annotation.JobBuilderFactory;
import org.springframework.batch.core.configuration.annotation.StepBuilderFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Configuration;

public class StepConfig {
    protected StepBuilderFactory stepBuilderFactory;

    protected JobBuilderFactory jobBuilderFactory;
}

@Issue("https://github.com/openrewrite/rewrite-spring/issues/788")
@Test
void replaceBuilderFactoryTest() {
    rewriteRun(spec -> spec.recipeFromResources("org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_4"),
      java(
        """
          package com.example.batch;

          import org.springframework.batch.core.configuration.annotation.JobBuilderFactory;
          import org.springframework.batch.core.configuration.annotation.StepBuilderFactory;
          import org.springframework.beans.factory.annotation.Autowired;
          import org.springframework.context.annotation.Configuration;

          public class StepConfig {
              protected StepBuilderFactory stepBuilderFactory;

              protected JobBuilderFactory jobBuilderFactory;
          }
          """, spec -> spec.path("StepConfig.java")
      ),
      java(
        """
          package com.example.batch;

          import org.springframework.batch.core.Job;
          import org.springframework.batch.core.Step;
          import org.springframework.beans.factory.annotation.Autowired;
          import org.springframework.context.annotation.Bean;
          import org.springframework.context.annotation.Configuration;

          @Configuration
          public class StepConfigExt extends StepConfig {

              @Bean
              Job myJob(Step step) {
                  return this.jobBuilderFactory.get("myJob")
                      .start(step)
                      .build();
              }
          }
          """
        , """
          package com.example.batch;

          import org.springframework.batch.core.Job;
          import org.springframework.batch.core.Step;
          import org.springframework.batch.core.job.builder.JobBuilder;
          import org.springframework.batch.core.repository.JobRepository;
          import org.springframework.context.annotation.Bean;
          import org.springframework.context.annotation.Configuration;

          @Configuration
          public class StepConfigExt extends StepConfig {

              @Bean
              Job myJob(Step step, JobRepository jobRepository) {
                  return new JobBuilder("myJob", jobRepository)
                      .start(step)
                      .build();
              }
          }
          """, spec -> spec.path("StepConfigEtx.java")
      )
    );
}

Are you interested in contributing a fix to OpenRewrite?

No

[timtebeek]

hi @dcsekar Indeed that case was not expected or supported by the recipe for now, as we have a precondition to only make changes in the case that the get method is present in the compilation unit:

rewrite-spring/src/main/java/org/openrewrite/java/spring/batch/MigrateStepBuilderFactory.java

Lines 48 to 59 in 2df85a6

	@Override
	public TreeVisitor<?, ExecutionContext> getVisitor() {
	return Preconditions.check(new UsesMethod<>(STEP_BUILDER_FACTORY_GET),
	new JavaVisitor<ExecutionContext>() {
	@Override
	public J visit(@Nullable Tree tree, ExecutionContext ctx) {
	tree = new AddJobRepositoryVisitor().visit(tree, ctx);
	return new NewStepBuilderVisitor().visit(tree, ctx);
	}
	}
	);
	}

It's not exactly clear what should be done in these cases, and how to do so without potentially doing harm in other aspects of the code. In such cases we then lean towards not making a code change at all, as to do no harm.

If we were to remove the fields then any constructor of StepConfig is likely to fail.
If we were to change the types from the factories to the repositories then the same constructors would fail, and those fields are not used in the replacement new JobBuilder, leading to further confusion.

I think in these presumed edge cases it makes sense for the developer to step in, unless you see this as a very common case with a guaranteed safe change we can make.