jakartaee / expression-language

Jakarta Expression Language
https://eclipse.org/ee4j/el
Other
60 stars 49 forks source link

Generify the API #157

Closed Thihup closed 3 years ago

Thihup commented 3 years ago

(Probably for 5.0) It would be great if the API were generified. There are places where the return is Object but a class is passed, like ELContext::convertToType. There are other places that could be generified too, like ELProcessor::eval, to remove the requirement of a cast of the result.

arjantijms commented 3 years ago

+1

markt-asf commented 3 years ago

What do we want to do about Object ELContext.getContext(Class<?> key) ? The Javadoc states:

By convention, the object returned will be of the type specified by the key. However, this is not required and the key is used strictly as a unique identifier.

Do we enforce the convention or do we retain the current behaviour that the key is strictly just a unique identifier? I've looked at the usage of this API in Tomcat and it would break some optimisation code. My concern isn't strictly about the breakage in Tomcat - the fix would be simple - my concern is that this suggests that there could be breakage elsewhere.

Thoughts?

markt-asf commented 3 years ago

@Thihup Thoughts on Object ELContext.getContext(Class<?> key)? Also any feedback on the generics changes I have already made and/or any additional changes you think could/should be made?

Thihup commented 3 years ago

@markt-asf In JoEL, the getContext is backed by a HashMap, so to me, it made sense to generify. But as Javadoc says it is not required to return the same type, this can be a little tricky.

I tried to generify the ELProcessor.eval too, but this may not be OK. As the result is Object, you probably will cast the result to something else, so I did something like this:

    public <T> T eval(String expression) {
        return (T) getValue(expression, Object.class);
    }
markt-asf commented 3 years ago

I'll add generics to ELProcessor.eval() as you suggest as it removes the need for the user to cast the result. I think we'll have to leave 'ELContext.getContext()` as it is as the risk of breaking backwards compatibility does not seem worth the benefits.

markt-asf commented 3 years ago

Closing, as I think we have done all we can here.