-
Notifications
You must be signed in to change notification settings - Fork 105
Add ScanningRecipe for migrate Hooks.enableAutomaticContextPropagation()
to Spring property
#772
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
base: main
Are you sure you want to change the base?
Add ScanningRecipe for migrate Hooks.enableAutomaticContextPropagation()
to Spring property
#772
Conversation
src/main/java/org/openrewrite/java/spring/boot3/MigrateHooksToReactorContextProperty.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateHooksToReactorContextProperty.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateHooksToReactorContextProperty.java
Outdated
Show resolved
Hide resolved
...oot_3_2/java/org/openrewrite/java/spring/boot3/MigrateHooksToReactorContextPropertyTest.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public AtomicBoolean getInitialValue(ExecutionContext ctx) { | ||
return new AtomicBoolean(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While perhaps not as common in practice, we'd need to correctly handle multi module projects too, which might each have this hook and a configuration properties file, or multiple. You can look at this example for how to handle that with an alternative Accumulator:
Lines 245 to 250 in e7a8b99
@Data | |
static class JavaProjects { | |
Set<JavaProject> applicableProjects = new HashSet<>(); | |
Set<JavaProject> processedProjects = new HashSet<>(); | |
Map<JavaProject, Path> sourceToCommentByProject = new HashMap<>(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start here @nurdy-kim ! I've called out a potential source of issues with the AtomicBoolean
you'd been using. That's an older pattern we're looking to phase out on new recipes. Could you make the necessary adjustments based on the example given?
Thanks for the feedback! I'll make the necessary adjustments! :) |
|
||
import static org.openrewrite.Preconditions.and; | ||
|
||
public class MigrateHooksToReactorContextProperty extends ScanningRecipe<AtomicBoolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also might be good to hook this recipe into one of the larger composites, or Spring Boot best practices, such that it's run automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recipe can be applicable from Spring Boot 3.2. I believe it would be a good fit for the Migrate to Spring Boot 3.2 recipe. If not, could you please provide some guidance on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That then makes sense, yes thanks!
What's changed?
I added a new
ScanningRecipe
-MigrateHooksToReactorContextProperty
. This recipe automatically removes theHooks.enableAutomaticContextPropagation()
method call frommain
methods within@SpringBootApplication
classes. After that it adds the newspring.reactor.context-propagation=true
property toapplication.properties
orapplication.yml
files, which provides the same functionality.Anything in particular you'd like reviewers to focus on?
I'm particularly concerned about whether the addSpringProperty method is working as expected. I'd appreciate it if you could pay close attention to that part during the review.
Anyone you would like to review specifically?
@nmck257, @timtebeek
Checklist