TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.18k stars 287 forks source link

ArchUnit: how can one silence references to arrays of basic types for method mayOnlyAccessLayers(...)? #887

Closed mmoser18 closed 2 years ago

mmoser18 commented 2 years ago

For a big GWT-based application I am defining an architecture check to prevent that our junior programmers accidentally reference classes that are not GWT-serializable and the resulting objects then can not make it "over the wire" (which then throws exceptions at runtime).

For this I am using a layer-check like so:

    @ArchTest
    void checkGWTAccess(JavaClasses classes) {
        LayeredArchitecture layers = Architectures.layeredArchitecture()
            .layer("Client").definedBy("..client..")
            .layer("Shared").definedBy("..shared..")
            ... <many entries omitted here> ...
            ;

        ArchRule rule = layers
            .whereLayer("Client").mayOnlyAccessLayers("Shared", "DomDtos", "DomBase", "DomConfig",                                  
                                                      ... <many entries omitted here> ...
                                                     );
            ;
        rule.check(classes);

"In principle" this works ok EXCEPT that I am still getting errors for all fields that are arrays of basic types or collections of basic types (i.e. e.g. int[] digits;, char[] buffer;, List<Integer> allowedValues;) as well as methods returning such arrays or collections (i.e. e.g. int[] methodA(...) {...}, String[] methodB(...) {...}, List<Character> methodC(...) {...}, etc.).

These error read e.g.:

Field <com.example.client.ui.components.converter.TimeFieldConverter.timeNumberDigits> has type <[I> in (TimeFieldConverter.java:0)

IMHO this is a bug in ArchUnit. I can not fancy any reason arrays of basic types should possibly violate any layer-check rule.

In case this is NOT a bug: How can one exclude these types from a <layer>.mayOnlyAccessLayers(...)-check?

Or - alternatively - can one include these types to the list of allowed layers (if it's possible to identify the and/or declare them as "layer")?

The purpose is simply, to avoid getting any rule violations for these references, so that the tests run green.

codecholeric commented 2 years ago

Yes, LayeredArchitecture is missing an option to specify how to deal with dependencies, similar to PlantUmlArchCondition. You can probably find an answer and how to mitigate your problem here though -> #841 Does that help you?

mmoser18 commented 2 years ago

==> Does that help you?

Unfortunately not. As described in the referenced append I added a

            .ignoreDependency(DescribedPredicate.alwaysTrue(),
                              JavaClass.Predicates.resideInAnyPackage("java..", "javax.."))

to the layers definition - i.e. it now reads:

        LayeredArchitecture layers = Architectures.layeredArchitecture()
            .layer("Client").definedBy("..client..")
            ...
            .ignoreDependency(DescribedPredicate.alwaysTrue(),
                              JavaClass.Predicates.resideInAnyPackage("java..", "javax.."))

but I still get a dozen or so "false" errors like

Method <com.example.client.ui.components.converter.MaskedNumberFieldConverter.mask(java.lang.String, char, int, [I)> has parameter of type <[I> in (MaskedNumberFieldConverter.java:0)

Or did I miss something?

BTW: I also tried to add tried misc. variants of:

            .ignoreDependency("..", "[I")
            .ignoreDependency("..", "<[I>")
            .ignoreDependency("*", "[I")
            .ignoreDependency("*", "<[I>")
            ...

but - as expected - that didn't help, either.

mmoser18 commented 2 years ago

Re-reading the referenced append I finally found an expression that helped:

I added the following clause to the layeredArchitecture()-definition:

            .ignoreDependency(DescribedPredicate.alwaysTrue(), JavaClass.Predicates.simpleNameContaining("["))

... and that did the trick! :-)

Thanks for pointing me to a solution!

codecholeric commented 2 years ago

Glad you could solve the problem! :smiley: But I agree that it shouldn't be such a struggle! I'll try to make this simpler by guiding users to make a choice how to handle dependencies in the beginning that covers typical use cases. Which IMHO are as supported by the PlantUML condition, i.e. "all dependencies" (same behavior as now but I need to explicitly do ignores), "all dependencies in packages x, y, z" (which is good for "I want all classes in my app root package") and "all dependencies in the layered architecture" (which is good for "I'm really only interested in classes that are contained in these specified layers"). IMHO each has advantages and disadvantages, that's why I don't want to make the choice and only support one.

BTW: A slightly dirty way to ignore all those primitives and primitive arrays in one shot is to simply ignore package "". Because typically the default package doesn't contain any relevant classes and all those primitives are counted as residing in an empty package by ArchUnit.

mmoser18 commented 2 years ago

Triggered by your remark I tried your suggestion and instead added: .ignoreDependency(DescribedPredicate.alwaysTrue(), JavaClass.Predicates.resideInAPackage("")) (instead of my own solution .ignoreDependency(DescribedPredicate.alwaysTrue(), JavaClass.Predicates.simpleNameContaining("[]"))) and that worked nicely. Thanks for the hint!

codecholeric commented 2 years ago

FYI: With the next release you should simply be able to write

layeredArchitecture().consideringOnlyDependenciesInAnyPackage("com.myapp..")

or

layeredArchitecture().consideringOnlyDependenciesInLayers()

That should also solve the problem without any custom ignore...