rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

Add Dependecies only in specific modules

Open marcel-gepardec opened this issue 1 year ago • 4 comments

A new option to specify a module name in the existing AddDependency recipe has been added. There the dependency will only be added in the specific module.

What's changed?

Added the option "specificModuleName" to the AddDependency Recipe. If "specificModuleName" is added, the dependency will only be added in the specific module and not in the root POM.

Example:

---
type: specs.openrewrite.org/v1beta/recipe
name: com.yourorg.AddDependencyExample
displayName: Add Maven dependency example
recipeList:
    - org.openrewrite.maven.AddDependency:
          groupId: javax.persistence
          artifactId: javax.persistence-api
          version: 2.2
          specificModuleName: test1

Before:

<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>
    <parent>
        <groupId>org.example</groupId>
        <artifactId>rootTest</artifactId>
        <version>${revision}</version>
        <relativePath>../pom.xml</relativePath>
    </parent>
    <artifactId>test1</artifactId>
    <packaging>pom</packaging>
</project>

After:

<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>
    <parent>
        <groupId>org.example</groupId>
        <artifactId>rootTest</artifactId>
        <version>${revision}</version>
        <relativePath>../pom.xml</relativePath>
    </parent>
    <artifactId>test1</artifactId>
    <packaging>pom</packaging>
    <dependencies>
        <dependency>
            <groupId>javax.persistence</groupId>
            <artifactId>javax.persistence-api</artifactId>
            <version>2.2</version>
        </dependency>
    </dependencies>
</project>

What's your motivation?

A work colleague discovered that the option "onlyIfUsing" is not very consistent and will miss some types. So the solution was to add an option where you can specifically mention the module name in which the dependency should be added.

marcel-gepardec avatar Aug 28 '24 10:08 marcel-gepardec

Hmm; wouldn't this be better served with a precondition? We've introduced that concept to prevent us from having to add similar options to multiple recipes, for the long term maintainability of the project.

In general we want recipes to be able to run with the same configuration across multiple projects. An option like specificModuleName makes this hyper specific to a single project, or projects following a very strict layout. That overfit is something we'd like to avoid.

timtebeek avatar Aug 28 '24 11:08 timtebeek

Hi @timtebeek , the condition onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav()) in AddDependency Line 215 prevents the dependency from being added in some cases.

https://github.com/openrewrite/rewrite/blob/b64e1ed56584a03173c55abb831208af6e95afa3/rewrite-maven/src/main/java/org/openrewrite/maven/AddDependency.java#L238-L240

I tried to use it with a precondition at first. However, because I can't specify onlyIfUsing and my parent POM actually is defined in my current repository, the dependency never gets created. We thought, this would be the simplest increment without changing existing behavior. Of course, we're open for other suggestions ^^

For my own use-case, I've created my own recipe that calls the underlying AddDependencyVisitor with a precondition. At least, I could contribute my search recipe for selecting a module. Then @marcel-gepardec only has to change the condition in question.

sgartner03 avatar Aug 28 '24 13:08 sgartner03

I'm not sure fully see the use case here, but that could also be because I have limited time to dive into the specifics. I do still feel that adding specificModuleName might not be the answer though; again: because recipes ought to be run generically, not specific to any one project structure.

What's the practical use here at all of adding a dependency to a specific module without any onlyIfUsing? Could that be better served with a dedicated recipe instead as outlined above?

And don't get me wrong: I appreciate all the contributions, even if I don't always find time to review them within a week. In this case I'm mostly wondering if it even is a case we should generalize, and then maintain those options, or whether a specific in house recipe could be better.

timtebeek avatar Aug 29 '24 10:08 timtebeek

Hi, I already understood your doubts about specificModuleName. However, I am not able to just pair this recipe with a precondition for selecting a single module or multiple modules. That's because of the condition I've mentioned in my previous comment. I see two use-cases for adding a dependency without onlyIfUsing:

  • Dependencies without references in the code (Implementations of the Jakarta specification, for example)
  • Recipes that add some code using a new dependency

For these use-cases, You'd have to call AddDependencyVisitor manually in a java recipe. My proposition is to:

  • Change the condition onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav()) to allow this case
  • Contribute a search recipe for selecting maven modules.

Again, currently I can't add a dependency, when:

  • onlyIfUsing is not specified
  • AND the parent POM is defined in my repository

sgartner03 avatar Sep 03 '24 09:09 sgartner03

Appreciate the work and discussion here; it sounds like you've already found a solution using your internal AddDependencyVisitor and a precondition, and we agree the specificModuleName here is probably not something to take on. Closing this PR for now. 🙏🏻

timtebeek avatar Jan 17 '25 16:01 timtebeek