Improve ArchCondition that checks for transitive dependencies #878

Pfoerd · 2022-06-09T08:02:42Z

This adds an ArchCondition to the fluent API to check whether there is any matching transitive dependency. This is a much more efficient variant of #transitivelyDependOnClassesThat that can be used to detect transitive dependencies in a huge codebase or to classes in large 3rd-party libraries like the Android SDK. It focuses on detecting all direct dependencies of the selected classes that are themselves matched or have any transitive dependencies on matched classes. Thus, it doesn't discover all possible dependency paths but stops at the first match to be fast and resource-friendly.

Sample usage:

noClasses().that.resideInAPackage(“de.foo.service..”)
    .should().transitivelyDependOnAnyClassesThat.resideInAPackage("android..")

Then this Architecture

would output the following violations:

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that reside in a package 'de.foo.service..' should transitively depend on any classes that reside in a package ['android..'] was violated (3 times):
Class <de.foo.service.B> transitively depends on <android.I> by [F->E->android.I] in (B.java:0)
Class <de.foo.service.B> transitively depends on <android.I> by [E->android.I] in (B.java:0)
Class <de.foo.service.C> transitively depends on <android.H> by [D->android.H] in (C.java:0)

Resolves: #780
Resolves: #826

Pfoerd · 2022-06-20T11:36:13Z

Hi @codecholeric. Do you have time for some feedback on the PR? And maybe you could approve the execution of workflows? Thanks!

codecholeric

Thanks a lot, looks really nice and solves the issue with overboarding transitive violations very nicely 😄
Hope you're not regretting asking for feedback, because I usually have a lot of comments and questions 😂

My biggest pain point in general is the semantics of the name transitivelyDependOnAnyClassesThat vs transitivelyDependOnClassesThat is super vague to me 🤔 Because I would argue that "depend on any classes that" and "depend on classes that" means exactly the same?

I'm starting to wonder if we even need both versions. Are there really use cases where I would want to use the old version transitivelyDependOnClassesThat? What do you think @hankem? Because if we can't think of any scenarios for the existing version vs the new version, maybe we should just replace it?

...nit/src/main/java/com/tngtech/archunit/lang/conditions/AnyTransitiveDependencyCondition.java

archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/ShouldClassesThatTest.java

archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShould.java

Pfoerd · 2022-06-23T12:16:53Z

Hi, no regrets here, i really appreciate comprehensive feedback :) ! I was able to correct most of your comments right away, see my comments with the rest

I was struggling with the namings around transitivelyDependOnAnyClasses too. I like your idea to just replace the old implementation (at least because it solves my naming issue here 😆 ).

hankem · 2022-06-25T15:44:46Z

I don't mind replacing the old transitivelyDependOnClassesThat at all. Thanks!

Pfoerd · 2022-06-27T05:54:14Z

I don't mind replacing the old transitivelyDependOnClassesThat at all. Thanks!

@codecholeric should I then remove the "old" implementation, keep the original fluent API transitivelyDependOnClasses and just replace the implementation?

codecholeric · 2022-07-03T05:35:59Z

I don't mind replacing the old transitivelyDependOnClassesThat at all. Thanks!

@codecholeric should I then remove the "old" implementation, keep the original fluent API transitivelyDependOnClasses and just replace the implementation?

Yes, I think this makes sense! Because it will make the API easier to understand and if we can't think of any use case where the other implementation is more appropriate, then we should just remove it. It will be a breaking change though, so perfect opportunity now for ArchUnit 1.0 😉
As a side effect it will get rid of your test naming problem 😉 Then you can freely rename TestClass2, right? 😁

...nit/src/main/java/com/tngtech/archunit/lang/conditions/AnyTransitiveDependencyCondition.java

Pfoerd · 2022-07-04T12:01:33Z

Okay I removed the existing implementation and solved the rest of your comments. From my point of view the PR is now ready to merge?

This convenience method also helps users to create standard ArchUnit violation messages, so we offer it as public API. Since we meanwhile have a Java 8 source level we can even put this method into `ConditionEvent` where it fits better. Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>

The original implementation had the problem that it reported every single transitive dependency that matched the predicate. This would mean, even if a transitive dependency got reported as violation, all further transitive dependencies of that dependency would still be analyzed and potentially also reported as violation. For rules like ``` noClasses() .that().resideInAPackage("..example..") .should().transitivelyDependOnClassesThat().resideInAPackage("..some.framework..") ``` this would mean thousands of violations, even for classes that are only reached through other framework classes. We now do a depth first search approach for all dependency paths that are outgoing from the analyzed classes until we find a matching target. Then we stop and report the path. This way all further dependencies will be omitted and the analysis will complete a lot faster. Issue: TNG#780 Signed-off-by: e.solutions <17569373+Pfoerd@users.noreply.github.com> on-behalf-of: @e-esolutions-GmbH <info@esolutions.de>

codecholeric · 2022-07-08T10:43:59Z

Yes, looks good now 🥳 I did some minor changes, e.g. there was a commit in the other PR to more conveniently create condition messages that I cherry-picked in here to merge with this one instead. Other than that very nice and ready to merge 😄

codecholeric · 2022-07-08T10:45:14Z

Ah, but I just saw that I can't push my changes, permission denied 😞 Would you mind giving me access? I also rebased on main to solve that issue and added the one missing copyright note...

Pfoerd · 2022-07-08T11:17:05Z

Ah, but I just saw that I can't push my changes, permission denied 😞 Would you mind giving me access? I also rebased on main to solve that issue and added the one missing copyright note...

done!

Pfoerd · 2022-07-08T13:54:34Z

@codecholeric thx for your support! Looking forward to see this in ArchUnit 1.0 ❤️

codecholeric · 2022-07-08T14:25:13Z

Well, thanks for your contribution 😄 Without it I would certainly not have gotten around to improve this now...

Pfoerd force-pushed the feat/780/add_any_transitive_dependency_arch_condition branch from bfe8370 to 747c277 Compare June 9, 2022 09:15

codecholeric requested changes Jun 22, 2022

View reviewed changes

codecholeric reviewed Jun 22, 2022

View reviewed changes

archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/ClassesShould.java Show resolved Hide resolved

Pfoerd force-pushed the feat/780/add_any_transitive_dependency_arch_condition branch 2 times, most recently from 0c042b3 to 77831df Compare June 23, 2022 15:59

Pfoerd requested a review from codecholeric June 27, 2022 05:54

codecholeric reviewed Jul 3, 2022

View reviewed changes

...nit/src/main/java/com/tngtech/archunit/lang/conditions/AnyTransitiveDependencyCondition.java Outdated Show resolved Hide resolved

codecholeric reviewed Jul 3, 2022

View reviewed changes

...nit/src/main/java/com/tngtech/archunit/lang/conditions/AnyTransitiveDependencyCondition.java Outdated Show resolved Hide resolved

Pfoerd force-pushed the feat/780/add_any_transitive_dependency_arch_condition branch 2 times, most recently from e08d73f to eba0543 Compare July 4, 2022 11:52

Pfoerd requested a review from codecholeric July 4, 2022 11:53

codecholeric and others added 2 commits July 8, 2022 17:26

codecholeric force-pushed the feat/780/add_any_transitive_dependency_arch_condition branch from eba0543 to 499020a Compare July 8, 2022 12:10

codecholeric approved these changes Jul 8, 2022

View reviewed changes

codecholeric changed the title ~~Add arch condition to check for any transitive dependency~~ Improve ArchCondition to check for transitive dependencies Jul 8, 2022

codecholeric changed the title ~~Improve ArchCondition to check for transitive dependencies~~ Improve ArchCondition that checks for transitive dependencies Jul 8, 2022

codecholeric merged commit 5e56c99 into TNG:main Jul 8, 2022

Pfoerd deleted the feat/780/add_any_transitive_dependency_arch_condition branch July 8, 2022 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ArchCondition that checks for transitive dependencies #878

Improve ArchCondition that checks for transitive dependencies #878

Pfoerd commented Jun 9, 2022 •

edited by codecholeric

Loading

Pfoerd commented Jun 20, 2022

codecholeric left a comment

Pfoerd commented Jun 23, 2022

hankem commented Jun 25, 2022

Pfoerd commented Jun 27, 2022

codecholeric commented Jul 3, 2022

Pfoerd commented Jul 4, 2022

codecholeric commented Jul 8, 2022

codecholeric commented Jul 8, 2022

Pfoerd commented Jul 8, 2022

Pfoerd commented Jul 8, 2022

codecholeric commented Jul 8, 2022

Improve ArchCondition that checks for transitive dependencies #878

Improve ArchCondition that checks for transitive dependencies #878

Conversation

Pfoerd commented Jun 9, 2022 • edited by codecholeric Loading

Pfoerd commented Jun 20, 2022

codecholeric left a comment

Choose a reason for hiding this comment

Pfoerd commented Jun 23, 2022

hankem commented Jun 25, 2022

Pfoerd commented Jun 27, 2022

codecholeric commented Jul 3, 2022

Pfoerd commented Jul 4, 2022

codecholeric commented Jul 8, 2022

codecholeric commented Jul 8, 2022

Pfoerd commented Jul 8, 2022

Pfoerd commented Jul 8, 2022

codecholeric commented Jul 8, 2022

Pfoerd commented Jun 9, 2022 •

edited by codecholeric

Loading