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

transitivelyDependOnClasses unusable with frameworks #780

Closed
To6i opened this issue Jan 31, 2022 · 5 comments · Fixed by #878
Closed

transitivelyDependOnClasses unusable with frameworks #780

To6i opened this issue Jan 31, 2022 · 5 comments · Fixed by #878

Comments

@To6i
Copy link

To6i commented Jan 31, 2022

ArchUnit 0.19.0 introduced the assertion transitivelyDependOnClassesThat() (see #575).

Unfortunately the assertion is unusable when dealing with dependencies to a big framework / SDK which have a lot of dependencies within itself. Consider the following example:

  • MyClass (package com.mypackage)
    • depends on OtherClass
  • OtherClass (package com.intermediatepkg)
    • depends on FrameworkClass1
  • FrameworkClass1 (package com.framework)
    • depends on FrameworkClass2
    • depends on FrameworkClass3
  • FrameworkClass2 (package com.framework)
  • FrameworkClass3 (package com.framework)

If I want to ensure that the my code in com.mypackage has no (direct or indirect) dependencies to any class within com.framework, transitivelyDependOnClassesThat() will report the following dependencies as violations:

  • OtherClass -> FrameworkClass1
  • FrameworkClass1 -> FrameworkClass2
  • FrameworkClass1 -> FrameworkClass3

The report is unusable as MyClass does not even show up, but all dependencies within the framework / SDK itself.

In our case we want to ensure that a specific layer does not have any dependencies to the Android SDK. But Android has thousands of dependencies within itself. So if a single class has an indirect dependency to one Android class, we sometimes end up with multiple thousands (!) of violations with only one root cause. And the root cause (class) is not even visible.

In order to be helpful, transitivelyDependOnClassesThat() should report:

  1. the outgoing dependency path from the checked layer / classes down to the violation
  2. stop reporting any further violations when its based on the same outgoing dependency
@codecholeric
Copy link
Collaborator

Thanks for raising this issue! I understand why this is no good behavior in this case. We'll think about it, how this can be improved.

In the meantime, you could maybe also solve your problem by simply defining a stricter net of rules so that you don't need transitive dependencies at all. I.e. if you define for each of your layers exactly which other layer (and only that layer) may access it, and you also define which layers exactly may access the Android SDK, then there is also no way that the layer you want independent can transitively depend on Android. At least as long as the structure of the layers is set up correctly. This can for example be done fairly concise like this https://www.archunit.org/userguide/html/000_Index.html#_layer_checks (and maybe a separate rule to define the allowed dependency origins to the Android SDK)

@Pfoerd
Copy link
Contributor

Pfoerd commented Mar 10, 2022

@codecholeric we built a customAnyTransitiveDependencyCondition extends ArchCondition for this use case. I could raise a PR if you are interested in it?

It works like this: If you have a rule noClasses().that(match(“de.foo.service..”)).should(haveAnyTransitiveDependencyToClassesIn("android.."))

Then this Architecture
image

would output the following violations:

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

The implementation is very efficient as it only discovers one transitive path for each direct dependency. Of course it is less verbose in it's output because it does not report all "violating" dependency paths but for most of the use cases it is good enough to know all direct dependencies that cause a transitive dependency instead of the whole transitive dependency path.

@Pfoerd
Copy link
Contributor

Pfoerd commented May 24, 2022

@codecholeric, do you have time for some feedback on my suggested solution? Thanks!

@codecholeric
Copy link
Collaborator

Oh sorry, I must have overlooked this 🙈 This looks actually quite nice 👍 😃 So yeah, sure, I'd be interested in adding this! Would you add it as a separate ArchCondition only or also add it to the fluent API? Also wondering if it would be much trouble to offer it (additionally?) with a signature

haveAnyTransitiveDependencyToClassesThat(predicate)

I would guess internally that shouldn't make much difference, since residing in a package is also just a predicate? Because then the condition would be even more flexible 🙂

@Pfoerd
Copy link
Contributor

Pfoerd commented Jun 3, 2022

Thank you for your response! I like your suggested improvements. The "deep" integration via extending the fluent API shouldn't be a problem, nor the additional predicate based signature. I will raise a PR 👍 .

Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 3, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 7, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 7, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 8, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 9, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 9, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 9, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 9, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 9, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 23, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jun 23, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jul 4, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
Pfoerd added a commit to Pfoerd/ArchUnit that referenced this issue Jul 4, 2022
Issue: TNG#780
Signed-off-by: e.solutions <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
codecholeric pushed a commit to Pfoerd/ArchUnit that referenced this issue Jul 8, 2022
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 <[email protected]>
on-behalf-of: @e-esolutions-GmbH <[email protected]>
codecholeric added a commit that referenced this issue Jul 8, 2022
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
![image](https://user-images.githubusercontent.com/17569373/172873445-17111662-30e2-4388-912e-840a105cd2bc.png)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants