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.

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

[suggestion] Allow the mechanism for creating a file name for frozen violations to be specified via an SPI or similar. #1045

Open
danhaywood opened this issue Jan 20, 2023 · 3 comments

Comments

@danhaywood
Copy link
Contributor

this follows on (somewhat) #1025.

We have a modular monolith, and have set up archunit to run for each module. We are using frozen violations to manage the technical debt in each module, to prevent new violations from being added and to chip away at old ones.

In some circumstances Archunit will recreate the guid files with the current set of violations, which in theory is a good thing but we find that the guid file can sometimes change. Perhaps our workflow is wrong, but at any rate, when multiple developers on current feature branches, we find we get a bunch of git conflicts that need to be resolved.

What we think would work for us is if the file name storing the violations for a rule were deterministic rather than a guid. Looking at the code, this is done at https://github.com/TNG/ArchUnit/blob/main/archunit/src/main/java/com/tngtech/archunit/library/freeze/ViolationStoreFactory.java#L166 , so the suggestion is to provide an SPI that makes this strategy pluggable.

@danhaywood danhaywood changed the title [suggestion] Allow the [suggestion] Allow the mechanism for creating a file name for frozen violations to be specified via an SPI or similar. Jan 20, 2023
@danhaywood
Copy link
Contributor Author

On closer inspection, I see that the code in question is in TextFileBasedViolationStore, so perhaps this is already pluggable. Will dig a bit deeper.

danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 20, 2023
@danhaywood
Copy link
Contributor Author

I've raised a PR #1046 for your consideration.

danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
…hanism can be more easily overridden

Signed-off-by: Dan Haywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
Signed-off-by: Dan Haywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
so that it can be subclassed from any package

Signed-off-by: Dan Haywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
…hanism can be more easily overridden

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
so that it can be subclassed from any package

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 22, 2023
(for inheritance)

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 23, 2023
Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
@danhaywood
Copy link
Contributor Author

In the meantime, I've copied out the TextFileBasedViolationStore from the associated PR #1046, and I've created my subclass to create deterministic file names:

import java.util.ArrayList;
import java.util.List;

import com.tngtech.archunit.thirdparty.com.google.common.base.Joiner;
import com.tngtech.archunit.thirdparty.com.google.common.base.Splitter;

public class DeterministicTextFileBasedViolationStore extends TextFileBasedViolationStore {

    // 100 (directory) + 155 + 4 (extension) = 259  ... Windows has max limit of 260 chars.
    public static final int MAX_LENGTH = 155;

    protected String newRuleFileName(String description) {
        String s = description.replace(' ', '_').replaceAll("[^a-zA-Z0-9_-]", "");
        if(s.length() > MAX_LENGTH) {
            List<String> parts = new ArrayList<>(Splitter.on("_").splitToList(s));
            String candidate = Joiner.on("_").join(parts);
            int partToTruncate = parts.size() - 1;
            while( candidate.length() > MAX_LENGTH) {
                char firstChar = parts.get(partToTruncate).charAt(0);
                parts.set(partToTruncate, firstChar+"");
                partToTruncate--;
                candidate = Joiner.on("_").join(parts);
            }
            s = candidate;
        }
        return s  + ".txt";
    }
}

with

import lombok.RequiredArgsConstructor;

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;

class DeterministicTextFileBasedViolationStore_Test {

    @ParameterizedTest
    @EnumSource(Scenario.class)
    void newRuleFileName(Scenario scenario) {
        Assertions.assertThat(new DeterministicTextFileBasedViolationStore().newRuleFileName(scenario.input)).isEqualTo(scenario.expected);
    }

    @Test
    void long_rule_name() {
        Assertions.assertThat(
                new DeterministicTextFileBasedViolationStore().newRuleFileName(
                        "methods_that_have_name_matching_find_and_have_name_not_matching_findOrCreate_and_are_declared_in_classes_that_are_annotated_with_DomainService_and_are_declared_in_classes_that_have_name_matching_Repository_should_have_raw_return_type_either_Optional_or_Collection"))
                    .isEqualTo(
                            // <=155 characters (plus 100 for the directory name, so less than 260 overall)
                        "methods_that_have_name_matching_find_and_have_name_not_matching_findOrCreate_and_are_declared_in_classes_that_a_a_w_D_a_a_d_i_c_t_h_n_m_R_s_h_r_r_t_e_O_o_C.txt");
    }

    @RequiredArgsConstructor
    enum Scenario {
        lower_accepted          ("abc",   "abc.txt"),
        upper_accepted          ("ABC",   "ABC.txt"),
        number_accepted         ("123",   "123.txt"),
        underscore_accepted     ("a_123", "a_123.txt"),
        hyphen_accepted         ("a-123", "a-123.txt"),
        space_becomes_underscore("a bc",  "a_bc.txt"),
        colon_stripped          ("a:123", "a123.txt"),
        percentage_stripped     ("a%123", "a123.txt"),
        comma_stripped          ("a,123", "a123.txt"),
        period_stripped         ("a.123", "a123.txt"),
        ;
        private final String input;
        private final String expected;
    }
}

danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 25, 2023
…y defined

extension point of TextFileBasedViolationStore

Also marked public methods as final to limit the inheritance contract
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Jan 25, 2023
…y defined

extension point of TextFileBasedViolationStore

Also marked public methods as final to limit the inheritance contract

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
so that it can be subclassed from any package

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
(for inheritance)

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
…y defined

extension point of TextFileBasedViolationStore

Also marked public methods as final to limit the inheritance contract

Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
danhaywood added a commit to danhaywood/ArchUnit that referenced this issue Feb 5, 2023
Signed-off-by: danhaywood <dan@haywood-associates.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant