Add Annotations to MethodExpression / MethodInfo

[tandraschko]

I'm currently working on a prototype for jakartaee/faces#1554

i wonder if we could add it, so we can easily access Anntotation[] from MethodExpression / MethodInfo?

[markt-asf]

getAnnotations() or getDeclaredAnnotations() ? I'm a little hesitant as we don't expose similar objects (e.g. Method) in the API. Could a similar AnnotationInfo object meet your needs?

[tandraschko]

Why dont we need expose the Method itself? Would be much better as introduce AnnotationInfo.

Otherwise im OK with AnnotationInfo too.

[tandraschko]

We may also allow to access annotations of Method params

[tandraschko]

Any thoughts @markt-asf ?

[markt-asf]

It looks to me like it was a conscious design decision not to expose Method instances directly. While I can't find any discussion about this in the archives, I suspect that was due to security concerns. I'm much happier erring on the side of caution and continuing that approach. Looking at how best to expose the annotation info is on my TODO list for this month.

[markt-asf]

I've spent a bit of time looking at this and my current thinking is something along these lines added to MethodExpression:

public abstract Object getAnnotationElementValue(ELContext context, Class<? extends Annotation> type, String elementName);

I plan to continue to pursue a solution along these lines. My current thinking is to implement this in Tomcat to confirm an implementation is possible and, assuming that it is, then add the new abstract method to the API. It would be very helpful if you could confirm if this approach would meet your requirement.

[tandraschko]

Do you think we should make such abstraction over reflection API?
Also this doesnt cover multiple same annotations

[markt-asf]

The abstraction approach rather than providing raw access is consistent with how the EL API has approached similar requirements.

Handling multiple instances of the same annotation would need a slightly different approach. Still on MethodExpression something like:
public abstract AnnotationInfo[] getAnnotationInfo(ELContext context)
where

public class AnnotationInfo {
    Class getType();
    Map<String,Object> getElements();
}

That pushes some of the work onto the client. We could add a Predicate field to the getAnnotationInfo() to reduce the work the client needs to do whilst retaining the option to return info for all annotations.

I have the basics of this coded (but untested) in Tomcat so at this point I'm reasonably confident this is implementable.

[tandraschko]

+1 for AnnotationInfo

[arjantijms]

Hi, I remember we did had a plan to introduce a MethodReference type. I added this in OmniFaces a long time ago: https://omnifaces.org/docs/javadoc/1.11/org/omnifaces/el/MethodReference.html There’s a corresponding ExpressionInspector in OmniFaces to extract / find this method. It’s probably not needed to hide Method out of security reasons, since if you want (see OmniFaces code) you can get it anyway.

…

On Tuesday, September 7, 2021, Thomas Andraschko ***@***.***> wrote: +1 for AnnotationElement — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXFOTQU3RQYEGHQK43DWZTUAZWB7ANCNFSM425HF74A> .

[markt-asf]

The EL implementations will have (or can easily get) the method reference but the only way for EL clients to get it is via reflection.

With the deprecation of security manager, the concept of trusted and untrusted code looks to be going away in the JRE but we haven't reached a conclusion yet about what that means for Jakarta EE. We could expose the Method object now but that assumes that the conclusion of the Jakarta EE discussion about removal of the security manager will reach the same answer as the JRE and I don't want to make that assumption just yet.

I much more comfortable continuing the existing approach of not directly exposing (or requiring permission for client code to use) classes in the java.lang.reflect package for EL 5.0 and revisiting those aspects of the entire EL API once we have a Jakarta EE decision on how to handle the deprecation of the security manager.

[arjantijms]

Hi, Jakarta EE kinda has to remove the Security Manager as it (and important classes like Policy) will be removed from Java SE. Of course, theoretically they could be moved into a separate library, like jaxb and readded to EE. That’s however extremely unlikely, since the security manager never was an easy separate lib like jaxb already was, and afaik there’s literally zero interest let alone resources to maintain such a library.

…

On Wednesday, September 8, 2021, Mark Thomas ***@***.***> wrote: The EL implementations will have (or can easily get) the method reference but the only way for EL clients to get it is via reflection. With the deprecation of security manager, the concept of trusted and untrusted code looks to be going away in the JRE but we haven't reached a conclusion yet about what that means for Jakarta EE. We could expose the Method object now but that assumes that the conclusion of the Jakarta EE discussion about removal of the security manager will reach the same answer as the JRE and I don't want to make that assumption just yet. I much more comfortable continuing the existing approach of not directly exposing (or requiring permission for client code to use) classes in the java.lang.reflect package for EL 5.0 and revisiting those aspects of the entire EL API once we have a Jakarta EE decision on how to handle the deprecation of the security manager. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXFOTXCYQIBNZ54EMCIFA3UA4T7RANCNFSM425HF74A> .

[markt-asf]

I've done a little digging and Annotation looks to be safe to expose directly. There is a comment in the Javadoc for Method.getDeclaredAnnotations() that the caller is free to modify the returned array. That simplifies things and provides what looks to be a reasonable compromise for now. Something like: Annotation[] MethodExpression.getAnnotations(ELContext)

I tend to agree with you that the answer for the security manager will be complete removal. I can think of a few replacements that could be implemented for Jakarta EE (still complex but not as complex as security manager) but I'm not sure how much demand there will be for them. For Tomcat, users using the security manager are a very small minority.

[arjantijms]

Hi, For GlassFish users running with Security Manager enabled are likely a minority too. Purely anecdotal, but I’ve seen very few actual installations use it.

…

On Wednesday, September 8, 2021, Mark Thomas ***@***.***> wrote: I've done a little digging and Annotation looks to be safe to expose directly. There is a comment in the Javadoc for Method.getDeclaredAnnotations() that the caller is free to modify the returned array. That simplifies things and provides what looks to be a reasonable compromise for now. Something like: Annotation[] MethodExpression.getAnnotations(ELContext) I tend to agree with you that the answer for the security manager will be complete removal. I can think of a few replacements that could be implemented for Jakarta EE (still complex but not as complex as security manager) but I'm not sure how much demand there will be for them. For Tomcat, users using the security manager are a very small minority. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXFOTT2O3MO26PAEGWJSJ3UA5CR3ANCNFSM425HF74A> .

[markt-asf]

I'll expose getAnnotations() and getDeclaredAnnotations() as you can be sure if we only expose one, there will be users that want the other.

[markt-asf]

Actually, cancel that. They return the same thing for methods.

[arjantijms]

Hi, What about going for getMethodReference returning new type MethodReference, as per the example above? Omit getMethod() then for now (can be added later via default method on the interface). GetAnnotations can then be neatly grouped in the MethodReference. This also makes the API more consistent with ValueExpression and ValueReference.

…

On Wednesday, September 8, 2021, Mark Thomas ***@***.***> wrote: I'll expose getAnnotations() and getDeclaredAnnotations() as you can be sure if we only expose one, there will be users that want the other. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#154 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXFOTT7RRCBOWVIGHNBDM3UA5DDVANCNFSM425HF74A> .

[markt-asf]

Sounds like a plan. I'll get started. Should have something shortly.

[markt-asf]

A few questions:

• base for literal method expressions? null?
• actualParameters for literal method expressions Object[0]?

[markt-asf]

Implementation committed. I have an implementation ready in Tomcat 10.1.x as well if folks want to experiment with it.

[tandraschko]

Thanks, the code looks good!
I will give it a try in MyFaces next week

[arjantijms]

Thanks @markt-asf for other people looking at this issue, the code was added in this commit:

apache/tomcat@fda4faa