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

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

Closed
mmoser18 opened this issue Jun 14, 2022 · 6 comments · Fixed by #892

Comments

@mmoser18
Copy link

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
Copy link
Collaborator

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
Copy link
Author

mmoser18 commented Jun 15, 2022

==> 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
Copy link
Author

mmoser18 commented Jun 15, 2022

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
Copy link
Collaborator

Glad you could solve the problem! 😃 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
Copy link
Author

mmoser18 commented Jun 21, 2022

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
Copy link
Collaborator

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...

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

Successfully merging a pull request may close this issue.

2 participants