Closed abccbaandy closed 1 month ago
Hi @abccbaandy ; thanks for the report! Good to see the recipes that made changes included here; that helps reference the code. The dependency is added from this recipe step https://github.com/openrewrite/rewrite-migrate-java/blob/64f866913746c5b9f63d496c77540d156dc12c46/src/main/resources/META-INF/rewrite/add-common-annotations-dependencies.yml#L35-L40
Notice how it's constrained to only add the dependency if you are using anything from the javax.annotation..*
package. Can you confirm that is indeed the case?
We also should not add the dependency if it's already present transitively; that part might be sensitive to recipe execution order.
Could you let me know why you feel we should not add that dependency to your project in this case? Is it because you were already running successfully on a more recent Java & Spring version without? Or is the dependency already available transitively but not picked up some how?
Hi @timtebeek , the Spring boot version was 2.X, so yes, I am use javax.annotation. And the recipes change all of them to jakarta.annotation <= This is correct too.
But for the jakarta.annotation:jakarta.annotation-api:1.3.5
part. This is wrong because Spring boot 3.2.5 use jakarta.annotation:jakarta.annotation-api:2.1.1
ref: https://mvnrepository.com/artifact/org.springframework.boot/spring-boot-starter/3.2.5
And 1.3.5 don't have jakarta.annotation
package.
So after the recipes run finish, I got jakarta.annotation in my code with jakarta.annotation-api:1.3.5 dependency which cause build fail.
Also, since the spring boot starter contain the jakarta.annotation dependency and maintain the version already, I don't think it's right to add jakarta relate dependency,
Thanks for that additional context! I think I have an understanding now of what's going on.
We recently added an explicit recipe to change or add jakarta.annotation-api
as needed for folks that migrate to Java 11.
The Java 11 migration is also run as part of a Spring Boot 3 migration, as that includes Java 17 migration, which includes Java 11. These recipes are run in sequence, each performing the step necessary at that time. That means we need to add the dependency when going from 8 to 11, as at that point the imports have not yet changed. When we go from 11 to 17 and Jakarta though, we should remove that dependency again. It's only necessary for folks that wish to stay on Java 11 for whatever reason.
I imagine we could add a recipe to org.openrewrite.java.migrate.jakarta.JavaxMigrationToJakarta
to remove explicit jakarta.annotation-api
dependencies again if they also come in transitively. For that conditional we could use a precondition using DependencyInsight. Effectively that should then not result in any added dependency when folks leap from 8 to 17 + Jakarta.
Is this something you'd already be comfortable adding to src/main/resources/META-INF/rewrite/jakarta-ee-9.yml ? Even a draft PR with a unit test would be welcome; if not I'll try to fit it in but that could be a little while, as it's hard to get to everything.
I see you've started working toward this @abccbaandy ; thanks a lot! Feel free to open a draft pull request such that I can help out if needed.
lol how do you found that? Do you get any notify if anyone fork the project?
Anyways, I open a PR with some question, please help. Thanks.
Hi @timtebeek I have some new thought about this issue after study the source.
The DependencyInsight seems not support non-ResolvedDependency.
Means when we have direct dependency, the transitive dependency with the same name is gone.
ref
https://github.com/openrewrite/rewrite/blob/main/rewrite-maven/src/main/java/org/openrewrite/maven/search/DependencyInsight.java#L119
So, I guess this issue cause DependencyInsight onlyTransitive
feature not possible.
I found there is an issue with similar problem. He just checks if the spring exist then bypass the add dependency
applicability:
singleSource:
- org.openrewrite.maven.search.DoesNotIncludeDependency:
groupId: org.springframework.boot
artifactId: spring-boot-starter-web
https://github.com/nmck257/rewrite-migrate-java/blob/29319b419d1daa228abd8ebc6958b1f3f725a8b2/src/main/resources/META-INF/rewrite/jakarta-ee-9.yml#L696 ori PR: https://github.com/openrewrite/rewrite-migrate-java/pull/196
But it got removed in the latest master, I don't know why.
Can we use the same way to modify the org.openrewrite.java.migrate.javax.AddCommonAnnotationsDependencies
recipe?
Btw, I don't know why DoesNotIncludeDependency only available in maven, but I think we can use DependencyInsight to check if spring exist?
Ah yes single-source applicability tests is what we called preconditions in Rewrite v7; I suspect it was dropped as we moved to Rewrite v8 and not revisited since. I can see that DoesNotIncludeDependency recipe itself does use DependencyInsight, with a hint about use with multi module projects. https://github.com/openrewrite/rewrite/blob/74faca04cd0ffbbb3e4347c86460feb4e1ea139e/rewrite-maven/src/main/java/org/openrewrite/maven/search/DoesNotIncludeDependency.java#L66-L68
If we can make that work well as a precondition to check for the absence/presence of a transitive(!) dependency, for instance by looking at the MavenResolutionResult
marker, then we could use this before we remove the dependency. I'd need to look a little deeper than I have time for this week, but hope the context helps you explore there.
What version of OpenRewrite are you using?
I am using IntelliJ IDEA OpenRewrite feature, guess it's latest version? It didn't print the version :(
How are you running OpenRewrite?
with this recipes only
What is the smallest, simplest way to reproduce the problem?
N/A
What did you expect to see?
Noting.
What did you see instead?
in build.gradle, it add this line
What is the full stack trace of any errors you encountered?
N/A But here is the log relate to this issue
Are you interested in contributing a fix to OpenRewrite?
Sure, let me know what can I do.