-
Notifications
You must be signed in to change notification settings - Fork 38.4k
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
AnnotationTypeMappings does not filter repeatable annotations #25483
Conversation
@@ -91,9 +91,6 @@ private void addMetaAnnotationsToQueue(Deque<AnnotationTypeMapping> queue, Annot | |||
.findRepeatedAnnotations(metaAnnotation); | |||
if (repeatedAnnotations != null) { | |||
for (Annotation repeatedAnnotation : repeatedAnnotations) { | |||
if (!isMappable(source, metaAnnotation)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if that was perhaps a copy-and-paste error and if it should rather be:
if (!isMappable(source, repeatedAnnotation)) {
continue;
}
@philwebb, what are your thoughts on the matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philwebb, I'm assuming my above assumption is correct. (I know: a lot of assuming).
In light of that, we'll go forward with the change in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was a typo. A little concerning that no test covered it.
@sbrannen Yes, I think you are right |
@yilianhuaixiao with your latest commit that fixes the issue, please introduce a test that fails before the fix and passes after the fix. |
@sbrannen I added the testcase which can be passed after changing. |
void forAnnotationTypeWhenRepeatableMetaAnnotationFilterd() { | ||
AnnotationTypeMappings mappings = AnnotationTypeMappings.forAnnotationType(WithRepeatedMetaAnnotations.class, | ||
annotationType -> | ||
ObjectUtils.nullSafeEquals(annotationType, Repeating.class.getName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You omitted the import for ObjectUtils
, but don't worry about it. I'll add it before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, I was able to simplify the annotation filter in that test method as follows.
@Test
void forAnnotationTypeWhenRepeatableMetaAnnotationIsFiltered() {
AnnotationTypeMappings mappings = AnnotationTypeMappings.forAnnotationType(WithRepeatedMetaAnnotations.class,
Repeating.class.getName()::equals);
assertThat(getAll(mappings)).flatExtracting(AnnotationTypeMapping::getAnnotationType)
.containsExactly(WithRepeatedMetaAnnotations.class);
}
However, I won't be merging that change due to reasons I'll explain later.
@yilianhuaixiao, thanks again for the PR and the test case! It turns out that It is only invoked from Since none of the annotations from the In light of that, I am changing this from a "bug" to a "task" issue in order to remove the method from the package-private API and to remove the filtering of repeatable annotations altogether. |
Actually, I made a mistake while analyzing this in my IDE. An Thus, we will merge this PR as a bug fix. |
This has been merged into Thanks |
Prior to this commit, AnnotationTypeMappings did not filter repeatable annotations with the supplied annotation filter. Closes spring-projectsgh-25483
since
if (!isMappable(source, metaAnnotation)) { continue; }
is called before and 'source' ,'metaAnnotation' did not changed.
The code should be deleted.