MaybeAddJakartaServletApi - does not actually respect its DoesNotIncludeDependency precondition

[nmck257]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.1.5
• Maven/Gradle plugin v1.2.3 RewriteTest
• rewrite-migrate-java v2.0.6

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

import org.junit.jupiter.api.Test;
import org.openrewrite.java.JavaParser;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.java.Assertions.mavenProject;
import static org.openrewrite.java.Assertions.srcMainJava;
import static org.openrewrite.maven.Assertions.pomXml;

class MaybeAddJakartaServletApiTest implements RewriteTest {
    @Override
    public void defaults(RecipeSpec spec) {
        spec.recipe(new MaybeAddJakartaServletApi())
                .parser(JavaParser.fromJavaVersion().dependsOn("package javax.servlet;\npublic class Filter {}")));
    }

    @Test
    void hasSpringBootStarterWeb() {
        rewriteRun(
                mavenProject("my-project",
                        srcMainJava(java("""
                        import javax.servlet.Filter;
                        class A {
                            Filter foo = null;
                        }
                        """)),
                        pomXml("""
                        <?xml version="1.0" encoding="UTF-8"?>
                        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
                          <modelVersion>4.0.0</modelVersion>
                          <groupId>org.sample</groupId>
                          <artifactId>sample</artifactId>
                          <version>1.0.0</version>

                          <dependencies>
                            <dependency>
                              <groupId>org.springframework.boot</groupId>
                              <artifactId>spring-boot-starter-web</artifactId>
                              <version>2.7.0</version>
                            </dependency>
                          </dependencies>

                        </project>
                        """)
                )
        );
    }

    @Test
    void doesNotHaveSpringBootStarterWeb() {
        rewriteRun(
                mavenProject("my-project",
                        srcMainJava(java("""
                        import javax.servlet.Filter;
                        class A {
                            Filter foo = null;
                        }
                        """)),
                        pomXml("""
                        <?xml version="1.0" encoding="UTF-8"?>
                        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
                          <modelVersion>4.0.0</modelVersion>
                          <groupId>org.sample</groupId>
                          <artifactId>sample</artifactId>
                          <version>1.0.0</version>

                          <dependencies>
                          </dependencies>

                        </project>
                                """,
                        """
                        <?xml version="1.0" encoding="UTF-8"?>
                        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
                          <modelVersion>4.0.0</modelVersion>
                          <groupId>org.sample</groupId>
                          <artifactId>sample</artifactId>
                          <version>1.0.0</version>

                          <dependencies>
                            <dependency>
                              <groupId>jakarta.servlet</groupId>
                              <artifactId>jakarta.servlet-api</artifactId>
                              <version>6.0.0</version>
                            </dependency>
                          </dependencies>

                        </project>
                        """)
                )
        );
    }
}

What did you expect to see?

Passing tests

What did you see instead?

hasSpringBootStarterWeb fails, having added jakarta.servlet-api

Are you interested in contributing a fix to OpenRewrite?

Yeah. I thought it would be trivial (moving the AddDependency invocation from getRecipeList into the getVisitor), but, that actually doesn't work, because AddDependency is a ScanningRecipe. So, looks like it needs a bigger refactor to turn MaybeAddJakartaServletApi into a ScanningRecipe which composes with AddDependency. Still planning to contribute that (larger) fix.

[knutwannheden]

AFAICT the AddDependency recipe is a scacnning recipe because of its onlyIfUsing option which it then also uses to figure out the scope (or configuration in the case of Gradle) to add the dependency to. But it would seem like the lower-level operation to add a dependency (using a given scope) could in itself also be useful to have as a reusable visitor.

@nmck257 If that were the case, would the MaybeAddJakartaServletApi still have to be a scanning recipe (I haven't checked the details here)?

[nmck257]

I think AddDependencyVisitor already is set up as a reusable visitor, and yes, if MaybeAddJakartaServletApi used it directly then MaybeAddJakartaServletApi could be a non-scanning recipe and just use DoesNotIncludeDependency as a precondition basically as currently written.

Buuut we'd have to give up the onlyIfUsing check, and the scope inference. At a glance, I think that would make the Java 17 recipe unconditionally add the jakarta.servlet-api dependency, which seems worse.

On my local, I've drafted the relevant code to make MaybeAddJakartaServletApi a ScanningRecipe, and also experimented with an abstract CompositeScanningRecipe to make this pattern a little more friendly for reuse. I should be able to push some PRs next week (currently delayed by some local build environment issues).

@knutwannheden

[knutwannheden]

Sounds good. I still suspect there could be cases where it would be useful to have a visitor to add a dependency, when the calling recipe already knows the scope and that it needs to be added (e.g. because it itself just added a use of the type which onlyIfUsing would be scanning for).

[nmck257]

Agreed, those scenarios likely exist -- I think we already have the code in place for that, though: https://github.com/openrewrite/rewrite/blob/main/rewrite-maven/src/main/java/org/openrewrite/maven/AddDependencyVisitor.java

[knutwannheden]

Yes, you are right, that is what I was looking for!