Add a reflection util class to extract generic type and annotations for a class #10962
Conversation
| * @since 4.6.0 | ||
| */ | ||
| @Internal | ||
| public final class AnnotationReflectionUtils { |
There was a problem hiding this comment.
can this be combined with the current ReflectionUtils class? Seems unnecessary to create a new one
There was a problem hiding this comment.
That class doesn't access annotation metadata
There was a problem hiding this comment.
Perhaps put it into ClassUtils in that case
There was a problem hiding this comment.
The same problem as ReflectionUtils, core doesn't see mutable annotation metadata
| * @since 4.6 | ||
| */ | ||
| @Nullable | ||
| public static Argument<?> findImplementationAsArgument(@NonNull Class<?> implementationType, |
There was a problem hiding this comment.
can we rename this resolveArgumentFromGenerics the name here is confusing imo
There was a problem hiding this comment.
Is it understandable that it will return Argument.of(implementedType, ...) not just a generic type?
inject/src/main/java/io/micronaut/context/AnnotationReflectionUtils.java
Outdated
Show resolved
Hide resolved
| for (Annotation annotation : annotations) { | ||
| Map<CharSequence, Object> values = new LinkedHashMap<>(); | ||
| Class<? extends Annotation> annotationType = annotation.annotationType(); | ||
| Method[] methods = annotationType.getMethods(); |
There was a problem hiding this comment.
we should add reflection logging for Graal
There was a problem hiding this comment.
log the methods accessed reflectively with ClassUtils.REFLECTION_LOGGER
| Map<String, AnnotatedType> resolvedVariables = new LinkedHashMap<>(); | ||
| for (int i = 0; i < actualTypeArguments.length; i++) { | ||
| AnnotatedType actualTypeArgument = actualTypeArguments[i]; | ||
| if (actualTypeArgument.getType() instanceof TypeVariable<?>) { |
There was a problem hiding this comment.
this is very common, e.g. ArrayList<T> implements List<T>
There was a problem hiding this comment.
i think this method needs the previous resolved arguments to resolve these type vars
There was a problem hiding this comment.
Not sure what do you mean
There was a problem hiding this comment.
when you have interface A<T>, interface B<T> extends A<T>, class C extends B<String>, this code will fail to propagate the String all the way to A
| throw new IllegalArgumentException("Unknown type: " + type); | ||
| } | ||
|
|
||
| private static Map<String, AnnotatedType> resolvedVariables(AnnotatedType annotatedType, Class<?> type) { |
There was a problem hiding this comment.
why do you need to pass type here? it should always be the raw type of annotatedType, no?
There was a problem hiding this comment.
How else would you get type variable names?
There was a problem hiding this comment.
i would just resolve the type by getting the raw type of annotatedType. it's a bit more clear imo. but if you want to keep it for performance, maybe make it clear that one is the raw type of the other
| return getClassFromType(parameterizedType.getRawType()); | ||
| } | ||
| if (type instanceof GenericArrayType) { | ||
| return Object[].class; |
There was a problem hiding this comment.
should be based on getGenericComponentType
| if (resolvedVariable == null) { | ||
| resolvedVariable = annotatedActualTypeArguments[i]; | ||
| TypeVariable<? extends Class<?>> typeParameter = typeParameters[i]; | ||
| variableBaseType = getClassFromType(typeParameter.getBounds()[0]); |
There was a problem hiding this comment.
getClassFromType does not support TypeVariable but typeParameter.getBounds()[0] can be a TypeVariable
There was a problem hiding this comment.
We don't really support 100% cases, if something else we can try to fix it
| AnnotatedType[] annotatedInterfaces = implementationType.getAnnotatedInterfaces(); | ||
| for (int i = 0; i < annotatedInterfaces.length; i++) { | ||
| AnnotatedType annotatedInterface = annotatedInterfaces[i]; | ||
| Class<?> classFromType = getClassFromType(annotatedInterface.getType()); |
| } | ||
| Class<?>[] interfaces = implementationType.getInterfaces(); | ||
| AnnotatedType[] annotatedInterfaces = implementationType.getAnnotatedInterfaces(); | ||
| for (int i = 0; i < annotatedInterfaces.length; i++) { |
There was a problem hiding this comment.
not sure if it's worth it, but maybe you can merge this loop with the superclass type?
|
|
||
| @Nullable | ||
| private static Argument<?> findResolvedImplementation(Class<?> implementationType, | ||
| Class<?> implementedType, // Interface or an abstract class |
There was a problem hiding this comment.
implementedType is always the raw type of annotatedType
| private static Argument<?> argumentOf(@NonNull AnnotatedType type, | ||
| @NonNull Class<?> interfaceType, | ||
| @NonNull Map<String, AnnotatedType> resolvedVariables) { | ||
| if (type.getType() instanceof TypeVariable<?> typeVariable) { |
There was a problem hiding this comment.
can this happen? i think type is always a concrete type?
There was a problem hiding this comment.
It only resolve the generic type of the implemented class not of the superclasses / interfaces. If you are loocking for that we need to geet it from the resolvd variables.
There was a problem hiding this comment.
what I mean is that I believe that argumentOf is always called with one of the types that is directly declared inside an extends or implements expression, so it can never be a type variable (you cant class A<T> extends T)
|
@yawkat Would you like to refactor it? I don't really understand much of the reflection |
|
i can try tomorrow, meetings all day today |
|
@dstepanov @graemerocher i've pushed a different implementation. I don't think it's much better, but it is a bit more complete |
|
|
Looks fine |
Originaly created in the validation module to pass some of the reflection TCK. Moving to Core to allow using it for some edge cases of Jakarta APIs.