fix line ending problems in FreezingArchRule

This addresses two problems:
1) if the rule description itself had different line separators than it was originally frozen with, `TextBasedViolationStore` would report `contains(rule) == false` and thus the result would be stored as new and the rule checking would succeed
2) if the single lines of the violation descriptions had a different line break than they were originally frozen with, these violations would all be removed as "solved" and the same violations with the different line break would be reported as new.

I think unfortunately the API is a little strange, i.e. that `event.getDescriptionLines()` would return lines that contain a line break (since what is the use to talk about a list of lines if the lines can contain line breaks and are thus multiple lines in themselves). But unfortunately this is not so easy to change, since the counting of violations is based on this at the moment. For example, that one long cycle report `cycle detected ... \n dependencies of ... \n ...` is counted as a single "description line". To change this we would have to adjust this mechanism and then optimally make it illegal to return strings with line breaks from `event.getDescriptionLines()`. But extending `ConditionEvent` and thus implementing this method is public API, so this might be a breaking change again. Altogether I decided to just fix this within `FreezingArchRule` for now and see, if there will be further problems with this in other areas.

To solve 1) I have adjusted `TextBasedViolationStore` to clean up line breaks from property names on save and load. I decided to add it to the file reading direction as well to a) also fix it for stores that have already saved Windows line breaks and b) support manual tinkering adding Windows line breaks in the future. Otherwise just storing them exclusively with `\n` as line separator would have probably been good enough. It would be better to solve 1) in a store independent way, so other store implementations would not have to deal with this problem, but unfortunately I could not find any way to do this, as long as the API is `contains(ArchRule)` and `store(ArchRule, ...)`. We could maybe always have converted these rules via `contains(rule.as(ensureUnixLineBreaks(rule.getDescription())))`, but that also felt a little shady, and might be surprising in a custom store implementation to get `rule.as(..)` instead of the rule directly.

Problem 2) I have addressed within `FreezingArchRule` to make it independent of the concrete store implementation. I decided to put adapters on both "ends" of the rule, i.e. on the `EvaluationResult` producing the violations "lines" on the one side, and the violation store where we read violations from on the other side. That way comparisons should only happen between Unix line separators and violations should always be stored with Unix line breaks within the store.

Signed-off-by: Peter Gafert <[email protected]>

archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java

@@ -19,6 +19,7 @@
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Properties;
 import java.util.Set;
 
 import com.tngtech.archunit.ArchConfiguration;
@@ -27,6 +28,7 @@
 import com.tngtech.archunit.core.domain.JavaClasses;
 import com.tngtech.archunit.lang.ArchRule;
 import com.tngtech.archunit.lang.EvaluationResult;
+import com.tngtech.archunit.lang.Priority;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -73,12 +75,16 @@ public final class FreezingArchRule implements ArchRule {
     private static final Logger log = LoggerFactory.getLogger(FreezingArchRule.class);
 
     private final ArchRule delegate;
-    private final ViolationStore store;
+    private final ViolationStoreLineBreakAdapter store;
     private final ViolationLineMatcher matcher;
 
     private FreezingArchRule(ArchRule delegate, ViolationStore store, ViolationLineMatcher matcher) {
+        this(delegate, new ViolationStoreLineBreakAdapter(store), matcher);
+    }
+
+    private FreezingArchRule(ArchRule delegate, ViolationStoreLineBreakAdapter store, ViolationLineMatcher matcher) {
         this.delegate = checkNotNull(delegate);
-        this.store = checkNotNull(store);
+        this.store = store;
         this.matcher = checkNotNull(matcher);
     }
 
@@ -105,21 +111,21 @@ public FreezingArchRule as(String newDescription) {
     public EvaluationResult evaluate(JavaClasses classes) {
         store.initialize(ArchConfiguration.get().getSubProperties(FREEZE_STORE_PROPERTY_NAME));
 
-        EvaluationResult result = delegate.evaluate(classes);
+        EvaluationResultLineBreakAdapter result = new EvaluationResultLineBreakAdapter(delegate.evaluate(classes));
         if (!store.contains(delegate)) {
             return storeViolationsAndReturnSuccess(result);
         } else {
             return removeObsoleteViolationsFromStoreAndReturnNewViolations(result);
         }
     }
 
-    private EvaluationResult storeViolationsAndReturnSuccess(EvaluationResult result) {
+    private EvaluationResult storeViolationsAndReturnSuccess(EvaluationResultLineBreakAdapter result) {
         log.debug("No results present for rule '{}'. Freezing rule result...", delegate.getDescription());
-        store.save(delegate, result.getFailureReport().getDetails());
+        store.save(delegate, result.getViolations());
         return new EvaluationResult(delegate, result.getPriority());
     }
 
-    private EvaluationResult removeObsoleteViolationsFromStoreAndReturnNewViolations(EvaluationResult result) {
+    private EvaluationResult removeObsoleteViolationsFromStoreAndReturnNewViolations(EvaluationResultLineBreakAdapter result) {
         log.debug("Found frozen result for rule '{}'", delegate.getDescription());
         final List<String> knownViolations = store.getViolations(delegate);
         CategorizedViolations categorizedViolations = new CategorizedViolations(matcher, result, knownViolations);
@@ -135,7 +141,7 @@ private void removeObsoleteViolationsFromStore(CategorizedViolations categorized
         }
     }
 
-    private EvaluationResult filterOutKnownViolations(EvaluationResult result, final Set<String> knownActualViolations) {
+    private EvaluationResult filterOutKnownViolations(EvaluationResultLineBreakAdapter result, final Set<String> knownActualViolations) {
         log.debug("Filtering out known violations: {}", knownActualViolations);
         return result.filterDescriptionsMatching(new Predicate<String>() {
             @Override
@@ -196,14 +202,26 @@ public static FreezingArchRule freeze(ArchRule rule) {
         return new FreezingArchRule(rule, ViolationStoreFactory.create(), ViolationLineMatcherFactory.create());
     }
 
+    static String ensureUnixLineBreaks(String string) {
+        return string.replaceAll("\r\n", "\n");
+    }
+
+    private static List<String> ensureUnixLineBreaks(List<String> strings) {
+        List<String> result = new ArrayList<>();
+        for (String string : strings) {
+            result.add(ensureUnixLineBreaks(string));
+        }
+        return result;
+    }
+
     private static class CategorizedViolations {
         private final Set<String> knownActualViolations = new HashSet<>();
         private final List<String> storedSolvedViolations;
         private final List<String> storedUnsolvedViolations = new ArrayList<>();
 
-        CategorizedViolations(ViolationLineMatcher matcher, EvaluationResult actualResult, List<String> storedViolations) {
+        CategorizedViolations(ViolationLineMatcher matcher, EvaluationResultLineBreakAdapter actualResult, List<String> storedViolations) {
             List<String> storedViolationsLeft = new ArrayList<>(storedViolations);
-            for (String actualViolation : actualResult.getFailureReport().getDetails()) {
+            for (String actualViolation : actualResult.getViolations()) {
                 for (Iterator<String> iterator = storedViolationsLeft.iterator(); iterator.hasNext(); ) {
                     String storedViolation = iterator.next();
                     if (matcher.matches(actualViolation, storedViolation)) {
@@ -230,4 +248,53 @@ List<String> getStoredUnsolvedViolations() {
             return storedUnsolvedViolations;
         }
     }
+
+    private static class ViolationStoreLineBreakAdapter {
+        private final ViolationStore store;
+
+        ViolationStoreLineBreakAdapter(ViolationStore store) {
+            this.store = checkNotNull(store);
+        }
+
+        void initialize(Properties properties) {
+            store.initialize(properties);
+        }
+
+        boolean contains(ArchRule rule) {
+            return store.contains(rule);
+        }
+
+        List<String> getViolations(ArchRule rule) {
+            return ensureUnixLineBreaks(store.getViolations(rule));
+        }
+
+        void save(ArchRule rule, List<String> violations) {
+            store.save(rule, ensureUnixLineBreaks(violations));
+        }
+    }
+
+    private static class EvaluationResultLineBreakAdapter {
+        private final EvaluationResult result;
+
+        private EvaluationResultLineBreakAdapter(EvaluationResult result) {
+            this.result = checkNotNull(result);
+        }
+
+        List<String> getViolations() {
+            return ensureUnixLineBreaks(result.getFailureReport().getDetails());
+        }
+
+        Priority getPriority() {
+            return result.getPriority();
+        }
+
+        EvaluationResult filterDescriptionsMatching(final Predicate<String> predicate) {
+            return result.filterDescriptionsMatching(new Predicate<String>() {
+                @Override
+                public boolean apply(String input) {
+                    return predicate.apply(ensureUnixLineBreaks(input));
+                }
+            });
+        }
+    }
 }

archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java

@@ -39,6 +39,7 @@
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.io.Files.toByteArray;
 import static com.tngtech.archunit.base.ReflectionUtils.newInstanceOf;
+import static com.tngtech.archunit.library.freeze.FreezingArchRule.ensureUnixLineBreaks;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 class ViolationStoreFactory {
@@ -196,10 +197,6 @@ private String readStoreFile(String fileName) {
             }
         }
 
-        private String ensureUnixLineBreaks(String string) {
-            return string.replaceAll("\r\n", "\n");
-        }
-
         private static class FileSyncedProperties {
             private final File propertiesFile;
             private final Properties loadedProperties;
@@ -234,15 +231,15 @@ private Properties loadRulesFrom(File file) {
             }
 
             boolean containsKey(String propertyName) {
-                return loadedProperties.containsKey(propertyName);
+                return loadedProperties.containsKey(ensureUnixLineBreaks(propertyName));
             }
 
             String getProperty(String propertyName) {
-                return loadedProperties.getProperty(propertyName);
+                return loadedProperties.getProperty(ensureUnixLineBreaks(propertyName));
             }
 
             void setProperty(String propertyName, String value) {
-                loadedProperties.setProperty(propertyName, value);
+                loadedProperties.setProperty(ensureUnixLineBreaks(propertyName), ensureUnixLineBreaks(value));
                 syncFileSystem();
             }