Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup obsolete frozen rules #1407

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+136 −0
Open

cleanup obsolete frozen rules #1407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions archunit/src/main/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,16 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.Properties;
import java.util.Set;
import java.util.UUID;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import com.google.common.base.Predicates;
import com.google.common.base.Splitter;
import com.tngtech.archunit.PublicAPI;
import com.tngtech.archunit.lang.ArchRule;
Expand Down Expand Up @@ -110,6 +115,8 @@ public void initialize(Properties properties) {
log.trace("Initializing {} at {}", TextFileBasedViolationStore.class.getSimpleName(), storedRulesFile.getAbsolutePath());
storedRules = new FileSyncedProperties(storedRulesFile);
checkInitialization(storedRules.initializationSuccessful(), "Cannot create rule store at %s", storedRulesFile.getAbsolutePath());
removeObsoleteRules();
removeObsoleteRuleFiles();
}

private File getStoredRulesFile() {
Expand All @@ -132,6 +139,45 @@ private void checkInitialization(boolean initializationSuccessful, String messag
}
}

private void removeObsoleteRules() {
Set<String> obsoleteStoredRules = storedRules.keySet().stream()
.filter(ruleDescription -> !new File(storeFolder, storedRules.getProperty(ruleDescription)).exists())
.collect(Collectors.toSet());
if (!obsoleteStoredRules.isEmpty() && !storeUpdateAllowed) {
throw new StoreUpdateFailedException(String.format(
"Failed to remove %d obsolete stored rule(s). Updating frozen violations is disabled (enable by configuration %s.%s=true)",
obsoleteStoredRules.size(), ViolationStoreFactory.FREEZE_STORE_PROPERTY_NAME, ALLOW_STORE_UPDATE_PROPERTY_NAME));
}
obsoleteStoredRules.forEach(storedRules::removeProperty);
}

private void removeObsoleteRuleFiles() {
Set<String> ruleFiles = storedRules.keySet().stream()
.map(storedRules::getProperty)
.collect(Collectors.toSet());

List<String> danglingFiles = Arrays.stream(storeFolder.list())
.filter(name -> !name.equals(STORED_RULES_FILE_NAME))
.filter(Predicates.not(ruleFiles::contains))
.collect(toList());

if (!danglingFiles.isEmpty() && !storeUpdateAllowed) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for controlling the new features we can either reuse storeUpdateAllowed property (current implementation) or a new property. reusing the existing property would make this change not backward compatible and is probably not favored. But first I'd like to hear feedback from the maintainers.

throw new StoreUpdateFailedException(String.format(
"Failed to remove %d unreferenced rule files. Updating frozen store is disabled (enable by configuration %s.%s=true)",
danglingFiles.size(), ViolationStoreFactory.FREEZE_STORE_PROPERTY_NAME, ALLOW_STORE_UPDATE_PROPERTY_NAME));

}

for (String fileName : danglingFiles) {
Path path = new File(storeFolder, fileName).toPath();
try {
Files.delete(path);
} catch (IOException e) {
throw new StoreInitializationFailedException("Cannot delete unreferenced rule file: " + fileName, e);
}
}
}

@Override
public boolean contains(ArchRule rule) {
return storedRules.containsKey(rule.getDescription());
Expand Down Expand Up @@ -255,6 +301,15 @@ void setProperty(String propertyName, String value) {
syncFileSystem();
}

void removeProperty(String propertyName) {
loadedProperties.remove(ensureUnixLineBreaks(propertyName));
syncFileSystem();
}

Set<String> keySet() {
return loadedProperties.stringPropertyNames();
}

private void syncFileSystem() {
try (FileOutputStream outputStream = new FileOutputStream(propertiesFile)) {
loadedProperties.store(outputStream, "");
Expand Down
81 changes: 81 additions & 0 deletions ...it/src/test/java/com/tngtech/archunit/library/freeze/TextFileBasedViolationStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -10,6 +11,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.io.Files;
import com.tngtech.archunit.lang.ArchRule;
import org.assertj.core.api.ThrowableAssert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -38,6 +40,79 @@ public void setUp() throws Exception {
"default.allowStoreCreation", String.valueOf(true)));
}

@Test
public void throws_exception_when_there_are_obsolete_entries_in_storedRules_files() throws Exception {
// given
store.save(defaultRule(), ImmutableList.of("first violation", "second violation"));
Properties properties = readProperties(new File(configuredFolder, "stored.rules"));
File ruleViolationsFile = new File(configuredFolder, properties.getProperty(defaultRule().getDescription()));
assertThat(ruleViolationsFile.delete()).isTrue();

// when && then
ThrowableAssert.ThrowingCallable storeInitialization = () -> store.initialize(propertiesOf(
"default.path", configuredFolder.getAbsolutePath(),
"default.allowStoreUpdate", String.valueOf(false)));
assertThatThrownBy(storeInitialization)
.isInstanceOf(StoreUpdateFailedException.class)
.hasMessage("Failed to remove 1 obsolete stored rule(s). Updating frozen violations is disabled (enable by configuration freeze.store.default.allowStoreUpdate=true)");
assertThat(store.contains(defaultRule())).isTrue();
}

@Test
public void deletes_obsolete_entries_from_storedRules_files() throws Exception {
// given
store.save(defaultRule(), ImmutableList.of("first violation", "second violation"));
Properties properties = readProperties(new File(configuredFolder, "stored.rules"));
File ruleViolationsFile = new File(configuredFolder, properties.getProperty(defaultRule().getDescription()));
assertThat(ruleViolationsFile.delete()).isTrue();

// when
store.initialize(propertiesOf("default.path", configuredFolder.getAbsolutePath()));

// then
assertThat(store.contains(defaultRule())).isFalse();
}

@Test
public void throws_exception_when_there_are_unreferenced_files_in_store_directory() throws Exception {
// given
store.save(defaultRule(), ImmutableList.of("first violation", "second violation"));
File propertiesFile = new File(configuredFolder, "stored.rules");
Properties properties = readProperties(propertiesFile);
File ruleViolationsFile = new File(configuredFolder, properties.getProperty(defaultRule().getDescription()));
assertThat(ruleViolationsFile).exists();
properties.remove(defaultRule().getDescription());
storeProperties(propertiesFile, properties);

// when && then
ThrowableAssert.ThrowingCallable storeInitialization = () -> store.initialize(propertiesOf(
"default.path", configuredFolder.getAbsolutePath(),
"default.allowStoreUpdate", String.valueOf(false)));
assertThatThrownBy(storeInitialization)
.isInstanceOf(StoreUpdateFailedException.class)
.hasMessage("Failed to remove 1 unreferenced rule files. Updating frozen store is disabled (enable by configuration freeze.store.default.allowStoreUpdate=true)");
assertThat(ruleViolationsFile).exists();
}

@Test
public void deletes_files_not_referenced_in_storedRules() throws Exception {
// given
store.save(defaultRule(), ImmutableList.of("first violation", "second violation"));
File propertiesFile = new File(configuredFolder, "stored.rules");
Properties properties = readProperties(propertiesFile);
File ruleViolationsFile = new File(configuredFolder, properties.getProperty(defaultRule().getDescription()));
assertThat(ruleViolationsFile).exists();
properties.remove(defaultRule().getDescription());
storeProperties(propertiesFile, properties);

// when
store.initialize(propertiesOf("default.path", configuredFolder.getAbsolutePath()));

// then
assertThat(store.contains(defaultRule())).isFalse();
assertThat(ruleViolationsFile).doesNotExist();
}

@Test
public void reports_unknown_rule_as_unstored() {
assertThat(store.contains(defaultRule())).as("store contains random rule").isFalse();
Expand Down Expand Up @@ -126,6 +201,12 @@ private Properties readProperties(File file) throws IOException {
return properties;
}

private static void storeProperties(File propertiesFile, Properties properties) throws IOException {
try (FileOutputStream outputStream = new FileOutputStream(propertiesFile)) {
properties.store(outputStream, "");
}
}

private Properties propertiesOf(String... keyValuePairs) {
Properties result = new Properties();
LinkedList<String> keyValues = new LinkedList<>(asList(keyValuePairs));
Expand Down