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

RemoveDependency doesn't remove the dependency if one of its part is defined with properties #4796

Open
svaningelgem opened this issue Dec 18, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@svaningelgem
Copy link
Contributor

What version of OpenRewrite are you using?

  • OpenRewrite master branch

How are you running OpenRewrite?

via the test suite, adding this test in java/org/openrewrite/maven/RemoveDependencyTest.java:

    @Test
    void removeDependencyDefinedWithProperties() {
        rewriteRun(
          spec -> spec.recipe(new RemoveDependency("junit", "junit", null)),
          pomXml(
            """
              <project>
                <modelVersion>4.0.0</modelVersion>
              
                <groupId>com.mycompany.app</groupId>
                <artifactId>my-app</artifactId>
                <version>1</version>
              
                <properties>
                  <junit.groupId>junit</junit.groupId>
                </properties>
              
                <dependencies>
                  <dependency>
                    <groupId>com.google.guava</groupId>
                    <artifactId>guava</artifactId>
                    <version>29.0-jre</version>
                  </dependency>
                  <dependency>
                    <groupId>${junit.groupId}</groupId>
                    <artifactId>junit</artifactId>
                    <version>4.13.1</version>
                    <scope>test</scope>
                  </dependency>
                </dependencies>
              </project>
              """,
            """
              <project>
                <modelVersion>4.0.0</modelVersion>
                
                <groupId>com.mycompany.app</groupId>
                <artifactId>my-app</artifactId>
                <version>1</version>
                
                <dependencies>
                  <dependency>
                    <groupId>com.google.guava</groupId>
                    <artifactId>guava</artifactId>
                    <version>29.0-jre</version>
                  </dependency>
                </dependencies>
              </project>
              """
          )
        );
    }

What did you expect to see?

That the test works :-)

What did you see instead?

That there were no changes to the template, even though I expected junit to be removed.

What is the full stack trace of any errors you encountered?

No stacktrace, just no changes done.

Are you interested in contributing a fix to OpenRewrite?

I'll try to look into it.

@svaningelgem svaningelgem added the bug Something isn't working label Dec 18, 2024
@svaningelgem
Copy link
Contributor Author

The issue is that inside MavenVisitor::findDependency, it will call tag.getChildValue. Which will return the literal child value. And not the one interpreted with properties.
I think this tag.getChildValue method should interpret properties to return the real value.

However, this is going very deep inside the XML reading features and might have numerous repercussions.

What would your advice be?

@timtebeek timtebeek moved this to Backlog in OpenRewrite Dec 19, 2024
@timtebeek
Copy link
Contributor

We saw a possibly similar change today when changing annotation processors in 000a4f0. Look for the getResolutionResult().getPom().getValue(oldVersion) to see how we resolve placeholders fairly easily from values that might contain placeholders. I hope that helps & all the best going into 2025!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants