GH-597 - Deduplicate violations with the same message.

Moving away from exceptions as carriers for violations.

Author: odrotbohm

spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModule.java

@@ -37,6 +37,7 @@
 import org.springframework.lang.Nullable;
 import org.springframework.modulith.core.Types.JMoleculesTypes;
 import org.springframework.modulith.core.Types.SpringTypes;
+import org.springframework.modulith.core.Violations.Violation;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 import org.springframework.util.function.SingletonSupplier;
@@ -952,7 +953,7 @@ Violations isValidDependencyWithin(ApplicationModules modules) {
 						.formatted(originModule.getName(), targetModule.getName(), source.getName(), target.getName(),
 								declaredDependencies.toString());
 
-				return violations.and(new IllegalStateException(message));
+				return violations.and(new Violation(message));
 			}
 
 			// No explicitly allowed dependencies - check for general access
@@ -962,7 +963,7 @@ Violations isValidDependencyWithin(ApplicationModules modules) {
 				var violationText = "Module '%s' depends on non-exposed type %s within module '%s'!"
 						.formatted(originModule.getName(), target.getName(), targetModule.getName());
 
-				return violations.and(new IllegalStateException(violationText + lineSeparator() + description));
+				return violations.and(new Violation(violationText + lineSeparator() + description));
 			}
 
 			return violations;
@@ -1138,7 +1139,7 @@ Violations isValidDependencyWithin(ApplicationModules modules) {
 
 				ApplicationModule module = getExistingModuleOf(source.getOwner(), modules);
 
-				violations = violations.and(new IllegalStateException(
+				violations = violations.and(new Violation(
 						String.format("Module %s uses field injection in %s. Prefer constructor injection instead!",
 								module.getDisplayName(), source.getFullName())));
 			}

spring-modulith-core/src/main/java/org/springframework/modulith/core/ApplicationModules.java

@@ -40,6 +40,7 @@
 import org.springframework.modulith.Modulith;
 import org.springframework.modulith.Modulithic;
 import org.springframework.modulith.core.Types.JMoleculesTypes;
+import org.springframework.modulith.core.Violations.Violation;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 
@@ -375,7 +376,7 @@ public Violations detectViolations() {
 		Violations violations = rootPackages.stream() //
 				.map(this::assertNoCyclesFor) //
 				.flatMap(it -> it.getDetails().stream()) //
-				.map(IllegalStateException::new) //
+				.map(Violation::new) //
 				.collect(Violations.toViolations());
 
 		if (JMoleculesTypes.areRulesPresent()) {

spring-modulith-core/src/main/java/org/springframework/modulith/core/Violations.java

@@ -20,7 +20,6 @@
 import java.util.List;
 import java.util.stream.Collector;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import org.springframework.util.Assert;
 
@@ -35,26 +34,26 @@ public class Violations extends RuntimeException {
 
 	public static Violations NONE = new Violations(Collections.emptyList());
 
-	private final List<RuntimeException> exceptions;
+	private final List<Violation> violations;
 
 	/**
-	 * Creates a new {@link Violations} from the given {@link RuntimeException}s.
+	 * Creates a new {@link Violations} from the given {@link ArchitecturalViolation}s.
 	 *
-	 * @param exceptions must not be {@literal null}.
+	 * @param violations must not be {@literal null}.
 	 */
-	private Violations(List<RuntimeException> exceptions) {
+	private Violations(List<Violation> violations) {
 
-		Assert.notNull(exceptions, "Exceptions must not be null!");
+		Assert.notNull(violations, "Exceptions must not be null!");
 
-		this.exceptions = exceptions;
+		this.violations = violations;
 	}
 
 	/**
 	 * A {@link Collector} to turn a {@link Stream} of {@link RuntimeException}s into a {@link Violations} instance.
 	 *
 	 * @return will never be {@literal null}.
 	 */
-	static Collector<RuntimeException, ?, Violations> toViolations() {
+	static Collector<Violation, ?, Violations> toViolations() {
 		return Collectors.collectingAndThen(Collectors.toList(), Violations::new);
 	}
 
@@ -65,8 +64,8 @@ private Violations(List<RuntimeException> exceptions) {
 	@Override
 	public String getMessage() {
 
-		return exceptions.stream() //
-				.map(RuntimeException::getMessage) //
+		return violations.stream() //
+				.map(Violation::message) //
 				.collect(Collectors.joining("\n- ", "- ", ""));
 	}
 
@@ -76,7 +75,7 @@ public String getMessage() {
 	 * @return
 	 */
 	public boolean hasViolations() {
-		return !exceptions.isEmpty();
+		return !violations.isEmpty();
 	}
 
 	/**
@@ -95,36 +94,52 @@ public void throwIfPresent() {
 	 * @param exception must not be {@literal null}.
 	 * @return
 	 */
-	Violations and(RuntimeException exception) {
+	Violations and(Violation violation) {
 
-		Assert.notNull(exception, "Exception must not be null!");
+		Assert.notNull(violation, "Exception must not be null!");
 
-		List<RuntimeException> newExceptions = new ArrayList<>(exceptions.size() + 1);
-		newExceptions.addAll(exceptions);
-		newExceptions.add(exception);
+		if (violations.isEmpty()) {
+			return new Violations(List.of(violation));
+		}
+
+		if (violations.contains(violation)) {
+			return this;
+		}
+
+		List<Violation> newExceptions = new ArrayList<>(violations.size() + 1);
+		newExceptions.addAll(violations);
+		newExceptions.add(violation);
 
 		return new Violations(newExceptions);
 	}
 
 	Violations and(Violations other) {
 
-		List<RuntimeException> newExceptions = new ArrayList<>(exceptions.size() + other.exceptions.size());
-		newExceptions.addAll(exceptions);
-		newExceptions.addAll(other.exceptions);
+		if (violations.isEmpty()) {
+			return new Violations(other.violations);
+		}
+
+		var newExceptions = new ArrayList<>(violations);
+
+		for (Violation candidate : other.violations) {
+			if (!violations.contains(candidate)) {
+				newExceptions.add(candidate);
+			}
+		}
 
 		return new Violations(newExceptions);
 	}
 
 	Violations and(String violation) {
-		return and(new ArchitecturalViolation(violation));
+		return and(new Violation(violation));
 	}
 
-	private static class ArchitecturalViolation extends RuntimeException {
-
-		private static final long serialVersionUID = 3587887036508024142L;
+	List<String> getMessages() {
 
-		public ArchitecturalViolation(String message) {
-			super(message);
-		}
+		return violations.stream()
+				.map(Violation::message)
+				.toList();
 	}
+
+	static record Violation(String message) {}
 }

spring-modulith-core/src/test/java/org/springframework/modulith/core/ViolationsUnitTests.java

@@ -30,10 +30,25 @@ class ViolationsUnitTests {
 	void combinesExceptionMessages() {
 
 		Violations violations = Violations.NONE //
-				.and(new IllegalArgumentException("First")) //
-				.and(new IllegalArgumentException("Second"));
+				.and("First") //
+				.and("Second");
 
 		assertThat(violations.getMessage()) //
 				.isEqualTo("- First\n- Second");
 	}
+
+	@Test
+	void deduplicatesViolationsWithTheSameMessage() {
+
+		var violations = Violations.NONE
+				.and("First")
+				.and("First")
+				.and("Second")
+				.and(Violations.NONE.and("First")
+						.and("Second")
+						.and("Third"));
+
+		assertThat(violations.getMessages())
+				.containsExactlyInAnyOrder("First", "Second", "Third");
+	}
 }