From c2e4492e6fab5747f9ab2a4fcc5883ea42c8a3bb Mon Sep 17 00:00:00 2001 From: Peter Gafert Date: Sun, 20 Oct 2019 16:40:05 +0200 Subject: [PATCH] It is now possible to explicitly allow/prevent default store creation and update Signed-off-by: Peter Gafert --- .../library/freeze/FreezingArchRule.java | 8 +- .../freeze/StoreUpdateFailedException.java | 4 + .../library/freeze/ViolationStoreFactory.java | 41 ++++++-- .../library/freeze/FreezingArchRuleTest.java | 95 ++++++++++++++++++- .../TextFileBasedViolationStoreTest.java | 6 +- 5 files changed, 138 insertions(+), 16 deletions(-) diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java index 7a3810495d..1cc58cd060 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/freeze/FreezingArchRule.java @@ -32,7 +32,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.tngtech.archunit.PublicAPI.Usage.ACCESS; -import static com.tngtech.archunit.library.freeze.ViolationStoreFactory.FREEZE_STORE_PROPERTY; +import static com.tngtech.archunit.library.freeze.ViolationStoreFactory.FREEZE_STORE_PROPERTY_NAME; /** * A decorator around an existing {@link ArchRule} that "freezes" the state of all violations on the first call instead of failing the test. @@ -102,7 +102,7 @@ public FreezingArchRule as(String newDescription) { @Override @PublicAPI(usage = ACCESS) public EvaluationResult evaluate(JavaClasses classes) { - store.initialize(ArchConfiguration.get().getSubProperties(FREEZE_STORE_PROPERTY)); + store.initialize(ArchConfiguration.get().getSubProperties(FREEZE_STORE_PROPERTY_NAME)); EvaluationResult result = delegate.evaluate(classes); if (!store.contains(delegate)) { @@ -129,7 +129,9 @@ private EvaluationResult removeObsoleteViolationsFromStoreAndReturnNewViolations private void removeObsoleteViolationsFromStore(CategorizedViolations categorizedViolations) { List solvedViolations = categorizedViolations.getStoredSolvedViolations(); log.debug("Removing {} obsolete violations from store: {}", solvedViolations.size(), solvedViolations); - store.save(delegate, categorizedViolations.getStoredUnsolvedViolations()); + if (!solvedViolations.isEmpty()) { + store.save(delegate, categorizedViolations.getStoredUnsolvedViolations()); + } } private EvaluationResult filterOutKnownViolations(EvaluationResult result, final Set knownActualViolations) { diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreUpdateFailedException.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreUpdateFailedException.java index 286c4e3adc..7a87e45b7a 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreUpdateFailedException.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/freeze/StoreUpdateFailedException.java @@ -16,6 +16,10 @@ package com.tngtech.archunit.library.freeze; class StoreUpdateFailedException extends RuntimeException { + StoreUpdateFailedException(String message) { + super(message); + } + StoreUpdateFailedException(Throwable cause) { super(cause); } diff --git a/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java b/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java index 69dcc1a5d9..23e39ebf8f 100644 --- a/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java +++ b/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java @@ -39,14 +39,16 @@ 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 java.lang.Boolean.FALSE; +import static java.lang.Boolean.TRUE; import static java.nio.charset.StandardCharsets.UTF_8; class ViolationStoreFactory { - static final String FREEZE_STORE_PROPERTY = "freeze.store"; + static final String FREEZE_STORE_PROPERTY_NAME = "freeze.store"; static ViolationStore create() { - return ArchConfiguration.get().containsProperty(FREEZE_STORE_PROPERTY) - ? createInstance(ArchConfiguration.get().getProperty(FREEZE_STORE_PROPERTY)) + return ArchConfiguration.get().containsProperty(FREEZE_STORE_PROPERTY_NAME) + ? createInstance(ArchConfiguration.get().getProperty(FREEZE_STORE_PROPERTY_NAME)) : new TextFileBasedViolationStore(); } @@ -56,7 +58,7 @@ private static ViolationStore createInstance(String violationStoreClassName) { return (ViolationStore) newInstanceOf(Class.forName(violationStoreClassName)); } catch (Exception e) { String message = String.format("Could not instantiate %s of configured type '%s=%s'", - ViolationStore.class.getSimpleName(), FREEZE_STORE_PROPERTY, violationStoreClassName); + ViolationStore.class.getSimpleName(), FREEZE_STORE_PROPERTY_NAME, violationStoreClassName); throw new StoreInitializationFailedException(message, e); } } @@ -66,20 +68,42 @@ static class TextFileBasedViolationStore implements ViolationStore { private static final Logger log = LoggerFactory.getLogger(TextFileBasedViolationStore.class); private static final Pattern UNESCAPED_LINE_BREAK_PATTERN = Pattern.compile("(? violations) { log.debug("Storing evaluated rule '{}' with {} violations: {}", rule.getDescription(), violations.size(), violations); + if (!storeUpdateAllowed) { + throw new StoreUpdateFailedException(String.format( + "Updating frozen violations is disabled (enable by configuration %s.%s=true)", + FREEZE_STORE_PROPERTY_NAME, ALLOW_STORE_UPDATE_PROPERTY_NAME)); + } String ruleFileName = ensureRuleFileName(rule); write(violations, new File(storeFolder, ruleFileName)); } diff --git a/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java index 797142fcdb..9a9b25b26f 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/freeze/FreezingArchRuleTest.java @@ -21,19 +21,29 @@ import com.tngtech.archunit.lang.ConditionEvent; import com.tngtech.archunit.lang.ConditionEvents; import com.tngtech.archunit.testutil.ArchConfigurationRule; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; import static com.google.common.base.Preconditions.checkNotNull; import static com.tngtech.archunit.core.domain.TestUtils.importClasses; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; import static com.tngtech.archunit.library.freeze.FreezingArchRule.freeze; import static com.tngtech.archunit.testutil.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +@RunWith(DataProviderRunner.class) public class FreezingArchRuleTest { + private static final String STORE_DEFAULT_PATH_PROPERTY_NAME = "freeze.store.default.path"; + private static final String ALLOW_STORE_CREATION_PROPERTY_NAME = "freeze.store.default.allowStoreCreation"; + private static final String ALLOW_STORE_UPDATE_PROPERTY_NAME = "freeze.store.default.allowStoreUpdate"; + private static final String LINE_MATCHER_PROPERTY_NAME = "freeze.lineMatcher"; + @Rule public final ArchConfigurationRule configurationRule = new ArchConfigurationRule(); @@ -239,8 +249,8 @@ public void rejects_illegal_ViolationStore_configuration() { @Test public void default_violation_store_works() throws IOException { - File folder = temporaryFolder.newFolder(); - ArchConfiguration.get().setProperty("freeze.store.default.path", folder.getAbsolutePath()); + useTemporaryDefaultStorePath(); + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "true"); String[] frozenViolations = {"first violation", "second violation"}; FreezingArchRule frozen = freeze(rule("some description") @@ -265,9 +275,24 @@ public void default_violation_store_works() throws IOException { .hasOnlyViolations(frozenViolations[1], "third violation"); } + @Test + public void existing_violation_store_can_be_updated_when_creation_is_disabled() throws IOException { + useTemporaryDefaultStorePath(); + + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "true"); + freeze(rule("first, store must be created").withoutViolations()).check(importClasses(getClass())); + + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "false"); + RuleCreator second = rule("second, store exists"); + assertThat(freeze(second.withViolations("first"))).checking(importClasses(getClass())) + .hasNoViolation(); + assertThat(freeze(second.withViolations("first", "second"))).checking(importClasses(getClass())) + .hasOnlyViolations("second"); + } + @Test public void allows_to_customize_ViolationLineMatcher_by_configuration() { - ArchConfiguration.get().setProperty("freeze.lineMatcher", ConsiderAllLinesWithTheSameStartLetterTheSame.class.getName()); + ArchConfiguration.get().setProperty(LINE_MATCHER_PROPERTY_NAME, ConsiderAllLinesWithTheSameStartLetterTheSame.class.getName()); TestViolationStore violationStore = new TestViolationStore(); createFrozen(violationStore, rule("some description") @@ -286,7 +311,7 @@ public void allows_to_customize_ViolationLineMatcher_by_configuration() { @Test public void rejects_illegal_ViolationLineMatcher_configuration() { String wrongConfig = "SomeBogus"; - ArchConfiguration.get().setProperty("freeze.lineMatcher", wrongConfig); + ArchConfiguration.get().setProperty(LINE_MATCHER_PROPERTY_NAME, wrongConfig); thrown.expect(ViolationLineMatcherInitializationFailedException.class); thrown.expectMessage("freeze.lineMatcher=" + wrongConfig); @@ -315,6 +340,61 @@ public void default_ViolationLineMatcher_ignores_line_numbers() { .hasOnlyViolations(locationClassDoesNotMatch, descriptionDoesNotMatch); } + @Test + public void can_prevent_default_ViolationStore_from_creation() throws IOException { + useTemporaryDefaultStorePath(); + + // Store creation is disabled by default + assertExceptionDueToDisabledViolationStoreCreationWhenCheckingFreezingArchRule(); + + // Explicitly disabling store creation has the same effect + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "false"); + assertExceptionDueToDisabledViolationStoreCreationWhenCheckingFreezingArchRule(); + } + + private void assertExceptionDueToDisabledViolationStoreCreationWhenCheckingFreezingArchRule() { + assertThatThrownBy(new ThrowingCallable() { + @Override + public void call() { + freeze(rule("some description").withoutViolations()).check(importClasses(getClass())); + } + }).isInstanceOf(StoreInitializationFailedException.class) + .hasMessage("Creating new violation store is disabled (enable by configuration " + ALLOW_STORE_CREATION_PROPERTY_NAME + "=true)"); + } + + @Test + public void can_prevent_default_ViolationStore_from_freezing_unknown_rules() throws IOException { + useTemporaryDefaultStorePath(); + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "true"); + + freeze(rule("new rule, updates enabled by default").withoutViolations()).check(importClasses(getClass())); + + ArchConfiguration.get().setProperty(ALLOW_STORE_UPDATE_PROPERTY_NAME, "true"); + freeze(rule("new rule, updates enabled explicitly").withoutViolations()).check(importClasses(getClass())); + + ArchConfiguration.get().setProperty(ALLOW_STORE_UPDATE_PROPERTY_NAME, "false"); + expectStoreUpdateDisabledException(); + freeze(rule("new rule, updates disabled").withoutViolations()).check(importClasses(getClass())); + } + + @Test + public void can_prevent_default_ViolationStore_from_updating_existing_rules() throws IOException { + useTemporaryDefaultStorePath(); + ArchConfiguration.get().setProperty(ALLOW_STORE_CREATION_PROPERTY_NAME, "true"); + + RuleCreator someRule = rule("some description"); + freeze(someRule.withViolations("remaining", "will be solved")).check(importClasses(getClass())); + + ArchConfiguration.get().setProperty(ALLOW_STORE_UPDATE_PROPERTY_NAME, "false"); + expectStoreUpdateDisabledException(); + freeze(someRule.withViolations("remaining")).check(importClasses(getClass())); + } + + private void expectStoreUpdateDisabledException() { + thrown.expect(StoreUpdateFailedException.class); + thrown.expectMessage("Updating frozen violations is disabled (enable by configuration " + ALLOW_STORE_UPDATE_PROPERTY_NAME + "=true)"); + } + private void createFrozen(TestViolationStore violationStore, ArchRule rule) { FreezingArchRule frozen = freeze(rule).persistIn(violationStore); @@ -323,6 +403,11 @@ private void createFrozen(TestViolationStore violationStore, ArchRule rule) { .hasNoViolation(); } + private void useTemporaryDefaultStorePath() throws IOException { + File folder = temporaryFolder.newFolder(); + ArchConfiguration.get().setProperty(STORE_DEFAULT_PATH_PROPERTY_NAME, folder.getAbsolutePath()); + } + private static RuleCreator rule(String description) { return new RuleCreator(description); } @@ -461,4 +546,4 @@ public void handleWith(Handler handler) { throw new UnsupportedOperationException("Implement me"); } } -} \ No newline at end of file +} diff --git a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java index fd43f687ef..4c4c41463a 100644 --- a/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java +++ b/archunit/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java @@ -37,7 +37,9 @@ public class TextFileBasedViolationStoreTest { public void setUp() throws Exception { configuredFolder = new File(temporaryFolder.newFolder(), "notyetthere"); - store.initialize(propertiesOf("default.path", configuredFolder.getAbsolutePath())); + store.initialize(propertiesOf( + "default.path", configuredFolder.getAbsolutePath(), + "default.allowStoreCreation", String.valueOf(true))); } @Test @@ -125,4 +127,4 @@ private ArchRule defaultRule() { private ArchRule rule(String description) { return classes().should().bePublic().as(description); } -} \ No newline at end of file +}