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

Cycles not detected #474

Closed
simasch opened this issue Nov 23, 2020 · 7 comments · Fixed by #518
Closed

Cycles not detected #474

simasch opened this issue Nov 23, 2020 · 7 comments · Fixed by #518

Comments

@simasch
Copy link

simasch commented Nov 23, 2020

slices().matching("ch.jtaf.(*)").should().beFreeOfCycles().check(classes);

does not detect cycle between ch.jtaf.ui and ch.jtaf.security packages.

I assume that the problem is that the cycle comes from a subpackage of ch.jtaf.ui (see attchached image).

Screenshot 2020-11-23 161306

@codecholeric
Copy link
Collaborator

Ah, I think you need the pattern ch.jtaf.(*).. instead of ch.jtaf.(*). The expression in total is still a package identifier (compare com.tngtech.archunit.base.PackageMatcher), so ch.jtaf.(*) only matches classes that are exactly in those packages, adding the .. will add all sub-packages as well.
Does that help you?

@simasch
Copy link
Author

simasch commented Dec 19, 2020

Hi Peter,

Unfortunately the test passes even with this expression:

slices().matching("ch.jtaf.(*)..").should().beFreeOfCycles().check(classes);

@codecholeric
Copy link
Collaborator

Do you know how exactly the cycle is formed? I.e. which class from ch.jtaf.ui is depending on ch,jtaf.security and vice versa? ArchUnit still has some missing features that I'm trying to fix at the moment with respect to generic type parameters and method references. So maybe it's one of those causing the cycle? BTW. the most recent version 0.15.0 detects a couple of more occurrences (e.g. generic type parameters and array component types)

@simasch
Copy link
Author

simasch commented Dec 19, 2020

You can check it out yourself. It's a public project: https://github.com/72services/jtaf4

These lines of code that produces the cycle:

SecurityContext

 public static boolean isAccessGranted(Class<?> securedClass) {
        final boolean publicView = LoginView.class.equals(securedClass) || DashboardView.class.equals(securedClass)
            || RouteNotFoundError.class.equals(securedClass);

ConfigureUIServiceInitListener

 private void beforeEnter(BeforeEnterEvent event) {
        final boolean accessGranted = SecurityContext.isAccessGranted(event.getNavigationTarget());
        if (!accessGranted) {
            if (SecurityContext.isUserLoggedIn()) {
                event.rerouteToError(AccessDeniedException.class);
            } else {
                event.rerouteTo(LoginView.class);
            }
        }
    }

image

@codecholeric
Copy link
Collaborator

Ah, I see. Yes, the problem is, that for a simple Foo.class.equals(..) there is no trace in the bytecode (at that place) about Foo.class 🤔
Because in the bytecode this is just a call -> java.lang.Class.equals(..). This has already come up in the past, I have to look into it, if it is possible to detect such accesses in a different way 🤔 I just couldn't find a quick way do this with the ASM library yet (because I'm sure there is at least a reference to Foo in the constant pool in such a case, but I haven't seen a way to get this info through the ASM visitor API)

codecholeric added a commit that referenced this issue Jan 31, 2021
So far ArchUnit has not been able to detect the pure usage of class objects as dependencies. E.g. the following example would not have detected a dependency on `Evil`, since besides the reference to the class object no further dependency on `Evil` (like a field access or method call) has occurred.

```
class Example {
  final Map<Class<?>, Object> association = Map.of(Evil.class, anything);
}
```

With this PR `JavaClass` now knows its `referencedClassObjects`, including the respective line number. Furthermore class objects are now parts of the `dependencies{From/To}Self` of a `JavaClass`.

Resolves: #309 
Issue: #446 
Resolves: #474 
Resolves: #515
@codecholeric
Copy link
Collaborator

@simasch Does the newest release now really detect your cycle? (due to #518) Or is there another issue?

@simasch
Copy link
Author

simasch commented Feb 1, 2021

Hi @codecholeric Yes the fix works. Thanks a lot!

codecholeric added a commit that referenced this issue Feb 21, 2021
So far ArchUnit has not been able to detect the pure usage of class objects as dependencies. E.g. the following example would not have detected a dependency on `Evil`, since besides the reference to the class object no further dependency on `Evil` (like a field access or method call) has occurred.

```
class Example {
  final Map<Class<?>, Object> association = Map.of(Evil.class, anything);
}
```

With this PR `JavaClass` now knows its `referencedClassObjects`, including the respective line number. Furthermore class objects are now parts of the `dependencies{From/To}Self` of a `JavaClass`.

Resolves: #309 
Issue: #446 
Resolves: #474 
Resolves: #515
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