openrewrite / rewrite-spring

OpenRewrite recipes for Spring projects.
Apache License 2.0
253 stars 75 forks source link

Should not remove dependency aspectjweaver when using TimedAspect #235

Closed timtebeek closed 1 year ago

timtebeek commented 1 year ago

We have a sample app that uses TimedAspect to track durations. Fairly simple reduced example:

import io.micrometer.core.aop.TimedAspect;
import io.micrometer.core.instrument.MeterRegistry;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.EnableAspectJAutoProxy;

@SpringBootApplication
@EnableAspectJAutoProxy
public class App {
    public static void main(String[] args) {
        SpringApplication.run(App.class, args);
    }
    @Bean
    TimedAspect timedAspect(MeterRegistry registry) {
        return new TimedAspect(registry);
    }
}
    @Timed("received-messages")
    public Foo messages(Bar baz) {
        return ...;
    }

But when I run SpringBoot1To2Migration, that cascades down to UpgradeSpringFramework_5_3, which removes the aspectjweaver dependency.

[WARNING] These recipes would make changes to pom.xml:
[WARNING]     org.openrewrite.java.spring.boot2.SpringBoot1To2Migration
[WARNING]         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                         org.openrewrite.java.spring.framework.UpgradeSpringFramework_5_3
[WARNING]                             org.openrewrite.maven.RemoveDependency: {groupId=org.aspectj, artifactId=aspectjweaver}
diff --git a/pom.xml b/pom.xml
index dc1263a..8c216d4 100644
--- a/pom.xml
+++ b/pom.xml
@@ -40,10 +40,6 @@ org.openrewrite.maven.RemoveDependency
        </dependency>
        <!-- TimedAspect -->
        <dependency>
-           <groupId>org.aspectj</groupId>
-           <artifactId>aspectjweaver</artifactId>
-       </dependency>
-       <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <optional>true</optional>

That breaks the TimedAspect functionally, without breaking compilation or any of the tests. I think some conditionals might be in order before aspectjweaver gets removed.

Fairly low priority for me; but thought to log is as it interferes with our enforcement of not having any pending migration steps.

tkvangorder commented 1 year ago

Thanks for reporting, I finally had some time to look at this.

We were definitely too aggressive with the removal of aspectjweaver. It is mentioned in the spring docs that this .jar should be included on the classpath.

This has been fixed.