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

Dependency considers annotations #171

Merged
merged 35 commits into from
Jan 9, 2020

Conversation

kosani
Copy link
Contributor

@kosani kosani commented Apr 13, 2019

Dependencies now consider annotations, nested annotations and annotation parameters.

Supported:

  • Dependencies on class, field, method and constructor annotations.
  • Dependencies on annotation parameters.
  • Dependencies on nested annotations (and parameters).

Not supported

  • Method parameter annotations.
  • Meta annotations.

Resolves #136

@codecholeric
Copy link
Collaborator

Just to let you know, I'm on it! Sorry that it took me so long!!
It's surprisingly complex, I've already made some adjustments, one bigger one because I think to be consistent we have to make the JavaAnnotation.owner the closest element where it is declared. Because ThrowsDeclarations for example have the method or constructor where they are declared on as their "owner", so I think to be consistent JavaAnnotation must have the class, but also maybe the field or even the annotation where it is declared as parameter as "owner".
I know that "owner" concept is a little fuzzy, because it's pretty generic 😉
Anyway, thus I'm still on the review, but I hope I'll get through with it within the next days. I'll probably still release a new minor upgrade of ArchUnit, because I don't want to block Java 14 support with this review.

@codecholeric
Copy link
Collaborator

What I also think, is that meta-annotations are actually irrelevant for this issue. Because a meta-annotation is a transitive dependency which we don't consider in any other place, so we should not do it here either.
E.g. if a class B extends a class A and a class declares a field of type B, we don't report a dependency on type A. For me meta-annotations are exactly the same. You can still test this yourself, or we'll add a library function like requested in #253. But it does not have to be covered by the low level API of JavaClass.

@codecholeric codecholeric force-pushed the dependency-considers-annotations branch from 04848cc to 2e56f0b Compare December 7, 2019 18:20
@ghost
Copy link

ghost commented Dec 7, 2019

DeepCode's analysis on #bf0ac8 found:

  • 0 critical issues. ⚠️ 0 warnings and 6 minor issues. ✔️ 3 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@codecholeric
Copy link
Collaborator

@kosani I've finally gotten around to reviewing your PR!! I'm really sorry that it took so long, not gonna make excuses ☹️
Can you look over my changes and see if it makes sense?
Thank you so much for your hard work!! Except from the refactoring from the concept change of "owner", it worked really well, good job 😄
Can't wait to finally merge this into master and have another important piece in place!!

Copy link
Contributor Author

@kosani kosani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecholeric I've looked at the changes.. I had to glance through a few of them since there has been a lot of changes since April. From what I see, some new domain concepts were introduced that remove a lot of clutter that providing annotation dependencies introduced.

Thank you for the review! I will definitely come back and give a more thorugh look, but from what I see, all of this seems very reasonable!

@codecholeric codecholeric force-pushed the dependency-considers-annotations branch from 97334b0 to e6b9109 Compare December 18, 2019 17:55
@codecholeric
Copy link
Collaborator

I just noticed that enum constants as annotation parameters are not recognized yet...
I'll try to fix that over xmas!!

@codecholeric codecholeric force-pushed the dependency-considers-annotations branch from e6b9109 to 4ba75d0 Compare December 20, 2019 12:22
@codecholeric codecholeric force-pushed the dependency-considers-annotations branch from 4ba75d0 to 7788870 Compare January 9, 2020 21:18
Alen Kosanović and others added 16 commits January 9, 2020 22:44
… annotation member dependencies

Issue: TNG#136
Signed-off-by: Alen Kosanović <[email protected]>
Issue: TNG#136
Signed-off-by: Alen Kosanović <[email protected]>
Issue: TNG#136
Signed-off-by: Alen Kosanović <[email protected]>
Issue: TNG#136
Signed-off-by: Alen Kosanović <[email protected]>
Issue: TNG#136
Signed-off-by: Alen Kosanović <[email protected]>
…constructor and method to make sure the behavior is the same for each type of member.

Issue: TNG#136

Signed-off-by: Peter Gafert <[email protected]>
Issue: TNG#136

Signed-off-by: Peter Gafert <[email protected]>
…f an annotation should be the closest "parent" where the annotation is declared. Like a ThrowsDeclaration on a method having that method as owner, an annotation on a method should as well. Likewise, if an annotation is a parameter of another annotation, it should still have that annotation as "owner". That way it is possible to traverse the declaration tree up and do assertions based on the specific case. Unfortunately for the dependency use case, this is a little less convenient, since we're interested in the origin class of the dependency, which now must be determined by traversal, but still the implementation of HasOwner should not be adjusted to a specific need in one place.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
… should be irrelevant (can still be tested via custom condition, etc., but is not part of the Getter)

Issue: TNG#136

Signed-off-by: Peter Gafert <[email protected]>
…endencies of first order. I.e. we do not need to recursively find annotations on annotation parameter types or meta-annotations. This is consistent to all other dependencies, e.g. we do not depend on the super type of a return type of a method, since it is no "direct" dependency, but a transitive one. This also solves the problem of a recursive cycle, because we simply do not need to follow those paths.

Issue: TNG#136

Signed-off-by: Peter Gafert <[email protected]>
…ixed concerns already

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
…es are possibly queried many times, we should only do the creation once (since JavaClass is immutable after construction)

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
Since we now have a consistent approach to finding annotation dependencies, where we handle as well class as member owners, we do not need to distinguish anymore before.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
It is good to have an example illustrating dependencies by nested annotation parameters, and also good to have an integration test covering this complex scenario.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
By completing the annotations in a later step we can avoid the complexity of the annotations Supplier. Since we have to access this supplier anyway for the import process, there is no value in creating annotations lazily.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
Did some quick performance analysis and could not detect any difference between creating the additional Iterable via `Iterables.concat(..)` and executing three loops on Hibernate Core. This way it is quicker to see that all members are handled the same.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
Unfortunately we overlooked to include annotation parameters with enum types into dependencies from annotations. Now all cases should be covered though, because dependencies (besides from primitives or String) can only arise from types, enums or nested annotations and arrays of those. We might have to revisit this, if value types are added to Java.

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
We can actually do a simple `instanceof Object[]` check, in case we have a primitive array, we'll just fall through and not add anything in the second branch (similar as strings and string arrays are ignored, even though we pass the first branch for `String[]`).

Issue: TNG#136
Signed-off-by: Peter Gafert <[email protected]>
So far primitive and string arrays have just been compared as is within `JavaAnnotationAssertion`. Since array equality would have to be compared by `Array.equals(..)` and not arr1.equals(arr2) (reference equality), we have to defensively copy primitive and string arrays to lists to make the assertion work reliably.

Signed-off-by: Peter Gafert <[email protected]>
So far we have only filtered out self dependencies from accesses, it actually can happen in other cases as well though (field or parameter of own type, etc.). Thus we should move this logic closer to the creation of dependencies since it is an invariant of `Dependency`. The signature returning `Optional<Dependency>` is not terribly pretty and expressive, but since it is only internal code, I considered it okay (`Dependency` is not a terribly complex class, so I think it is quick to find the cases when the result is `Optional.absent()`.

Signed-off-by: Peter Gafert <[email protected]>
Analogously to dependencies from annotation to enum parameters being missed, also the inverse one, i.e. dependencies to enum types by being a parameter of an annotation has been missed. We now also register dependencies from an annotation to the enum type, if we encounter an enum parameter of an annotation and consequently if we query the dependencies to an enum type, we will now also find classes with annotations declaring that enum as parameter type.

Signed-off-by: Peter Gafert <[email protected]>
It might prove useful to check for the existence of a certain enum constant within an enum type.

Signed-off-by: Peter Gafert <[email protected]>
Since the code to recursively analyze annotation parameters is complicated, error prone and duplicated, it makes sense to introduce a more sophisticated parameter handling within `JavaAnnotation` itself. This way we can a) separate traversal from actual handling (say what we want to do, not how), b) unify and test the traversal in isolation and c) offer a nicer end user API for handling annotation parameters for `JavaAnnotations` that are not used via Reflection in tests (so far there was pretty much no choice but to use instanceof's and casts requiring some knowledge about the imported annotation type).
Parameter handling is implemented as a Visitor pattern, which is well known and established for complex tree structures (with various types of nodes), where conditional traversal happens in various places.

Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the dependency-considers-annotations branch 2 times, most recently from 80388b9 to bf0ac80 Compare January 9, 2020 22:13
@codecholeric
Copy link
Collaborator

I'm finally done, thank you so much for your hard work @kosani !! And sorry that it took me so long, this issue I definitely underestimated badly. I overlooked the other way around when I added the enum support, thus if you did someEnumType.getDependenciesToSelf() the class bearing an annotation with an enum parameter of that type was not detected. I noticed that there was pretty much the same structure (array traversal and instanceof checks missing the enum case) in ClassGraphCreator, so I unified the traversal logic and added a new annotation API implementing some visitor pattern (if you want to check it out).

@codecholeric codecholeric merged commit 710a157 into TNG:master Jan 9, 2020
codecholeric added a commit that referenced this pull request Feb 21, 2021
Dependencies now consider annotations, nested annotations and annotation parameters.

* Dependencies from and to class, field, method and constructor annotations.
* Dependencies from and to annotation parameters. 
* Dependencies from and to nested annotations (and parameters).
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 this pull request may close these issues.

Dependencies should consider annotations
2 participants