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

Deprecating MessageHandlingMember annotation specific methods #1621

Merged
merged 22 commits into from
Mar 15, 2021

Conversation

smcvb
Copy link
Member

@smcvb smcvb commented Dec 1, 2020

This pull request aims to make the MessageHandlingMember interface less focused on annotations. This means the MessageHandlingMember#hasAnnotation(Class<? extends Annotation>) and MessageHandlingMember#annotationAttributes(Class<? extends Annotation>) methods have been deprecated. Obviously replacements are introduced, which come in the form of:

  • MessageHandlingMember#isA(String)
  • MessageHandlingMember#attributes(String)

This will allow non-annotation focused MessageHandlingMember implementations to provide similar behavior to validate if the member is of a given handler type (the String to provide to the aforementioned methods) and if so to retrieve that handler type's attributes.

Next to this API adjustment, the current AnnotatedMessageHandlingMember should implement these methods too, disregarding the direct usages of annotation classes. To that end, all message handling member annotations are, directly or indirectly, annotated with a new annotation: @HasHandlerAttributes. This allows the framework to based on this meta-annotation find all attributes on each level of an annotation, so that this collection can be used by the AnnotatedMessageHandlingMember to support he new methods.

The find all these attributes, the AnnotationUtils class has been adjusted and appended upon. Adjustment in the form of being able to only retrieve attributes of the target annotation, with potential overridden attributes from higher level annotations which are meta-annotated with the target.

Furthermore a method is included which allows to validate whether a given Annotation Class is meta-annotated with another annotation. This operation is used to check whether an AnnotatedElement like the Executable given to the AnnotatedMessageHandlingMember has annotations which are meta-annotated with HasHandlerAttributes. The two of these AnnotationUtils methods are used to collect all the attributes into a HandlerAttributes object (collected by a HandlerAttributesUtils class). It is this HandlerAttributes which provides the support needed by the AnnotatedMessageHandlingMember to correctly implement the isA(String) and attributes(String) methods.

This pull request should be considered as a form of preparation towards a non-annotation driven MessageHandlingMember.
Note that for any files which have been touched, IDE warnings have been resolved, including some method reordering and JavaDoc uptes.

Introduce the handler attributes container HandlerAttributes alongside
the meta-annotation HasHandlerAttributes. The annotation can be used to
meta-annotate all annotations which contain attributes of importance for
 message handling members. The HandlerAttributes will be used to store
 the attributes in, per annotation/handlerType found

#annotation-api-deprecation
- Move the null or empty string check from BuilderUtils to ObjectUtils
- Adjust AnnotationUtils#findAnnotationAttributes to specify whether all
 attributes on any level of the meta-annotations should be included, or
 if only attributes of the target annotation and overrides on other
 levels should be included
- Add a methods to AnnotationUtils which validates whether a given
Annotation is meta-annotated with another annotation. In doing so, it
should collect all annotations which are, directly or indirectly,
annotated with the subject annotation

#annotation-api-deprecation
Meta-annotated all message handling member annotations with
HasHandlerAttributes so that we can inspect all handlers for the
presence of this annotation

#annotation-api-deprecation
Introduce a static method which can construct a HandlerAttributes object
 by using the new AnnotationUtils functionality to retrieve attributes
 with potential overridden attributes from indirectly annotated
 annotations, combined with the new functionality to validate whether a
 given annotation is meta-annotated with HasHandlerAttributes.

#annotation-api-deprecation
Adjust the MessageHandlingMember by deprecating the annotation focused
methods in favor of generic methods based on handler type. The
AnnotatedMessageHandlingMember will use the HandlerAttributes to return
similar results as were previously provided through the annotation
focused methods. On top of that, resolve some warnings

#annotation-api-deprecation
@smcvb smcvb added Type: Enhancement Use to signal an issue enhances an already existing feature of the project. Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: In Progress Use to signal this issue is actively worked on. labels Dec 1, 2020
@smcvb smcvb added this to the Release 4.5 milestone Dec 1, 2020
@smcvb smcvb self-assigned this Dec 1, 2020
Rename field to attributes

#annotation-api-deprecation
- Remove private classes in favor of public methods
- Make local annotations protected
- Add test case for non HasHandlerAttributes annotated handling member

#annotation-api-deprecation
Move string specific method to a StringUtils class

#1621
Enforce deep unmodifiable map by moving through all entries and making
each value unmodifiable too.

#1621
@sonarcloud
Copy link

sonarcloud bot commented Dec 31, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

90.2% 90.2% Coverage
0.0% 0.0% Duplication

…api-deprecation

# Conflicts:
#	messaging/src/main/java/org/axonframework/messaging/annotation/AnnotatedMessageHandlingMember.java
Instead of claiming the entire set of attributes, allowing to retrieve a
 single attribute would be helpful too. Add a dictionary to simplify the
 retrieval of Axon specific attributes

#1621
Instead of having a single HandlerAttributes object, change it into an
interface with two implementations: an annotation based version and a
default Map version. The former should be used by the
AnnotatedMessageHandlingMember, whereas the latter serves the purpose of
 opening up custom MessageHandlingMember which decide to not use
 annotations at all. Additionally, remove the attributes() and isA()
 methods from the MessageHandlingMember, as they do not add any value.

#1621
@smcvb smcvb requested a review from abuijze March 2, 2021 14:33
The HandlerAttributes are not annotation specific, so should not reside
in the annotation package. Furthermore, we can benefit from an
AbstractHandlerAttributes, to deduplicate code in the Annotated- and
GenericHandlerAttributes class'

#1621
Adjust inspection order of fields to comply with sonar rules.

#1621
Add tests for additional coverage

#1621
- Remove AbstractHandlerAttributes
- Rename Generic- to SimpleHandlerAttributes
- Let AnnotatedHandlerAttributes hold a reference to a
SimpleHandlerAttributes
- Delegate AnnotatedHandlerAttributes operations to the
SimpleHandlerAttributes
- Adjust mergeWith operation to take in a HandlerAttributes
- Add mergeWith to HandlerAttributes interface
- Fine tune javadoc

#1621
Add tests for higher coverage

#1621
@smcvb smcvb dismissed abuijze’s stale review March 11, 2021 11:10

Comments processed

- Rename mergeWith to mergedWith, as it returns a new object instead of
adjusting the existing object
- Drop default constructor for the SimpleHandlerAttributes, as it
doesn't add any direct value
- Add comment specifying attributes map changes are not reflected by a
constructed SimpleHandlerAttributes

#1621
@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.9% 80.9% Coverage
0.0% 0.0% Duplication

@smcvb smcvb merged commit 41e4c2f into master Mar 15, 2021
@smcvb smcvb deleted the feature/annotation-api-deprecation branch March 15, 2021 09:15
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Mar 15, 2021
smcvb added a commit that referenced this pull request Mar 15, 2021
Logging and code style consistency

#1621
@smcvb smcvb changed the title Deprecating MessageHandlerMember annotation specific methods Deprecating MessageHandlingMember annotation specific methods Mar 30, 2021
@rhubarb
Copy link

rhubarb commented Aug 16, 2022

Where does this leave the examples involving using a custom annotation on CommandHandlers and using annotationAttributes to catch them? This doesn't work with the new non-deprecated method because custom annotations are not captured unless they extend Axon annotations

@smcvb
Copy link
Member Author

smcvb commented Aug 29, 2022

The MessageHandlingMember#hasAnnotation and MessageHandlingMember#annotationAttributes methods still work as they used to, regardless of this pull request.
They're just marked deprecated in favor of MessageHandlingMember#attributes(String).

For custom annotation, the user is required to meta-annotate they annotation with HasHandlerAttributes.
This could've been deduced from this paragraph in my original description:

Furthermore a method is included which allows to validate whether a given Annotation Class is meta-annotated with another annotation.
This operation is used to check whether an AnnotatedElement like the Executable given to the AnnotatedMessageHandlingMember has annotations which are meta-annotated with HasHandlerAttributes.
The two of these AnnotationUtils methods are used to collect all the attributes into a HandlerAttributes object (collected by a HandlerAttributesUtils class). It is this HandlerAttributes which provides the support needed by the AnnotatedMessageHandlingMember to correctly implement the isA(String) and attributes(String) methods.

However, I could've been clearer that this behavior is the solution for custom annotation too.
Note that I've adjusted the Reference Guide accordingly, following the issue you've constructed, @rhubarb.
Lastly, I'd want to extend my apologies for the uncertainty I've caused by neglecting the Reference Guide changes following this pull request.

I do hope everything is clear right now.
If not, please let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants