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.

Sign up for GitHub

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

Apply add dependency correctly, taking aggregating poms into account #4590

Merged
merged 16 commits into from
Oct 26, 2024
Merged

Apply add dependency correctly, taking aggregating poms into account #4590

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d82abba
WIP adding a mechanism to detect aggregating poms and only adding dep…
Laurens-W Oct 18, 2024
1ec3ca8
Test case with correct expectations
Laurens-W Oct 18, 2024
99b3781
Remove empty comment
Laurens-W Oct 18, 2024
030e874
Format
Laurens-W Oct 18, 2024
f0aa14a
Adjust test case and logic
Laurens-W Oct 21, 2024
59ee945
Merge branch 'main' into add-dependency-to-parent-pom
timtebeek Oct 21, 2024
9ed76d2
Refactor to maven4 `subProjects` naming and facilitate old maven modu…
Laurens-W Oct 21, 2024
db659db
Refactor isAggregatingPom
Laurens-W Oct 21, 2024
fc68b9a
De stream and fix FMT
Laurens-W Oct 22, 2024
c2010ac
Add testcase for RawPom
Laurens-W Oct 22, 2024
d1dedf9
Accidentally inverted logic
Laurens-W Oct 22, 2024
14a7f0b
Merge branch 'main' into add-dependency-to-parent-pom
timtebeek Oct 22, 2024
08ded76
Merge branch 'main' into add-dependency-to-parent-pom
timtebeek Oct 22, 2024
3b942ce
Apply formatter to tests
timtebeek Oct 22, 2024
19e364f
Change method name to something more recognizable for maven users
Laurens-W Oct 24, 2024
004e84c
Extract named methods to label the new conditionals
timtebeek Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions rewrite-maven/src/main/java/org/openrewrite/maven/AddDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@
import org.openrewrite.semver.Semver;
import org.openrewrite.xml.tree.Xml;

import java.util.*;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;

import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -180,17 +183,17 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
acc.usingType = true;
JavaProject javaProject = sourceFile.getMarkers().findFirst(JavaProject.class).orElse(null);
JavaSourceSet javaSourceSet = sourceFile.getMarkers().findFirst(JavaSourceSet.class).orElse(null);
if(javaProject != null && javaSourceSet != null) {
if (javaProject != null && javaSourceSet != null) {
acc.scopeByProject.compute(javaProject, (jp, scope) -> "compile".equals(scope) ?
scope /* a `compile` scope dependency will also be available in test source set */ :
"test".equals(javaSourceSet.getName()) ? "test" : "compile"
);
}
}
} else if(tree instanceof Xml.Document) {
} else if (tree instanceof Xml.Document) {
Xml.Document doc = (Xml.Document) tree;
MavenResolutionResult mrr = doc.getMarkers().findFirst(MavenResolutionResult.class).orElse(null);
if(mrr == null) {
if (mrr == null) {
return sourceFile;
}
acc.pomsDefinedInCurrentRepository.add(mrr.getPom().getGav());
Expand Down Expand Up @@ -235,7 +238,14 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
}
}

if(onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav())) {
if (onlyIfUsing == null && getResolutionResult().getParent() != null && acc.pomsDefinedInCurrentRepository.contains(getResolutionResult().getParent().getPom().getGav())) {
return maven;
}

if (!getResolutionResult().getPom().getModules().isEmpty()
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
&& (getResolutionResult().getModules().isEmpty()
|| getResolutionResult().getModules().stream().map(MavenResolutionResult::getPom).map(ResolvedPom::getGav).map(ResolvedGroupArtifactVersion::getArtifactId).noneMatch(art -> getResolutionResult().getPom().getModules().contains(art))
)) {
return maven;
}

Expand Down
27 changes: 22 additions & 5 deletions rewrite-maven/src/main/java/org/openrewrite/maven/internal/RawPom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ public class RawPom {
@Nullable
Profiles profiles;

@Nullable
Modules modules;

public static RawPom parse(InputStream inputStream, @Nullable String snapshotVersion) {
try {
RawPom pom = MavenXmlMapper.readMapper().readValue(inputStream, RawPom.class);
Expand Down Expand Up @@ -217,6 +220,19 @@ public Profiles(@JacksonXmlProperty(localName = "profile") List<Profile> profile
}
}

@Getter
public static class Modules {
private final List<String> modules;

public Modules() {
this.modules = emptyList();
}

public Modules(@JacksonXmlProperty(localName = "module") List<String> modules) {
this.modules = modules;
}
}

@FieldDefaults(level = AccessLevel.PRIVATE)
@Data
public static class Build {
Expand Down Expand Up @@ -346,9 +362,9 @@ public static class Profile {
}

public @Nullable String getVersion() {
if(version == null) {
if(currentVersion == null) {
if(parent == null) {
if (version == null) {
if (currentVersion == null) {
if (parent == null) {
return null;
} else {
return parent.getVersion();
Expand Down Expand Up @@ -382,8 +398,9 @@ public Pom toPom(@Nullable Path inputPath, @Nullable MavenRepository repo) {
.packaging(packaging)
.properties(getProperties() == null ? emptyMap() : getProperties())
.licenses(mapLicenses(getLicenses()))
.profiles(mapProfiles(getProfiles()));
if(StringUtils.isBlank(pomVersion)) {
.profiles(mapProfiles(getProfiles()))
.modules(getModules() == null ? emptyList() : getModules().getModules());
if (StringUtils.isBlank(pomVersion)) {
builder.dependencies(mapRequestedDependencies(getDependencies()))
.dependencyManagement(mapDependencyManagement(getDependencyManagement()))
.repositories(mapRepositories(getRepositories()))
Expand Down
16 changes: 16 additions & 0 deletions rewrite-maven/src/main/java/org/openrewrite/maven/tree/MavenResolutionResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,22 @@ public boolean parentPomIsProjectPom() {
.anyMatch(gav -> gav.equals(parentGav));
}

/**
* Often recipes operating on multi-module projects will prefer to make changes to the parent pom rather than in multiple child poms.
* But if the parent isn't in the same repository as the child, the recipe would need to be applied to the child poms.
*
* @return true when the parent pom of the current pom is present in the same repository as the current pom
*/
public boolean isAggregatingPom() {
if (getParent() == null) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
ResolvedGroupArtifactVersion parentGav = getParent().getPom().getGav();
return getProjectPoms().values().stream()
.map(Pom::getGav)
.anyMatch(gav -> gav.equals(parentGav));
}

private Map<Path, Pom> getProjectPomsRecursive(Map<Path, Pom> projectPoms) {
projectPoms.put(requireNonNull(pom.getRequested().getSourcePath()), pom.getRequested());
if (parent != null) {
Expand Down
6 changes: 5 additions & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/tree/Pom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ public static int getModelVersion() {
@Builder.Default
List<Plugin> pluginManagement = emptyList();

@Builder.Default
List<String> modules = emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion & future changes, perhaps it helps to already adopt the Maven 4 convention of calling these subprojects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, that takes away the ambiguity!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken both modules and subProjects into account for the RawPom and named it subProjects anywhere beyond there


public String getGroupId() {
return gav.getGroupId();
}
Expand Down Expand Up @@ -184,7 +187,8 @@ public ResolvedPom resolve(Iterable<String> activeProfiles,
repositories,
dependencies,
plugins,
pluginManagement)
pluginManagement,
modules)
.resolve(ctx, downloader);
}

Expand Down
12 changes: 9 additions & 3 deletions rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ public class ResolvedPom {
Iterable<String> activeProfiles;

public ResolvedPom(Pom requested, Iterable<String> activeProfiles) {
this(requested, activeProfiles, emptyMap(), emptyList(), null, emptyList(), emptyList(), emptyList(), emptyList());
this(requested, activeProfiles, emptyMap(), emptyList(), null, emptyList(), emptyList(), emptyList(), emptyList(), emptyList());
}

@JsonCreator
ResolvedPom(Pom requested, Iterable<String> activeProfiles, Map<String, String> properties, List<ResolvedManagedDependency> dependencyManagement, @Nullable List<MavenRepository> initialRepositories, List<MavenRepository> repositories, List<Dependency> requestedDependencies, List<Plugin> plugins, List<Plugin> pluginManagement) {
ResolvedPom(Pom requested, Iterable<String> activeProfiles, Map<String, String> properties, List<ResolvedManagedDependency> dependencyManagement, @Nullable List<MavenRepository> initialRepositories, List<MavenRepository> repositories, List<Dependency> requestedDependencies, List<Plugin> plugins, List<Plugin> pluginManagement, List<String> modules) {
this.requested = requested;
this.activeProfiles = activeProfiles;
this.properties = properties;
Expand All @@ -83,6 +83,7 @@ public ResolvedPom(Pom requested, Iterable<String> activeProfiles) {
this.requestedDependencies = requestedDependencies;
this.plugins = plugins;
this.pluginManagement = pluginManagement;
this.modules = modules;
}

@NonFinal
Expand Down Expand Up @@ -113,6 +114,10 @@ public ResolvedPom(Pom requested, Iterable<String> activeProfiles) {
@Builder.Default
List<Plugin> pluginManagement = emptyList();

@NonFinal
@Builder.Default
List<String> modules = emptyList();


/**
* Deduplicate dependencies and dependency management dependencies
Expand Down Expand Up @@ -172,6 +177,7 @@ public ResolvedPom resolve(ExecutionContext ctx, MavenPomDownloader downloader)
emptyList(),
emptyList(),
emptyList(),
emptyList(),
emptyList()
).resolver(ctx, downloader).resolve();

Expand Down Expand Up @@ -926,7 +932,7 @@ public List<ResolvedDependency> resolveDependencies(Scope scope, Map<GroupArtifa
ResolvedPom resolvedPom = cache.getResolvedDependencyPom(dPom.getGav());
if (resolvedPom == null) {
resolvedPom = new ResolvedPom(dPom, getActiveProfiles(), emptyMap(),
emptyList(), initialRepositories, emptyList(), emptyList(), emptyList(), emptyList());
emptyList(), initialRepositories, emptyList(), emptyList(), emptyList(), emptyList(), emptyList());
resolvedPom.resolver(ctx, downloader).resolveParentsRecursively(dPom);
cache.putResolvedDependencyPom(dPom.getGav(), resolvedPom);
}
Expand Down
185 changes: 182 additions & 3 deletions rewrite-maven/src/test/java/org/openrewrite/maven/AddDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,9 +1055,6 @@ void preferRootPom() {
)
),
mavenProject("project1",
srcMainJava(
java(usingGuavaIntMath)
),
pomXml(
"""
<project>
Expand Down Expand Up @@ -1464,6 +1461,188 @@ class Foo {}
);
}

@Test
void addDependencyToParentPomWhenAggregatingPomIsNotParent() {
rewriteRun(
spec -> spec.recipe(new AddDependency(
"org.hamcrest",
"hamcrest-junit",
"2.0.0.0",
null,
"test",
null,
null,
null,
null,
null,
null,
true)),
mavenProject("my-app-aggregate",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-aggregate</artifactId>
<version>1</version>
<modules>
<module>project-parent</module>
<module>project-child</module>
</modules>
</project>
""")),
mavenProject("project-parent",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-parent</artifactId>
<version>1</version>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>3.1.5</version>
</parent>
</project>
""",
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-parent</artifactId>
<version>1</version>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>3.1.5</version>
</parent>
<dependencies>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-junit</artifactId>
<version>2.0.0.0</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
""")
),
mavenProject("my-app-child",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-child</artifactId>
<version>1</version>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-parent</artifactId>
<version>1</version>
</parent>
</project>
"""))
);
}

@Test
void addDependencyToAggregatingPomAndParentPomWhenAggregatingPomIsParent() {
rewriteRun(
spec -> spec.recipe(new AddDependency(
"org.hamcrest",
"hamcrest-junit",
"2.0.0.0",
null,
"test",
null,
null,
null,
null,
null,
null,
true)),
mavenProject("my-app-aggregate",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-aggregate</artifactId>
<version>1</version>
<modules>
<module>my-app-parent</module>
<module>my-app-child</module>
</modules>
</project>
""",
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-aggregate</artifactId>
<version>1</version>
<modules>
<module>my-app-parent</module>
<module>my-app-child</module>
</modules>
<dependencies>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-junit</artifactId>
<version>2.0.0.0</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
""")),
mavenProject("project-parent",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-parent</artifactId>
<version>1</version>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>3.1.5</version>
</parent>
</project>
""",

"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-parent</artifactId>
<version>1</version>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>3.1.5</version>
</parent>
<dependencies>
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-junit</artifactId>
<version>2.0.0.0</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
""")
),
mavenProject("my-app-child",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-child</artifactId>
<version>1</version>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app-aggregate</artifactId>
<version>1</version>
</parent>
</project>
"""))
);
}

private AddDependency addDependency(@SuppressWarnings("SameParameterValue") String gav) {
return addDependency(gav, null, null, null);
}
Expand Down