Skip to content
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.

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

Spring Batch recipe for MigrateStepBuilderFactory #284

Merged
merged 32 commits into from
Aug 11, 2024

Conversation

desprez
Copy link
Contributor

@desprez desprez commented Feb 10, 2023

TEMPLATE_STOP tell me that something is missing by I don't know what

@Bean
Step myStep(JobRepository jobRepository) {
    return new StepBuilder("myStep", jobRepository)/*__TEMPLATE_STOP__*/
            ./*__TEMPLATE_STOP__*/<String, String>chunk(completionPolicy())
            .<String, String> chunk(completionPolicy())
            .reader(reader())
            .writer(writer())
            .build();
}

@desprez desprez marked this pull request as draft February 10, 2023 14:07
@timtebeek timtebeek added the recipe Recipe requested label Feb 10, 2023
@sambsnyd
Copy link
Member

sambsnyd commented Feb 13, 2023

Sorry I haven't had time to look at this yet, but I haven't forgotten

@timtebeek timtebeek added the question Further information is requested label Feb 24, 2023
@timtebeek timtebeek requested a review from sambsnyd March 28, 2023 08:39
@alexpm-14
Copy link

Hello, I'm interested in this recipe as SpringBatch4To5Migration doesn't contains this migration yet. Is it pending of anything?
Thanks in advance.

@timtebeek timtebeek changed the title Springbatch MigrateStepBuilderFactory recipe draft Spring Batch recipe for MigrateStepBuilderFactory Sep 14, 2023
@timtebeek
Copy link
Contributor

Hello, I'm interested in this recipe as SpringBatch4To5Migration doesn't contains this migration yet. Is it pending of anything? Thanks in advance.

Hi @alexpm-14 ; I'd missed this comment as I wasn't subscribed to this thread yet; I've gone ahead and made some changes to adopt rewrite 8; that hasn't yet solved the template errors reported before, but at least one of the three tests now only has type issues, no longer any template issues, so there is some progress.

As for what's needed to complete this one: I don't exactly know; I'd have to dive in more, but don't currently have the time. You're welcome to explore if you're up for it; you can fork https://github.com/desprez/rewrite-spring/tree/main and see if that get's you closer. Feel free to tag me if you make a PR there or here, and I can try to work in a review.

@timtebeek timtebeek removed the question Further information is requested label Sep 14, 2023
@timtebeek
Copy link
Contributor

@knutwannheden Perhaps you'd have an intuitive understanding of why we see template stop introduced here? A quick look would be appreciated if you're able to fit that in.

doAfterVisit(new RemoveStepBuilderFactoryVisitor(clazz, enclosingMethod));

JavaTemplate template = JavaTemplate
.builder("new StepBuilder(#{any(java.lang.String)}, jobRepository)")
Copy link
Contributor

Choose a reason for hiding this comment

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

From looking at the problem briefly it appears this jobRepository variable does not yet exist in the context when the template is applied, because that's only added as a method argument in doAfterVisit(new RemoveStepBuilderFactoryVisitor(clazz, enclosingMethod)). That's then likely what triggers the template error, even if the produced output would otherwise textually be ok. It might help to introduce the method argument first, before using it here.

timtebeek and others added 2 commits February 10, 2024 20:43
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Reworked this a bit to ensure the JobRepository is now injected as a bean method argument before we use it in the new StepBuilder. Feel like I'm close but can't work out the final step.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

@timtebeek
Copy link
Contributor

Isolated the issue into this PR:

timtebeek added a commit to openrewrite/rewrite that referenced this pull request Aug 11, 2024
* Isolate JavaTemplate issue with generic type parameters

- For openrewrite/rewrite-spring#284

* Appease the bot

* Inline template method `if`

* Also print preceding comments for J prefixes

* Look at padding before methodInvocation too

* Clear out unintentional whitespace

As this trips up the auto format in IntelliJ

* Restore intentional newlines in `changeFieldToMethod`

* Remove duplicate import for test template
@timtebeek timtebeek marked this pull request as ready for review August 11, 2024 13:44
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off @desprez , and you patience throughout. Wasn't easy to get this one across the line, given the JavaTemplate issue upstream, but we managed to polish this up into something I'm quite pleased with now.

@timtebeek timtebeek merged commit 3706324 into openrewrite:main Aug 11, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants