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.

Sign up for GitHub

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

Jump to bottom

AnnotationTypeMappings does not filter repeatable annotations #25483

Closed
wants to merge 4 commits into from
Closed

AnnotationTypeMappings does not filter repeatable annotations #25483

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e58c26
code optimized
yilianhuaixiao Jul 28, 2020
1a17007
fix
yilianhuaixiao Jul 28, 2020
0796bff
add #25483 test case
yilianhuaixiao Jul 29, 2020
ed09931
polishing
yilianhuaixiao Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMappings.java
View file
Open in desktop
Expand Up @@ -91,7 +91,7 @@ private void addMetaAnnotationsToQueue(Deque<AnnotationTypeMapping> queue, Annot
.findRepeatedAnnotations(metaAnnotation);
if (repeatedAnnotations != null) {
for (Annotation repeatedAnnotation : repeatedAnnotations) {
if (!isMappable(source, metaAnnotation)) {
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

if (!isMappable(source, repeatedAnnotation)) {
continue;
}
addIfPossible(queue, source, repeatedAnnotation);
Expand Down
10 changes: 10 additions & 0 deletions ...g-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java
View file
Open in desktop
Expand Up @@ -474,6 +474,16 @@ private Method[] resolveMirrorSets(AnnotationTypeMapping mapping, Class<?> eleme
return result;
}

@Test
void forAnnotationTypeWhenRepeatableMetaAnnotationFilterd() {
AnnotationTypeMappings mappings = AnnotationTypeMappings.forAnnotationType(WithRepeatedMetaAnnotations.class,
annotationType ->
ObjectUtils.nullSafeEquals(annotationType, Repeating.class.getName()));
Copy link
Member

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.

Copy link
Member

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.

assertThat(getAll(mappings)).flatExtracting(
AnnotationTypeMapping::getAnnotationType).containsExactly(
WithRepeatedMetaAnnotations.class);
}

@Nullable
private Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) {
int mapped = mapping.getAliasMapping(attributeIndex);
Expand Down