From 1895ed9d397c6aef9553ea785f3171669b079f77 Mon Sep 17 00:00:00 2001 From: alundblad Date: Tue, 22 Oct 2013 12:35:27 +0200 Subject: [PATCH] 8004912: Repeating annotations - getAnnotationsByType(Class) is not working as expected for few inheritance scenarios 8019420: Repeatable non-inheritable annotation types are mishandled by Core Reflection Reviewed-by: jfranck --- src/share/classes/java/lang/Class.java | 8 +- .../classes/java/lang/reflect/Executable.java | 2 +- .../classes/java/lang/reflect/Field.java | 2 +- .../classes/java/lang/reflect/Parameter.java | 2 +- .../annotation/AnnotatedTypeFactory.java | 2 +- .../reflect/annotation/AnnotationSupport.java | 253 +++++++++++++----- .../reflectiveObjects/TypeVariableImpl.java | 2 +- .../NonInheritableContainee.java | 63 +++++ .../repeatingAnnotations/OrderUnitTest.java | 74 +++++ .../RepeatedUnitTest.java | 60 ++++- 10 files changed, 391 insertions(+), 77 deletions(-) create mode 100644 test/java/lang/annotation/repeatingAnnotations/NonInheritableContainee.java create mode 100644 test/java/lang/annotation/repeatingAnnotations/OrderUnitTest.java diff --git a/src/share/classes/java/lang/Class.java b/src/share/classes/java/lang/Class.java index 3ea769d14..64376970a 100644 --- a/src/share/classes/java/lang/Class.java +++ b/src/share/classes/java/lang/Class.java @@ -3314,7 +3314,10 @@ public final class Class implements java.io.Serializable, public A[] getAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(annotationData().annotations, annotationClass); + AnnotationData annotationData = annotationData(); + return AnnotationSupport.getAssociatedAnnotations(annotationData.declaredAnnotations, + annotationData.annotations, + annotationClass); } /** @@ -3344,7 +3347,8 @@ public final class Class implements java.io.Serializable, public A[] getDeclaredAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(annotationData().declaredAnnotations, annotationClass); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(annotationData().declaredAnnotations, + annotationClass); } /** diff --git a/src/share/classes/java/lang/reflect/Executable.java b/src/share/classes/java/lang/reflect/Executable.java index c92c9a57f..bdb86fc1a 100644 --- a/src/share/classes/java/lang/reflect/Executable.java +++ b/src/share/classes/java/lang/reflect/Executable.java @@ -527,7 +527,7 @@ public abstract class Executable extends AccessibleObject public T[] getAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass); } /** diff --git a/src/share/classes/java/lang/reflect/Field.java b/src/share/classes/java/lang/reflect/Field.java index e84b6b242..6e243bc82 100644 --- a/src/share/classes/java/lang/reflect/Field.java +++ b/src/share/classes/java/lang/reflect/Field.java @@ -1123,7 +1123,7 @@ class Field extends AccessibleObject implements Member { public T[] getAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass); } /** diff --git a/src/share/classes/java/lang/reflect/Parameter.java b/src/share/classes/java/lang/reflect/Parameter.java index 2285819c2..d8c992c15 100644 --- a/src/share/classes/java/lang/reflect/Parameter.java +++ b/src/share/classes/java/lang/reflect/Parameter.java @@ -295,7 +295,7 @@ public final class Parameter implements AnnotatedElement { public T[] getAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(declaredAnnotations(), annotationClass); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(declaredAnnotations(), annotationClass); } /** diff --git a/src/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java b/src/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java index 9e01fc59b..0b9d755a2 100644 --- a/src/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java +++ b/src/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java @@ -166,7 +166,7 @@ public class AnnotatedTypeFactory { @Override public T[] getDeclaredAnnotationsByType(Class annotation) { - return AnnotationSupport.getMultipleAnnotations(annotations, annotation); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(annotations, annotation); } // AnnotatedType diff --git a/src/share/classes/sun/reflect/annotation/AnnotationSupport.java b/src/share/classes/sun/reflect/annotation/AnnotationSupport.java index ed1443a5c..4bb33d649 100644 --- a/src/share/classes/sun/reflect/annotation/AnnotationSupport.java +++ b/src/share/classes/sun/reflect/annotation/AnnotationSupport.java @@ -28,109 +28,224 @@ package sun.reflect.annotation; import java.lang.annotation.*; import java.lang.reflect.*; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; public final class AnnotationSupport { + /** - * Finds and returns all annotation of the type indicated by - * {@code annotationClass} from the {@code Map} {@code - * annotationMap}. Looks into containers of the {@code - * annotationClass} (as specified by an the {@code - * annotationClass} type being meta-annotated with an {@code - * Repeatable} annotation). + * Finds and returns all annotations in {@code annotations} matching + * the given {@code annoClass}. + * + * Apart from annotations directly present in {@code annotations} this + * method searches for annotations inside containers i.e. indirectly + * present annotations. * - * @param annotationMap the {@code Map} used to store annotations indexed by their type - * @param annotationClass the type of annotation to search for + * The order of the elements in the array returned depends on the iteration + * order of the provided map. Specifically, the directly present annotations + * come before the indirectly present annotations if and only if the + * directly present annotations come before the indirectly present + * annotations in the map. * - * @return an array of instances of {@code annotationClass} or an empty array if none were found + * @param annotations the {@code Map} in which to search for annotations + * @param annoClass the type of annotation to search for + * @param includeNonInheritedContainees if false, the annoClass must be + * inheritable for the containers to be searched + * + * @return an array of instances of {@code annoClass} or an empty + * array if none were found */ - public static A[] getMultipleAnnotations( - final Map, Annotation> annotationMap, - final Class annotationClass) { - final List res = new ArrayList(); + private static A[] getDirectlyAndIndirectlyPresent( + Map, Annotation> annotations, + Class annoClass, + boolean includeNonInheritedContainees) { + + List result = new ArrayList(); @SuppressWarnings("unchecked") - final A candidate = (A)annotationMap.get(annotationClass); - if (candidate != null) { - res.add(candidate); + A direct = (A) annotations.get(annoClass); + if (direct != null) + result.add(direct); + + if (includeNonInheritedContainees || + AnnotationType.getInstance(annoClass).isInherited()) { + A[] indirect = getIndirectlyPresent(annotations, annoClass); + + if (indirect != null) { + + boolean indirectFirst = direct == null || + containerBeforeContainee(annotations, annoClass); + + result.addAll((indirectFirst ? 0 : 1), Arrays.asList(indirect)); + } } - final Class containerClass = getContainer(annotationClass); - if (containerClass != null) { - res.addAll(unpackAll(annotationMap.get(containerClass), annotationClass)); + @SuppressWarnings("unchecked") + A[] arr = (A[]) Array.newInstance(annoClass, result.size()); + return result.toArray(arr); + } + + + /** + * Equivalent to calling {@code getDirectlyAndIndirectlyPresentAnnotations( + * annotations, annoClass, true)}. + */ + public static A[] getDirectlyAndIndirectlyPresent( + Map, Annotation> annotations, + Class annoClass) { + + return getDirectlyAndIndirectlyPresent(annotations, annoClass, true); + } + + + /** + * Finds and returns all annotations matching the given {@code annoClass} + * indirectly present in {@code annotations}. + * + * @param annotations annotations to search indexed by their types + * @param annoClass the type of annotation to search for + * + * @return an array of instances of {@code annoClass} or an empty array if no + * indirectly present annotations were found + */ + private static A[] getIndirectlyPresent( + Map, Annotation> annotations, + Class annoClass) { + + Repeatable repeatable = annoClass.getDeclaredAnnotation(Repeatable.class); + if (repeatable == null) + return null; // Not repeatable -> no indirectly present annotations + + Class containerClass = repeatable.value(); + + Annotation container = annotations.get(containerClass); + if (container == null) + return null; + + // Unpack container + A[] valueArray = getValueArray(container); + checkTypes(valueArray, container, annoClass); + + return valueArray; + } + + + /** + * Figures out if conatiner class comes before containee class among the + * keys of the given map. + * + * @return true if container class is found before containee class when + * iterating over annotations.keySet(). + */ + private static boolean containerBeforeContainee( + Map, Annotation> annotations, + Class annoClass) { + + Class containerClass = + annoClass.getDeclaredAnnotation(Repeatable.class).value(); + + for (Class c : annotations.keySet()) { + if (c == containerClass) return true; + if (c == annoClass) return false; } - @SuppressWarnings("unchecked") // should be safe annotationClass is a token for A - final A[] emptyTemplateArray = (A[])Array.newInstance(annotationClass, 0); - return res.isEmpty() ? emptyTemplateArray : res.toArray(emptyTemplateArray); + // Neither containee nor container present + return false; } - /** Helper to get the container, or null if none, of an annotation. */ - private static Class getContainer(Class annotationClass) { - Repeatable containingAnnotation = annotationClass.getDeclaredAnnotation(Repeatable.class); - return (containingAnnotation == null) ? null : containingAnnotation.value(); + + /** + * Finds and returns all associated annotations matching the given class. + * + * The order of the elements in the array returned depends on the iteration + * order of the provided maps. Specifically, the directly present annotations + * come before the indirectly present annotations if and only if the + * directly present annotations come before the indirectly present + * annotations in the relevant map. + * + * @param declaredAnnotations the declared annotations indexed by their types + * @param allAnnotations declared and inherited annotations indexed by their types + * @param annoClass the type of annotation to search for + * + * @return an array of instances of {@code annoClass} or an empty array if none were found. + */ + public static A[] getAssociatedAnnotations( + Map, Annotation> declaredAnnotations, + Map, Annotation> allAnnotations, + Class annoClass) { + + // Search declared + A[] result = getDirectlyAndIndirectlyPresent(declaredAnnotations, annoClass); + + // Search inherited + if (result.length == 0) + result = getDirectlyAndIndirectlyPresent(allAnnotations, annoClass, false); + + return result; } - /** Reflectively look up and get the returned array from the the - * invocation of the value() element on an instance of an - * Annotation. + + /* Reflectively invoke the values-method of the given annotation + * (container), cast it to an array of annotations and return the result. */ - private static A[] getValueArray(Annotation containerInstance) { + private static A[] getValueArray(Annotation container) { try { - // the spec tells us the container must have an array-valued - // value element. Get the AnnotationType, get the "value" element - // and invoke it to get the contents. + // According to JLS the container must have an array-valued value + // method. Get the AnnotationType, get the "value" method and invoke + // it to get the content. - Class containerClass = containerInstance.annotationType(); + Class containerClass = container.annotationType(); AnnotationType annoType = AnnotationType.getInstance(containerClass); if (annoType == null) - throw new AnnotationFormatError(containerInstance + " is an invalid container for repeating annotations"); + throw invalidContainerException(container, null); Method m = annoType.members().get("value"); if (m == null) - throw new AnnotationFormatError(containerInstance + - " is an invalid container for repeating annotations"); + throw invalidContainerException(container, null); + m.setAccessible(true); - @SuppressWarnings("unchecked") // not provably safe, but we catch the ClassCastException - A[] a = (A[])m.invoke(containerInstance); // this will erase to (Annotation[]) but we - // do a runtime cast on the return-value - // in the methods that call this method - return a; - } catch (IllegalAccessException | // couldnt loosen security - IllegalArgumentException | // parameters doesn't match + // This will erase to (Annotation[]) but we do a runtime cast on the + // return-value in the method that call this method. + @SuppressWarnings("unchecked") + A[] values = (A[]) m.invoke(container); + + return values; + + } catch (IllegalAccessException | // couldn't loosen security + IllegalArgumentException | // parameters doesn't match InvocationTargetException | // the value method threw an exception - ClassCastException e) { // well, a cast failed ... - throw new AnnotationFormatError( - containerInstance + " is an invalid container for repeating annotations", - e); + ClassCastException e) { + + throw invalidContainerException(container, e); + } } - /* Sanity check type of and return a list of all the annotation - * instances of type {@code annotationClass} from {@code - * containerInstance}. - */ - private static List unpackAll(Annotation containerInstance, - Class annotationClass) { - if (containerInstance == null) { - return Collections.emptyList(); // container not present - } - try { - A[] a = getValueArray(containerInstance); - List l = new ArrayList<>(a.length); - for (int i = 0; i < a.length; i++) - l.add(annotationClass.cast(a[i])); - return l; - } catch (ClassCastException | - NullPointerException e) { - throw new AnnotationFormatError( - String.format("%s is an invalid container for repeating annotations of type: %s", - containerInstance, annotationClass), - e); + private static AnnotationFormatError invalidContainerException(Annotation anno, + Throwable cause) { + return new AnnotationFormatError( + anno + " is an invalid container for repeating annotations", + cause); + } + + + /* Sanity check type of all the annotation instances of type {@code annoClass} + * from {@code container}. + */ + private static void checkTypes(A[] annotations, + Annotation container, + Class annoClass) { + for (A a : annotations) { + if (!annoClass.isInstance(a)) { + throw new AnnotationFormatError( + String.format("%s is an invalid container for " + + "repeating annotations of type: %s", + container, annoClass)); + } } } } diff --git a/src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java b/src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java index 63da5ea75..58a307000 100644 --- a/src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java +++ b/src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java @@ -198,7 +198,7 @@ public class TypeVariableImpl @Override public T[] getAnnotationsByType(Class annotationClass) { Objects.requireNonNull(annotationClass); - return AnnotationSupport.getMultipleAnnotations(mapAnnotations(getAnnotations()), annotationClass); + return AnnotationSupport.getDirectlyAndIndirectlyPresent(mapAnnotations(getAnnotations()), annotationClass); } @Override diff --git a/test/java/lang/annotation/repeatingAnnotations/NonInheritableContainee.java b/test/java/lang/annotation/repeatingAnnotations/NonInheritableContainee.java new file mode 100644 index 000000000..a4a5ff7eb --- /dev/null +++ b/test/java/lang/annotation/repeatingAnnotations/NonInheritableContainee.java @@ -0,0 +1,63 @@ +/* + * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8019420 + * @summary Repeatable non-inheritable annotation types are mishandled by Core Reflection + */ + +import java.util.*; +import java.lang.annotation.*; +import java.lang.reflect.*; + +public class NonInheritableContainee { + + @Retention(RetentionPolicy.RUNTIME) + @Repeatable(InheritedAnnotationContainer.class) + @interface NonInheritedAnnotationRepeated { + String name(); + } + + @Inherited + @Retention(RetentionPolicy.RUNTIME) + @interface InheritedAnnotationContainer { + NonInheritedAnnotationRepeated[] value(); + } + + @NonInheritedAnnotationRepeated(name="A") + @NonInheritedAnnotationRepeated(name="B") + class Parent {} + class Sample extends Parent {} + + + public static void main(String[] args) { + + Annotation[] anns = Sample.class.getAnnotationsByType( + NonInheritedAnnotationRepeated.class); + + if (anns.length != 0) + throw new RuntimeException("Non-@Inherited containees should not " + + "be inherited even though its container is @Inherited."); + } +} diff --git a/test/java/lang/annotation/repeatingAnnotations/OrderUnitTest.java b/test/java/lang/annotation/repeatingAnnotations/OrderUnitTest.java new file mode 100644 index 000000000..211963f23 --- /dev/null +++ b/test/java/lang/annotation/repeatingAnnotations/OrderUnitTest.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* + * @test + * @bug 8004912 + * @summary Unit test for order of annotations returned by get[Declared]AnnotationsByType. + * + * @run main OrderUnitTest + */ + +import java.lang.annotation.*; +import java.lang.reflect.*; + +public class OrderUnitTest { + + public static void main(String[] args) { + testOrder(Case1.class); + testOrder(Case2.class); + } + + private static void testOrder(AnnotatedElement e) { + Annotation[] decl = e.getDeclaredAnnotations(); + Foo[] declByType = e.getDeclaredAnnotationsByType(Foo.class); + + if (decl[0] instanceof Foo != declByType[0].isDirect() || + decl[1] instanceof Foo != declByType[1].isDirect()) { + throw new RuntimeException("Order of directly / indirectly present " + + "annotations from getDeclaredAnnotationsByType does not " + + "match order from getDeclaredAnnotations."); + } + } +} + +@Retention(RetentionPolicy.RUNTIME) +@interface FooContainer { + Foo[] value(); +} + +@Retention(RetentionPolicy.RUNTIME) +@Repeatable(FooContainer.class) +@interface Foo { + boolean isDirect(); +} + + +@Foo(isDirect = true) @FooContainer({@Foo(isDirect = false)}) +class Case1 { +} + + +@FooContainer({@Foo(isDirect = false)}) @Foo(isDirect = true) +class Case2 { +} diff --git a/test/java/lang/annotation/repeatingAnnotations/RepeatedUnitTest.java b/test/java/lang/annotation/repeatingAnnotations/RepeatedUnitTest.java index 674f37fbb..a1d60ffb0 100644 --- a/test/java/lang/annotation/repeatingAnnotations/RepeatedUnitTest.java +++ b/test/java/lang/annotation/repeatingAnnotations/RepeatedUnitTest.java @@ -23,7 +23,7 @@ /* * @test - * @bug 7154390 8005712 8007278 + * @bug 7154390 8005712 8007278 8004912 * @summary Unit test for repeated annotation reflection * * @compile RepeatedUnitTest.java subpackage/package-info.java subpackage/Container.java subpackage/Containee.java subpackage/NonRepeated.java subpackage/InheritedContainee.java subpackage/InheritedContainer.java subpackage/InheritedNonRepeated.java @@ -51,6 +51,12 @@ public class RepeatedUnitTest { inheritedMe3(); inheritedMe4(); + inheritedMe5(); // ContainerOnSuperSingleOnSub + inheritedMe6(); // RepeatableOnSuperSingleOnSub + inheritedMe7(); // SingleAnnoOnSuperContainerOnSub + inheritedMe8(); // SingleOnSuperRepeatableOnSub + + // CONSTRUCTOR checkMultiplier(Me1.class.getConstructor(new Class[0]), 10); @@ -159,6 +165,30 @@ public class RepeatedUnitTest { check(e.getAnnotationsByType(NonRepeated.class)[0].value() == 1000); } + static void inheritedMe5() { + AnnotatedElement e = Me5.class; + check(2 == e.getAnnotations().length); + check(1 == countAnnotation(e, InheritedContainee.class)); + } + + static void inheritedMe6() { + AnnotatedElement e = Me6.class; + check(2 == e.getAnnotations().length); + check(1 == countAnnotation(e, InheritedContainee.class)); + } + + static void inheritedMe7() { + AnnotatedElement e = Me7.class; + check(2 == e.getAnnotations().length); + check(2 == countAnnotation(e, InheritedContainee.class)); + } + + static void inheritedMe8() { + AnnotatedElement e = Me8.class; + check(2 == e.getAnnotations().length); + check(2 == countAnnotation(e, InheritedContainee.class)); + } + static void checkMultiplier(AnnotatedElement e, int m) { // Basic sanity of non-repeating getAnnotation(Class) check(e.getAnnotation(NonRepeated.class).value() == 5 * m); @@ -252,3 +282,31 @@ class Me3 extends Father {} @InheritedContainee(1000) @InheritedContainee(2000) @InheritedContainee(3000) @InheritedContainee(4000) @Containee(1000) @Containee(2000) @Containee(3000) @Containee(4000) class Me4 extends Father {} + + +@InheritedContainer({@InheritedContainee(1), @InheritedContainee(2)}) +class SuperOf5 {} + +@InheritedContainee(3) +class Me5 extends SuperOf5{} + + +@InheritedContainee(1) @InheritedContainee(2) +class SuperOf6 {} + +@InheritedContainee(3) +class Me6 extends SuperOf6 {} + + +@InheritedContainee(1) +class SuperOf7 {} + +@InheritedContainer({@InheritedContainee(2), @InheritedContainee(3)}) +class Me7 extends SuperOf7 {} + + +@InheritedContainee(1) +class SuperOf8 {} + +@InheritedContainee(2) @InheritedContainee(3) +class Me8 extends SuperOf8 {} -- GitLab