AnnotationTypeMappings does not filter repeatable annotations#25483
AnnotationTypeMappings does not filter repeatable annotations#25483yilianhuaixiao wants to merge 4 commits into
Conversation
| .findRepeatedAnnotations(metaAnnotation); | ||
| if (repeatedAnnotations != null) { | ||
| for (Annotation repeatedAnnotation : repeatedAnnotations) { | ||
| if (!isMappable(source, metaAnnotation)) { |
There was a problem hiding this comment.
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.
@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.
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.
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.
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.