From 5c12b1d7005d5e003cddfefeb5ebc9aa19509104 Mon Sep 17 00:00:00 2001 From: Ravi Gupta Date: Wed, 26 Oct 2022 20:57:00 +0530 Subject: [PATCH 1/4] Initial changes for dependency merge changes --- .../build/api/DependencyChangeResolver.java | 93 +++++++++++++++++++ .../sbm/build/api/DependencyChangeSpec.java | 25 +++++ 2 files changed, 118 insertions(+) create mode 100644 components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java create mode 100644 components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java new file mode 100644 index 000000000..1efafe39a --- /dev/null +++ b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java @@ -0,0 +1,93 @@ +package org.springframework.sbm.build.api; + +import lombok.NonNull; +import org.openrewrite.maven.tree.Scope; + +import java.util.*; +import java.util.stream.Collectors; +import static org.openrewrite.maven.tree.Scope.*; + +public class DependencyChangeResolver { + + public static final EnumSet ALL_SCOPES = EnumSet.range(None, System); + @NonNull + private BuildFile buildFile; + @NonNull + private DependencyChangeSpec proposedChangeSpec; + private List potentialMatches; + + private static Map> dependencyGraph = new HashMap<>(); + static { + dependencyGraph.put(Provided, List.of(Provided)); + dependencyGraph.put(Compile, List.of(Provided,Compile)); + dependencyGraph.put(System, List.of(Provided,Compile,System)); + dependencyGraph.put(Test, List.of(Provided,Compile,System,Test)); + } + + public DependencyChangeResolver(BuildFile buildFile, DependencyChangeSpec proposedChangeSpec){ + this.buildFile = buildFile; + this.proposedChangeSpec = proposedChangeSpec; + this.potentialMatches = buildFile.getEffectiveDependencies() + .stream() + .filter(d -> d.equals(this.proposedChangeSpec.getDependency())) + .collect(Collectors.toList()); + } + + public void apply(){ + this.upsertDependencies(upgradeDependencySpec().orElse(proposedChangeSpec)); + } + + private Optional upgradeDependencySpec() { + if(potentialMatches.isEmpty()) + return Optional.empty(); + + Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getDependency().getScope()); + if(proposedDependencyScope == None) + proposedDependencyScope = Compile; + + //What if the transitive declares the scope as test but the project dont have direct dependency. + // Do we really need to care about the transitives? + Optional upgradedDependencySpec + = potentialMatches + .stream() + .map(Dependency::getScope) + .map(Scope::fromName) + .filter(dependencyGraph.get(proposedDependencyScope)::contains) + .map(proposedChangeSpec::changeScope) + .findFirst(); + + upgradedDependencySpec + .orElseThrow(() -> new IllegalArgumentException(getInvalidScopeMessage())); + + return upgradedDependencySpec; + } + + private String getInvalidScopeMessage() { + Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getDependency().getScope()); + List allowedScopes = dependencyGraph.get(proposedDependencyScope); + List disallowedScopes + = ALL_SCOPES + .stream() + .filter(s -> !allowedScopes.contains(s)) + .map(Scope::name) + .collect(Collectors.toList()); + + return "Dependency " + + this.proposedChangeSpec.getDependency().getCoordinates() + + " already present in scope " + + disallowedScopes; + } + + private void upsertDependencies(DependencyChangeSpec effectiveSpec){ + List declaredDependencies = buildFile.getDeclaredDependencies(ALL_SCOPES.toArray(new Scope[0])); + + List existingDependenciesList = declaredDependencies + .stream() + .filter(proposedChangeSpec::equals) + .collect(Collectors.toList()); + + buildFile.removeDependencies(existingDependenciesList); + buildFile.addDependency(effectiveSpec.getDependency()); + } + +} diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java new file mode 100644 index 000000000..31636d404 --- /dev/null +++ b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java @@ -0,0 +1,25 @@ +package org.springframework.sbm.build.api; + +import lombok.AllArgsConstructor; +import lombok.Getter; +import org.openrewrite.maven.tree.Scope; + +@Getter +@AllArgsConstructor +public class DependencyChangeSpec { + + public enum Operation{ + ADD, + REMOVE + } + + @Getter + private Dependency dependency; + private Operation operation; + + public DependencyChangeSpec changeScope(Scope scope){ + DependencyChangeSpec newSpec = new DependencyChangeSpec(this.dependency, this.operation); + newSpec.dependency.setScope(scope.name()); + return newSpec; + } +} From 188801cc83a1e683d1ac6d3670c632cbad389c96 Mon Sep 17 00:00:00 2001 From: Ravi Gupta Date: Fri, 28 Oct 2022 09:14:26 +0530 Subject: [PATCH 2/4] Initial proposal for Dependency resolution based on scope --- .../build/api/DependencyChangeResolver.java | 92 +++++++++---------- 1 file changed, 45 insertions(+), 47 deletions(-) diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java index 1efafe39a..9b5bd7afb 100644 --- a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java +++ b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java @@ -7,78 +7,74 @@ import java.util.stream.Collectors; import static org.openrewrite.maven.tree.Scope.*; +/** + * Resolve the dependency change spec. Their is a ascending + * order of the scopes where a higher order covers its predecessor + * and more. Below is the ascending order of the scopes. + * + * Test (lowest), Runtime, Provided, Compile (highest) + * + * Based on the above scope, the following rule decides the fate + * of a proposed dependency change spec. + * + * Rule 1 :- If the proposed dependency already exists + * transitively but its scope is lesser than the proposed + * scope, the proposed dependency will be added to the + * build file. + * + * Rule 2 :- If the proposed dependency already declared + * directly but its scope is lesser than the the proposed + * scope, the existing dependency will be replaced. + * + * Rule 3 :- If there is no matching dependency already exists + * the proposed dependency will beadded. + */ public class DependencyChangeResolver { public static final EnumSet ALL_SCOPES = EnumSet.range(None, System); @NonNull private BuildFile buildFile; - @NonNull - private DependencyChangeSpec proposedChangeSpec; + private @NonNull Dependency proposedChangeSpec; private List potentialMatches; - private static Map> dependencyGraph = new HashMap<>(); + private static Map> UPGRADE_GRAPH = new HashMap<>(); static { - dependencyGraph.put(Provided, List.of(Provided)); - dependencyGraph.put(Compile, List.of(Provided,Compile)); - dependencyGraph.put(System, List.of(Provided,Compile,System)); - dependencyGraph.put(Test, List.of(Provided,Compile,System,Test)); + // For a given scope (key), SBM will upgrade ( upsert) if any of the listed scope + // exists in the directly included dependencies + UPGRADE_GRAPH.put(Compile, List.of(Test,Provided,Runtime)); + UPGRADE_GRAPH.put(Provided,List.of(Test,Runtime)); + UPGRADE_GRAPH.put(Runtime,List.of(Test)); + UPGRADE_GRAPH.put(Test,Collections.emptyList()); } - public DependencyChangeResolver(BuildFile buildFile, DependencyChangeSpec proposedChangeSpec){ + public DependencyChangeResolver(BuildFile buildFile, @NonNull Dependency proposedChangeSpec){ this.buildFile = buildFile; this.proposedChangeSpec = proposedChangeSpec; this.potentialMatches = buildFile.getEffectiveDependencies() .stream() - .filter(d -> d.equals(this.proposedChangeSpec.getDependency())) + .filter(d -> d.equals(this.proposedChangeSpec)) .collect(Collectors.toList()); } public void apply(){ - this.upsertDependencies(upgradeDependencySpec().orElse(proposedChangeSpec)); + if(isUpsertRequired()) + upsertDependencies(proposedChangeSpec); } - private Optional upgradeDependencySpec() { - if(potentialMatches.isEmpty()) - return Optional.empty(); + private boolean isUpsertRequired() { + if(potentialMatches.isEmpty()) // Rule 3 + return true; - Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getDependency().getScope()); - if(proposedDependencyScope == None) - proposedDependencyScope = Compile; + Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getScope()); - //What if the transitive declares the scope as test but the project dont have direct dependency. - // Do we really need to care about the transitives? - Optional upgradedDependencySpec - = potentialMatches + return potentialMatches .stream() .map(Dependency::getScope) .map(Scope::fromName) - .filter(dependencyGraph.get(proposedDependencyScope)::contains) - .map(proposedChangeSpec::changeScope) - .findFirst(); - - upgradedDependencySpec - .orElseThrow(() -> new IllegalArgumentException(getInvalidScopeMessage())); - - return upgradedDependencySpec; + .anyMatch(UPGRADE_GRAPH.get(proposedDependencyScope)::contains); // Rule 1 } - private String getInvalidScopeMessage() { - Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getDependency().getScope()); - List allowedScopes = dependencyGraph.get(proposedDependencyScope); - List disallowedScopes - = ALL_SCOPES - .stream() - .filter(s -> !allowedScopes.contains(s)) - .map(Scope::name) - .collect(Collectors.toList()); - - return "Dependency " - + this.proposedChangeSpec.getDependency().getCoordinates() - + " already present in scope " - + disallowedScopes; - } - - private void upsertDependencies(DependencyChangeSpec effectiveSpec){ + private void upsertDependencies(@NonNull Dependency effectiveSpec){ List declaredDependencies = buildFile.getDeclaredDependencies(ALL_SCOPES.toArray(new Scope[0])); List existingDependenciesList = declaredDependencies @@ -86,8 +82,10 @@ private void upsertDependencies(DependencyChangeSpec effectiveSpec){ .filter(proposedChangeSpec::equals) .collect(Collectors.toList()); - buildFile.removeDependencies(existingDependenciesList); - buildFile.addDependency(effectiveSpec.getDependency()); + if(!existingDependenciesList.isEmpty()) // Rule 2 + buildFile.removeDependencies(existingDependenciesList); + + buildFile.addDependency(proposedChangeSpec); } } From 850a4270a1e82864f0d7e8e5a3b7f66283841f0f Mon Sep 17 00:00:00 2001 From: Ravi Gupta Date: Fri, 28 Oct 2022 09:14:50 +0530 Subject: [PATCH 3/4] Initial proposal for Dependency resolution based on scope --- .../sbm/build/api/DependencyChangeSpec.java | 25 ------------------- 1 file changed, 25 deletions(-) delete mode 100644 components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java deleted file mode 100644 index 31636d404..000000000 --- a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeSpec.java +++ /dev/null @@ -1,25 +0,0 @@ -package org.springframework.sbm.build.api; - -import lombok.AllArgsConstructor; -import lombok.Getter; -import org.openrewrite.maven.tree.Scope; - -@Getter -@AllArgsConstructor -public class DependencyChangeSpec { - - public enum Operation{ - ADD, - REMOVE - } - - @Getter - private Dependency dependency; - private Operation operation; - - public DependencyChangeSpec changeScope(Scope scope){ - DependencyChangeSpec newSpec = new DependencyChangeSpec(this.dependency, this.operation); - newSpec.dependency.setScope(scope.name()); - return newSpec; - } -} From 20aaa4b2d86fdf65ba1f3e6067b77d51a6b03b8c Mon Sep 17 00:00:00 2001 From: Ravi Gupta Date: Tue, 8 Nov 2022 20:44:31 +0530 Subject: [PATCH 4/4] Initial proposal for Dependency resolution based on scope --- .../build/api/DependencyChangeResolver.java | 58 ++++--- .../migration/actions/AddDependencies.java | 24 ++- .../api/DependencyChangeResolverTest.java | 142 ++++++++++++++++++ 3 files changed, 193 insertions(+), 31 deletions(-) create mode 100644 components/sbm-core/src/test/java/org/springframework/sbm/build/api/DependencyChangeResolverTest.java diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java index 9b5bd7afb..510d6e242 100644 --- a/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java +++ b/components/sbm-core/src/main/java/org/springframework/sbm/build/api/DependencyChangeResolver.java @@ -1,31 +1,33 @@ package org.springframework.sbm.build.api; import lombok.NonNull; +import org.apache.commons.lang3.tuple.Pair; import org.openrewrite.maven.tree.Scope; import java.util.*; import java.util.stream.Collectors; + import static org.openrewrite.maven.tree.Scope.*; /** * Resolve the dependency change spec. Their is a ascending * order of the scopes where a higher order covers its predecessor * and more. Below is the ascending order of the scopes. - * + *

* Test (lowest), Runtime, Provided, Compile (highest) - * + *

* Based on the above scope, the following rule decides the fate * of a proposed dependency change spec. - * + *

* Rule 1 :- If the proposed dependency already exists * transitively but its scope is lesser than the proposed * scope, the proposed dependency will be added to the * build file. - * + *

* Rule 2 :- If the proposed dependency already declared * directly but its scope is lesser than the the proposed * scope, the existing dependency will be replaced. - * + *

* Rule 3 :- If there is no matching dependency already exists * the proposed dependency will beadded. */ @@ -38,16 +40,17 @@ public class DependencyChangeResolver { private List potentialMatches; private static Map> UPGRADE_GRAPH = new HashMap<>(); + static { // For a given scope (key), SBM will upgrade ( upsert) if any of the listed scope // exists in the directly included dependencies - UPGRADE_GRAPH.put(Compile, List.of(Test,Provided,Runtime)); - UPGRADE_GRAPH.put(Provided,List.of(Test,Runtime)); - UPGRADE_GRAPH.put(Runtime,List.of(Test)); - UPGRADE_GRAPH.put(Test,Collections.emptyList()); + UPGRADE_GRAPH.put(Compile, List.of(Test, Provided, Runtime)); + UPGRADE_GRAPH.put(Provided, List.of(Test, Runtime)); + UPGRADE_GRAPH.put(Runtime, List.of(Test)); + UPGRADE_GRAPH.put(Test, Collections.emptyList()); } - public DependencyChangeResolver(BuildFile buildFile, @NonNull Dependency proposedChangeSpec){ + public DependencyChangeResolver(BuildFile buildFile, @NonNull Dependency proposedChangeSpec) { this.buildFile = buildFile; this.proposedChangeSpec = proposedChangeSpec; this.potentialMatches = buildFile.getEffectiveDependencies() @@ -56,36 +59,31 @@ public DependencyChangeResolver(BuildFile buildFile, @NonNull Dependency propose .collect(Collectors.toList()); } - public void apply(){ - if(isUpsertRequired()) - upsertDependencies(proposedChangeSpec); - } - - private boolean isUpsertRequired() { - if(potentialMatches.isEmpty()) // Rule 3 - return true; + /** + * Return a pair of dependencies to be removed ( left) and added ( right) + * @return + */ + public Pair, Optional> apply() { + if (potentialMatches.isEmpty()) + return Pair.of(Collections.emptyList(), Optional.of(proposedChangeSpec)); Scope proposedDependencyScope = Scope.fromName(proposedChangeSpec.getScope()); + List supersededScopes = UPGRADE_GRAPH.get(proposedDependencyScope); - return potentialMatches + Optional right = potentialMatches .stream() - .map(Dependency::getScope) - .map(Scope::fromName) - .anyMatch(UPGRADE_GRAPH.get(proposedDependencyScope)::contains); // Rule 1 - } - - private void upsertDependencies(@NonNull Dependency effectiveSpec){ - List declaredDependencies = buildFile.getDeclaredDependencies(ALL_SCOPES.toArray(new Scope[0])); + .filter(d -> supersededScopes.contains(fromName(d.getScope()))) + .findAny() + .map(any -> proposedChangeSpec); - List existingDependenciesList = declaredDependencies + List left = buildFile + .getDeclaredDependencies(ALL_SCOPES.toArray(new Scope[0])) .stream() .filter(proposedChangeSpec::equals) .collect(Collectors.toList()); - if(!existingDependenciesList.isEmpty()) // Rule 2 - buildFile.removeDependencies(existingDependenciesList); + return Pair.of(left, right); - buildFile.addDependency(proposedChangeSpec); } } diff --git a/components/sbm-core/src/main/java/org/springframework/sbm/build/migration/actions/AddDependencies.java b/components/sbm-core/src/main/java/org/springframework/sbm/build/migration/actions/AddDependencies.java index e2d0386f6..af586008b 100644 --- a/components/sbm-core/src/main/java/org/springframework/sbm/build/migration/actions/AddDependencies.java +++ b/components/sbm-core/src/main/java/org/springframework/sbm/build/migration/actions/AddDependencies.java @@ -16,8 +16,10 @@ package org.springframework.sbm.build.migration.actions; import lombok.AllArgsConstructor; +import org.apache.commons.lang3.tuple.Pair; import org.springframework.sbm.build.api.BuildFile; import org.springframework.sbm.build.api.Dependency; +import org.springframework.sbm.build.api.DependencyChangeResolver; import org.springframework.sbm.engine.recipe.AbstractAction; import org.springframework.sbm.engine.context.ProjectContext; import lombok.Getter; @@ -29,6 +31,9 @@ import javax.validation.Valid; import java.util.ArrayList; import java.util.List; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Collectors; @Setter @Getter @@ -51,6 +56,23 @@ public AddDependencies(List dependencies) { @Override public void apply(ProjectContext context) { BuildFile buildFile = context.getBuildFile(); - buildFile.addDependencies(dependencies); + List, Optional>> pairs = dependencies.stream() + .map(d -> new DependencyChangeResolver(buildFile, d)) + .map(DependencyChangeResolver::apply) + .collect(Collectors.toList()); + + List removeList = pairs.stream() + .map(Pair::getLeft) + .flatMap(List::stream) + .collect(Collectors.toList()); + + List addList = pairs.stream() + .map(Pair::getRight) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(Collectors.toList()); + + buildFile.removeDependencies(removeList); + buildFile.addDependencies(addList); } } diff --git a/components/sbm-core/src/test/java/org/springframework/sbm/build/api/DependencyChangeResolverTest.java b/components/sbm-core/src/test/java/org/springframework/sbm/build/api/DependencyChangeResolverTest.java new file mode 100644 index 000000000..9b442ea2f --- /dev/null +++ b/components/sbm-core/src/test/java/org/springframework/sbm/build/api/DependencyChangeResolverTest.java @@ -0,0 +1,142 @@ +package org.springframework.sbm.build.api; + +import org.apache.commons.lang3.tuple.Pair; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.sbm.build.impl.OpenRewriteMavenBuildFile; + +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +public class DependencyChangeResolverTest { + + @Mock + OpenRewriteMavenBuildFile buildFile; + + @Test + public void givenBuildFile_addNewDependency_withScopeCompile_expectNewDependencyAdded(){ + Dependency proposedDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("compile") + .build(); + + when(buildFile.getEffectiveDependencies()) + .thenReturn(Collections.emptySet()); + + Pair, Optional> pair = + new DependencyChangeResolver(buildFile, proposedDependency).apply(); + + assertThat(pair.getLeft().isEmpty()); + assertThat(pair.getRight().isPresent()); + assertThat(pair.getRight().get().equals(proposedDependency)); + } + + @Test + public void givenBuildFile_addExistingTransitiveDependency_withLowerScope_expectNoOp(){ + Dependency proposedDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("test") + .build(); + + Dependency existingDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("compile") + .build(); + + when(buildFile.getEffectiveDependencies()) + .thenReturn(Set.of(existingDependency)); + + when(buildFile.getDeclaredDependencies(any())) + .thenReturn(Collections.emptyList()); + + Pair, Optional> pair = + new DependencyChangeResolver(buildFile, proposedDependency).apply(); + + assertThat(pair.getLeft().isEmpty()); + assertThat(pair.getRight().isEmpty()); + } + + @Test + public void givenBuildFile_addExistingTransitiveDependency_withHigherScope_expectNoOp(){ + Dependency proposedDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("compile") + .build(); + + Dependency existingDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("test") + .build(); + + when(buildFile.getEffectiveDependencies()) + .thenReturn(Set.of(existingDependency)); + + when(buildFile.getDeclaredDependencies(any())) + .thenReturn(Collections.emptyList()); + + Pair, Optional> pair = + new DependencyChangeResolver(buildFile, proposedDependency).apply(); + + assertThat(pair.getLeft().isEmpty()); + assertThat(pair.getRight().isPresent()); + assertThat(pair.getRight().get().equals(proposedDependency)); + } + + @Test + public void givenBuildFile_addExistingDirectDependency_withHigherScope_expectNoOp(){ + Dependency proposedDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("compile") + .build(); + + Dependency existingDependency = + new Dependency.DependencyBuilder() + .groupId("org.springframework.sbm") + .artifactId("directDependency") + .version("1.0") + .scope("test") + .build(); + + when(buildFile.getEffectiveDependencies()) + .thenReturn(Set.of(existingDependency)); + + when(buildFile.getDeclaredDependencies(any())) + .thenReturn(List.of(existingDependency)); + + Pair, Optional> pair = + new DependencyChangeResolver(buildFile, proposedDependency).apply(); + + assertThat(!pair.getLeft().isEmpty()); + assertThat(pair.getLeft().get(0).getScope().equals("test")); + assertThat(pair.getRight().isPresent()); + assertThat(pair.getRight().get().getScope().equals("compile")); + } + +}