From 0ace0be29b43ca5a567d69f6804ba41880c63c29 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 25 Mar 2019 16:32:42 -0700 Subject: [PATCH] Fix annotation value mapping regression Refine `TypeMappedAnnotation.getValueFromMetaAnnotation` to correctly account for aliases when finding values from meta-annotations. Prior to this commit, only convention based mappings were considered when searching for values on meta-annotations. This meant that declared meta-annotations that used aliases could return the "default value" rather than the merged value. Closes gh-22654 --- .../annotation/AnnotationTypeMapping.java | 58 ++++++++++++- .../core/annotation/TypeMappedAnnotation.java | 38 ++++----- .../annotation/MergedAnnotationsTests.java | 81 +++++++++++++++++++ 3 files changed, 151 insertions(+), 26 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java index f3c519a4390..0c2e6796819 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java @@ -67,6 +67,10 @@ final class AnnotationTypeMapping { private final int[] conventionMappings; + private final int[] annotationValueMappings; + + private final AnnotationTypeMapping[] annotationValueSource; + private final Map> aliasedBy; private final Set claimedAliases = new HashSet<>(); @@ -92,9 +96,12 @@ final class AnnotationTypeMapping { this.mirrorSets = new MirrorSets(); this.aliasMappings = filledIntArray(this.attributes.size(), -1); this.conventionMappings = filledIntArray(this.attributes.size(), -1); + this.annotationValueMappings = filledIntArray(this.attributes.size(), -1); + this.annotationValueSource = new AnnotationTypeMapping[this.attributes.size()]; this.aliasedBy = resolveAliasedForTargets(); processAliases(); addConventionMappings(); + addConventionAnnotationValues(); } @@ -200,7 +207,7 @@ final class AnnotationTypeMapping { aliases.add(this.attributes.get(i)); collectAliases(aliases); if (aliases.size() > 1) { - processAliases(aliases); + processAliases(i, aliases); } } } @@ -219,7 +226,7 @@ final class AnnotationTypeMapping { } } - private void processAliases(List aliases) { + private void processAliases(int attributeIndex, List aliases) { int rootAttributeIndex = getFirstRootAttributeIndex(aliases); AnnotationTypeMapping mapping = this; while (mapping != null) { @@ -232,6 +239,16 @@ final class AnnotationTypeMapping { } mapping.mirrorSets.updateFrom(aliases); mapping.claimedAliases.addAll(aliases); + if (mapping.annotation != null) { + int[] resolvedMirrors = mapping.mirrorSets.resolve(null, + mapping.annotation, ReflectionUtils::invokeMethod); + for (int i = 0; i < mapping.attributes.size(); i++) { + if (aliases.contains(mapping.attributes.get(i))) { + this.annotationValueMappings[attributeIndex] = resolvedMirrors[i]; + this.annotationValueSource[attributeIndex] = mapping; + } + } + } mapping = mapping.parent; } } @@ -267,6 +284,22 @@ final class AnnotationTypeMapping { } } + private void addConventionAnnotationValues() { + for (int i = 0; i < this.attributes.size(); i++) { + Method attribute = this.attributes.get(i); + AnnotationTypeMapping mapping = this; + while (mapping.depth > 0) { + int mapped = mapping.getAttributes().indexOf(attribute.getName()); + if (mapped != -1 && (this.annotationValueMappings[i] == -1 + || this.annotationValueSource[i].depth > mapping.depth)) { + this.annotationValueMappings[i] = mapped; + this.annotationValueSource[i] = mapping; + } + mapping = mapping.parent; + } + } + } + /** * Method called after all mappings have been set. At this point no further * lookups from child mappings will occur. @@ -389,6 +422,26 @@ final class AnnotationTypeMapping { return this.conventionMappings[attributeIndex]; } + /** + * Return a mapped attribute value from the most suitable + * {@link #getAnnotation() meta-annotation}. The resulting value is obtained + * from the closest meta-annotation, taking into consideration both + * convention and alias based mapping rules. For root mappings, this method + * will always return {@code null}. + * @param attributeIndex the attribute index of the source attribute + * @return the mapped annotation value, or {@code null} + */ + @Nullable + Object getMappedAnnotationValue(int attributeIndex) { + int mapped = this.annotationValueMappings[attributeIndex]; + if (mapped == -1) { + return null; + } + AnnotationTypeMapping source = this.annotationValueSource[attributeIndex]; + return ReflectionUtils.invokeMethod(source.attributes.get(mapped), + source.annotation); + } + /** * Return if the specified value is equivalent to the default value of the * attribute at the given index. @@ -615,5 +668,4 @@ final class AnnotationTypeMapping { } } - } diff --git a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java index 9229b7f1c74..df3fe6583f9 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java @@ -140,7 +140,7 @@ final class TypeMappedAnnotation extends AbstractMergedAnn private Object getValueForMirrorResolution(Method attribute, Object annotation) { int attributeIndex = this.mapping.getAttributes().indexOf(attribute); boolean valueAttribute = VALUE.equals(attribute.getName()); - return getValue(attributeIndex, !valueAttribute, false); + return getValue(attributeIndex, !valueAttribute, true); } @Override @@ -183,7 +183,7 @@ final class TypeMappedAnnotation extends AbstractMergedAnn @Override public boolean hasDefaultValue(String attributeName) { int attributeIndex = getAttributeIndex(attributeName, true); - Object value = getValue(attributeIndex, true, true); + Object value = getValue(attributeIndex, true, false); return value == null || this.mapping.isEquivalentToDefaultValue(attributeIndex, value, this.valueExtractor); } @@ -371,7 +371,7 @@ final class TypeMappedAnnotation extends AbstractMergedAnn @Nullable private T getValue(int attributeIndex, Class type) { Method attribute = this.mapping.getAttributes().get(attributeIndex); - Object value = getValue(attributeIndex, true, true); + Object value = getValue(attributeIndex, true, false); if (value == null) { value = attribute.getDefaultValue(); } @@ -380,7 +380,8 @@ final class TypeMappedAnnotation extends AbstractMergedAnn @Nullable private Object getValue(int attributeIndex, boolean useConventionMapping, - boolean resolveMirrors) { + boolean forMirrorResolution) { + AnnotationTypeMapping mapping = this.mapping; if (this.useMergedValues) { int mappedIndex = this.mapping.getAliasMapping(attributeIndex); @@ -392,7 +393,7 @@ final class TypeMappedAnnotation extends AbstractMergedAnn attributeIndex = mappedIndex; } } - if (resolveMirrors) { + if (!forMirrorResolution) { attributeIndex = (mapping.getDepth() != 0 ? this.resolvedMirrors : this.resolvedRootMirrors)[attributeIndex]; @@ -400,31 +401,22 @@ final class TypeMappedAnnotation extends AbstractMergedAnn if (attributeIndex == -1) { return null; } - Method attribute = mapping.getAttributes().get(attributeIndex); if (mapping.getDepth() == 0) { + Method attribute = mapping.getAttributes().get(attributeIndex); return this.valueExtractor.apply(attribute, this.rootAttributes); } - return getValueFromMetaAnnotation(attribute); + return getValueFromMetaAnnotation(attributeIndex, forMirrorResolution); } @Nullable - private Object getValueFromMetaAnnotation(Method attribute) { - AnnotationTypeMapping mapping = this.mapping; - if (this.useMergedValues && !VALUE.equals(attribute.getName())) { - AnnotationTypeMapping candidate = mapping; - while (candidate != null && candidate.getDepth() > 0) { - int attributeIndex = candidate.getAttributes().indexOf(attribute.getName()); - if (attributeIndex != -1) { - Method candidateAttribute = candidate.getAttributes().get(attributeIndex); - if (candidateAttribute.getReturnType().equals(attribute.getReturnType())) { - mapping = candidate; - attribute = candidateAttribute; - } - } - candidate = candidate.getParent(); - } + private Object getValueFromMetaAnnotation(int attributeIndex, + boolean forMirrorResolution) { + + if (this.useMergedValues && !forMirrorResolution) { + return this.mapping.getMappedAnnotationValue(attributeIndex); } - return ReflectionUtils.invokeMethod(attribute, mapping.getAnnotation()); + Method attribute = this.mapping.getAttributes().get(attributeIndex); + return ReflectionUtils.invokeMethod(attribute, this.mapping.getAnnotation()); } @SuppressWarnings("unchecked") diff --git a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java index 84419c9f35c..e767c15dc32 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java @@ -2036,6 +2036,25 @@ public class MergedAnnotationsTests { assertThat(annotation.getString("text")).isEqualTo("metameta"); } + @Test // gh-22654 + public void getValueWhenHasDefaultOverrideWithImplicitAlias() { + MergedAnnotation annotation1 = MergedAnnotations.from( + DefaultOverrideImplicitAliasMetaClass1.class).get(DefaultOverrideRoot.class); + assertThat(annotation1.getString("text")).isEqualTo("alias-meta-1"); + MergedAnnotation annotation2 = MergedAnnotations.from( + DefaultOverrideImplicitAliasMetaClass2.class).get(DefaultOverrideRoot.class); + assertThat(annotation2.getString("text")).isEqualTo("alias-meta-2"); + } + + @Test // gh-22654 + public void getValueWhenHasDefaultOverrideWithExplicitAlias() { + MergedAnnotation annotation = MergedAnnotations.from( + DefaultOverrideExplicitAliasRootMetaMetaClass.class).get( + DefaultOverrideExplicitAliasRoot.class); + assertThat(annotation.getString("text")).isEqualTo("meta"); + assertThat(annotation.getString("value")).isEqualTo("meta"); + } + // @formatter:off @Retention(RetentionPolicy.RUNTIME) @@ -3359,6 +3378,68 @@ public class MergedAnnotationsTests { } + @Retention(RetentionPolicy.RUNTIME) + @DefaultOverrideRoot + @interface DefaultOverrideImplicitAlias { + + @AliasFor(annotation=DefaultOverrideRoot.class, attribute="text") + String text1() default "alias"; + + @AliasFor(annotation=DefaultOverrideRoot.class, attribute="text") + String text2() default "alias"; + + } + + @Retention(RetentionPolicy.RUNTIME) + @DefaultOverrideImplicitAlias(text1="alias-meta-1") + @interface DefaultOverrideAliasImplicitMeta1 { + + } + + @Retention(RetentionPolicy.RUNTIME) + @DefaultOverrideImplicitAlias(text2="alias-meta-2") + @interface DefaultOverrideImplicitAliasMeta2 { + + } + + @DefaultOverrideAliasImplicitMeta1 + static class DefaultOverrideImplicitAliasMetaClass1 { + + } + + @DefaultOverrideImplicitAliasMeta2 + static class DefaultOverrideImplicitAliasMetaClass2 { + + } + + @Retention(RetentionPolicy.RUNTIME) + @interface DefaultOverrideExplicitAliasRoot { + + @AliasFor("value") + String text() default ""; + + @AliasFor("text") + String value() default ""; + + } + + @Retention(RetentionPolicy.RUNTIME) + @DefaultOverrideExplicitAliasRoot("meta") + @interface DefaultOverrideExplicitAliasRootMeta { + + } + + @Retention(RetentionPolicy.RUNTIME) + @DefaultOverrideExplicitAliasRootMeta + @interface DefaultOverrideExplicitAliasRootMetaMeta { + + } + + @DefaultOverrideExplicitAliasRootMetaMeta + static class DefaultOverrideExplicitAliasRootMetaMetaClass { + + } + // @formatter:on }