From 8fb7f07b4a86cb7f0560d892b947444caab51468 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Fri, 21 Feb 2025 14:34:05 +0100 Subject: [PATCH] GH-1060 - Remove optional dependency on JGraphT. We now implement the topological sorting of application modules ourselves. Properly recalculate module order after ApplicationModules recreation with shared modules. Make iterator resilient against premature invocation by falling back to the unordered modules. Cleanup of generated type ignorance. --- .../modulith/core/ApplicationModules.java | 143 +++++++++++------- 1 file changed, 87 insertions(+), 56 deletions(-) diff --git a/spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModules.java b/spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModules.java index dab0fc40..ecfbe96e 100644 --- a/spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModules.java +++ b/spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModules.java @@ -23,6 +23,7 @@ import static java.util.stream.Collectors.*; import static org.springframework.modulith.core.Violations.*; import java.util.*; +import java.util.Map.Entry; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; import java.util.function.Predicate; @@ -31,10 +32,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; -import org.jgrapht.Graph; -import org.jgrapht.graph.DefaultDirectedGraph; -import org.jgrapht.graph.DefaultEdge; -import org.jgrapht.traverse.TopologicalOrderIterator; import org.springframework.aot.generate.Generated; import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.lang.Nullable; @@ -44,10 +41,8 @@ import org.springframework.util.ClassUtils; import org.springframework.util.function.SingletonSupplier; import com.tngtech.archunit.base.DescribedPredicate; -import com.tngtech.archunit.core.domain.JavaAnnotation; import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaClasses; -import com.tngtech.archunit.core.domain.JavaType; import com.tngtech.archunit.core.domain.properties.CanBeAnnotated; import com.tngtech.archunit.core.domain.properties.HasName; import com.tngtech.archunit.core.importer.ClassFileImporter; @@ -67,19 +62,12 @@ public class ApplicationModules implements Iterable { private static final Map CACHE = new ConcurrentHashMap<>(); private static final ImportOption IMPORT_OPTION = new ImportOption.DoNotIncludeTests(); - private static final boolean JGRAPHT_PRESENT = ClassUtils.isPresent("org.jgrapht.Graph", - ApplicationModules.class.getClassLoader()); - private static final DescribedPredicate IS_AOT_TYPE, IS_GENERATED; + private static final DescribedPredicate IS_GENERATED; private static final DescribedPredicate IS_SPRING_CGLIB_PROXY = nameContaining("$$SpringCGLIB$$"); static { - - IS_AOT_TYPE = ClassUtils.isPresent("org.springframework.aot.generate.Generated", + IS_GENERATED = ClassUtils.isPresent("org.springframework.aot.generate.Generated", ApplicationModules.class.getClassLoader()) ? getAtGenerated() : DescribedPredicate.alwaysFalse(); - - IS_GENERATED = annotatedWith(JavaClass.Predicates.simpleName("Generated") - .onResultOf(JavaType::toErasure) - .onResultOf(JavaAnnotation::getType)); } @Nullable @@ -129,7 +117,8 @@ public class ApplicationModules implements Iterable { Assert.notNull(ignored, "Ignores must not be null!"); Assert.notNull(option, "ImportOptions must not be null!"); - DescribedPredicate excluded = DescribedPredicate.or(ignored, IS_AOT_TYPE, IS_SPRING_CGLIB_PROXY); + DescribedPredicate excluded = DescribedPredicate.or(ignored, IS_GENERATED, + IS_SPRING_CGLIB_PROXY); this.metadata = metadata; var importer = new ClassFileImporter() // @@ -174,14 +163,7 @@ public class ApplicationModules implements Iterable { .toList()); this.sharedModules = Collections.emptySet(); - - Supplier> fallback = () -> modules.values().stream() - .map(ApplicationModule::getIdentifier) - .sorted() - .toList(); - - this.orderedNames = Optional.ofNullable(JGRAPHT_PRESENT ? TopologicalSorter.topologicallySortModules(this) : null) - .orElseGet(fallback); + this.orderedNames = topologicallyOrderIdentifiers(this); } /** @@ -564,7 +546,10 @@ public class ApplicationModules implements Iterable { */ @Override public Iterator iterator() { - return orderedNames.stream().map(this::getRequiredModule).iterator(); + + return orderedNames != null + ? orderedNames.stream().map(this::getRequiredModule).iterator() + : modules.values().iterator(); } /* @@ -683,6 +668,22 @@ public class ApplicationModules implements Iterable { }; } + /** + * Returns all {@link ApplicationModuleIdentifier} topologically sorted. + * + * @param modules must not be {@literal null}. + * @return will never be {@literal null}. + */ + private static final List topologicallyOrderIdentifiers(ApplicationModules modules) { + + Supplier> fallback = () -> modules.stream() + .map(ApplicationModule::getIdentifier) + .sorted() + .toList(); + + return Optional.ofNullable(ModuleSorter.sortTopologically(modules)).orElseGet(fallback); + } + public static class Filters { public static DescribedPredicate withoutModules(String... names) { @@ -763,43 +764,73 @@ public class ApplicationModules implements Iterable { } } - /** - * Dedicated class to be able to only optionally depend on the JGraphT library. - * - * @author Oliver Drotbohm - */ - private static class TopologicalSorter { + private static class ModuleSorter { - @Nullable - private static List topologicallySortModules(ApplicationModules modules) { + /** + * Returns a topologically sorted list of {@link ApplicationModuleIdentifier} for the given + * {@link ApplicationModules} or {@literal null} in case we detect a dependency cycle. + * + * @param modules must not be {@literal null}. + * @return can be {@literal null}. + */ + public static @Nullable List sortTopologically(ApplicationModules modules) { - Graph graph = new DefaultDirectedGraph<>(DefaultEdge.class); + var visited = new HashSet(); + var inProgress = new HashSet(); + var levelMap = new TreeMap>(); - modules.modules.values() - .stream() - .sorted() - .forEach(project -> { + boolean cycleDetected = modules.stream() + .filter(module -> !visited.contains(module)) + .anyMatch(module -> visit(module, modules, visited, inProgress, 0, levelMap)); - graph.addVertex(project); - - project.getDirectDependencies(modules).uniqueModules() // - .forEach(dependency -> { - graph.addVertex(dependency); - graph.addEdge(project, dependency); - }); - }); - - var names = new ArrayList(); - var iterator = new TopologicalOrderIterator<>(graph); - - try { - - iterator.forEachRemaining(it -> names.add(0, it.getIdentifier())); - return names; - - } catch (IllegalArgumentException o_O) { + if (cycleDetected) { return null; } + + return levelMap.values().stream().flatMap(Set::stream).toList(); + } + + private static boolean visit(ApplicationModule module, ApplicationModules modules, Set visited, + Set inProgress, int level, Map> levelMap) { + + if (inProgress.contains(module)) { + return true; + } + + if (visited.contains(module)) { + return false; + } + + inProgress.add(module); + + var cycleDetected = module.getDirectDependencies(modules).uniqueModules() + .anyMatch(dependency -> visit(dependency, modules, visited, inProgress, level + 1, levelMap)); + + if (cycleDetected) { + return true; + } + + inProgress.remove(module); + visited.add(module); + + int maxDependencyLevel = module.getDirectDependencies(modules).uniqueModules() + .mapToInt(it -> findLevelForClosestDependency(it, levelMap)) + .max() + .orElse(-1); + + levelMap.computeIfAbsent(maxDependencyLevel + 1, __ -> new TreeSet<>()).add(module.getIdentifier()); + + return false; + } + + private static int findLevelForClosestDependency(ApplicationModule module, + Map> levelMap) { + + return levelMap.entrySet().stream() + .filter(it -> it.getValue().contains(module.getIdentifier())) + .mapToInt(Entry::getKey) + .findFirst() + .orElse(-1); } }