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

Jump to bottom

Also exclude omitted dependencies #4563

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Also exclude omitted dependencies #4563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5277e49
ExcludeDependency should also exclude omitted dependencies
ckcd Oct 9, 2024
7000b92
Merge branch 'main' into ck2410_omit_exclude
timtebeek Oct 9, 2024
4bf2eca
Merge branch 'main' into ck2410_omit_exclude
timtebeek Oct 27, 2024
9b7e294
Merge branch 'openrewrite:main' into ck2410_omit_exclude
SiBorea Nov 5, 2024
a8b50a6
Merge branch 'main' into ck2410_omit_exclude
timtebeek Nov 14, 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
2 changes: 1 addition & 1 deletion rewrite-gradle/src/main/java/org/openrewrite/gradle/AddDependencyVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ static G.CompilationUnit addDependency(
}
return resolved;
}),
new ResolvedDependency(null, resolvedGav, newRequested, transitiveDependencies,
new ResolvedDependency(null, resolvedGav, newRequested, transitiveDependencies, emptyList(),
emptyList(), "jar", classifier, null, 0, null)));
}
newNameToConfiguration.put(newGdc.getName(), newGdc);
Expand Down
9 changes: 7 additions & 2 deletions rewrite-maven/src/main/java/org/openrewrite/maven/ExcludeDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
ResolvedDependency dependency = findDependency(tag, scope);
if (dependency != null &&
!(matchesGlob(dependency.getGroupId(), groupId) && matchesGlob(dependency.getArtifactId(), artifactId)) &&
dependency.findDependency(groupId, artifactId) != null) {
dependency.findDependencyWithOmit(groupId, artifactId) != null) {
boolean needUpdate = false;
Optional<Xml.Tag> maybeExclusions = tag.getChild("exclusions");
if (maybeExclusions.isPresent()) {
Xml.Tag exclusions = maybeExclusions.get();
Expand All @@ -116,6 +117,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
"</exclusion>"))
.visitNonNull(exclusions, ctx, getCursor());
tag = tag.withContent(ListUtils.map((List<Content>) tag.getContent(), t -> t == exclusions ? newExclusions : t));
needUpdate = true;
}
} else {
tag = (Xml.Tag) new AddToTagVisitor<>(tag,
Expand All @@ -127,8 +129,11 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
"</exclusion>\n" +
"</exclusions>"))
.visitNonNull(tag, ctx, getCursor().getParentOrThrow());
needUpdate = true;
}
if (needUpdate) {
maybeUpdateModel();
}
maybeUpdateModel();
}
return tag;
}
Expand Down
42 changes: 42 additions & 0 deletions rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ public class ResolvedDependency implements Serializable {
@EqualsAndHashCode.Exclude
List<ResolvedDependency> dependencies;

@NonFinal
@EqualsAndHashCode.Exclude
List<GroupArtifactVersion> omitDependencies;
Comment on lines +55 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing to note here is that omitDependencies is technically @Nullable, since we serialize LSTs at Moderne, which mean folks might deserialize an older ResolvedDependency class without any omitDepedencies field, which can then lead to a null pointer exception further down. Let's make that explicit by adding that annotation here to be safe, and then handle the usage site below to guard against a null value.


List<License> licenses;

@Nullable
Expand Down Expand Up @@ -81,6 +85,10 @@ void unsafeSetDependencies(List<ResolvedDependency> dependencies) {
this.dependencies = dependencies;
}

void unsafeSetOmitDependencies(List<GroupArtifactVersion> omitDependencies) {
this.omitDependencies = omitDependencies;
}

void unsafeSetEffectiveExclusions(List<GroupArtifact> effectiveExclusions) {
this.effectiveExclusions = effectiveExclusions;
}
Expand Down Expand Up @@ -142,6 +150,40 @@ public boolean isTransitive() {
return null;
}

public @Nullable ResolvedDependency findDependencyWithOmit(String groupId, String artifactId) {
return findDependencyWithOmit0(groupId, artifactId, Collections.newSetFromMap(new IdentityHashMap<>()));
}

private @Nullable ResolvedDependency findDependencyWithOmit0(String groupId, String artifactId, Set<ResolvedDependency> visited) {
if (matchesGlob(getGroupId(), groupId) && matchesGlob(getArtifactId(), artifactId)) {
return this;
} else if (!visited.add(this)) {
return null;
}
outer:
for (ResolvedDependency dependency : dependencies) {
ResolvedDependency found = dependency.findDependencyWithOmit0(groupId, artifactId, visited);
if (found != null) {
if (getRequested().getExclusions() != null) {
for (GroupArtifact exclusion : getRequested().getExclusions()) {
if (matchesGlob(found.getGroupId(), exclusion.getGroupId()) &&
matchesGlob(found.getArtifactId(), exclusion.getArtifactId())) {
continue outer;
}
}
}
return found;
}
}

for (GroupArtifactVersion omit : omitDependencies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where we'd have to handle a possible null omitDependencies field.

if (matchesGlob(omit.getGroupId(), groupId) && matchesGlob(omit.getArtifactId(), artifactId)) {
return this;
}
}
return null;
}

@Override
public String toString() {
return (repository == null ? "" : repository.getUri() + "/") +
Expand Down
9 changes: 8 additions & 1 deletion 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 @@ -916,6 +916,13 @@ public List<ResolvedDependency> resolveDependencies(Scope scope, Map<GroupArtifa
} else if (contains(dependencies, ga, d.getClassifier())) {
// we've already resolved this previously and the requirement didn't change,
// so just skip and continue on
ResolvedDependency includedBy = dd.getDependent();
if (includedBy != null) {
if (includedBy.getOmitDependencies().isEmpty()) {
includedBy.unsafeSetOmitDependencies(new ArrayList<>());
}
includedBy.getOmitDependencies().add(new GroupArtifactVersion(d.getGroupId(), d.getArtifactId(), d.getVersion()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here the generated getter might return null; as an alternative we could create an explicit getter that always returns a collection.

}
continue;
}
}
Expand All @@ -938,7 +945,7 @@ public List<ResolvedDependency> resolveDependencies(Scope scope, Map<GroupArtifa
}

ResolvedDependency resolved = new ResolvedDependency(dPom.getRepository(),
resolvedPom.getGav(), dd.getDependency(), emptyList(),
resolvedPom.getGav(), dd.getDependency(), emptyList(), emptyList(),
resolvedPom.getRequested().getLicenses(),
resolvedPom.getValue(dd.getDependency().getType()),
resolvedPom.getValue(dd.getDependency().getClassifier()),
Expand Down
65 changes: 65 additions & 0 deletions rewrite-maven/src/test/java/org/openrewrite/maven/ExcludeDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.openrewrite.Issue;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.TypeValidation;

import static org.openrewrite.maven.Assertions.pomXml;

Expand Down Expand Up @@ -234,4 +235,68 @@ void respectTransitiveDependencyOriginalScopeWhenDeterminingIfExclusionIsNecessa
""")
);
}

@Test
void excludeAlsoWhereConflictOmitted() {
rewriteRun(
spec -> spec.typeValidationOptions(TypeValidation.none()).afterTypeValidationOptions(TypeValidation.none())
.recipe(new ExcludeDependency("org.apache.logging.log4j", "log4j-api", null)),
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<version>0.0.1-SNAPSHOT</version>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
<version>2.7.18</version>
</dependency>
<dependency>
<!-- (org.apache.logging.log4j:log4j-api:jar:2.20.0:compile - omitted for conflict with 2.17.2) -->
<groupId>org.opensearch.client</groupId>
<artifactId>spring-data-opensearch-starter</artifactId>
<version>1.3.0</version>
</dependency>
</dependencies>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<version>0.0.1-SNAPSHOT</version>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
<version>2.7.18</version>
<exclusions>
<exclusion>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<!-- (org.apache.logging.log4j:log4j-api:jar:2.20.0:compile - omitted for conflict with 2.17.2) -->
<groupId>org.opensearch.client</groupId>
<artifactId>spring-data-opensearch-starter</artifactId>
<version>1.3.0</version>
<exclusions>
<exclusion>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</exclusion>
</exclusions>
</dependency>
</dependencies>
</project>
"""
)
);
}
}