jakartaee / expression-language

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

Add special case for length in ArrayELResolver #175

Closed Thihup closed 1 year ago

Thihup commented 2 years ago

Currently, I couldn't find a good solution to get the size of an array. My solution for now is arr.stream().count(), but I think we could add a special case in ArrayELResolver::getValue to handle the length property and return the array size.

Or is there already some other way to get the array size?

https://stackoverflow.com/questions/70129030/how-do-i-use-el-to-evaluate-the-length-of-an-array-in-a-jsr-380-constraint-mes

markt-asf commented 2 years ago

I can't think of a more direct way just using the EL API.

The most efficient solution with the current API is probably a static method on an imported class.

My only concern with adding the special handling for the property length is that while a similar solution would work for ListELResolver, it wouldn't work for MapELResolver. I'd prefer a consistent approach if possible.

markt-asf commented 1 year ago

I've come back to this several times since it opened and have yet to figure out an approach that would work for MapELResolver as well.

A simple length property is fine for ArrayELResolver and ListELResolver but potentially conflicts with a key of the same name for MapELResolver. Whichever we define as taking precedence in the case of a conflict creates a risk of breakage. I don't like the inconsistency of having the feature for ArrayELResolver and ListELResolver but not MapELResolver.

A new operator could work but we'd need to define the operator and there are no obvious candidates.

A built-in function eg sizeof() could work but sizeof isn't reserved so again that creates the potential for conflicts and breakage. Built-in functions would also be a new concept for the EL spec.

The only thing I don't like about collection.stream().count() is that it is potentially inefficient. A static method on an imported class would address that concern.

So I think this leaves us with two options:

I'm leaning towards do nothing unless someone either comes up with a better solution than the ones I have thought of or a convincing argument to implement the property for just arrays and lists.

SQB commented 1 year ago

How about just for ArrayELResolver? The rationale being that when validating constraints, the default level is BEAN_PROPERTIES. That means that using a stream is not an option. That also means only arrays are available (I think).

So if the level is such that Maps and Lists are available, using streams is an option. If the level is such that using streams is not an option, only arrays are present.

markt-asf commented 1 year ago

To some extent, this feels more like an issue with its origins in JSR 380 than in this specification and normally my inclination would be to suggest that is where the solution should be found. However, after thinking about this for a while I can see the case for a length property handled by ArrayELResolver. While I don't like the inconsistency with lists and maps (they can use size()) the same inconsistency exists in the Java language. So adding support for length would at least be consistently inconsistent ;)

I intend to work on a PR to add handling for a length property to ArrayELResolver.

Thihup commented 1 year ago

What if we added a method "length()" to Array, List, and MapELResolver instead of a property? The specification already includes a way to create a Stream object from an array (2.3.1. Stream and Pipeline The method stream can be used to obtain a Stream from a java.util.Collection or a Java array.)

markt-asf commented 1 year ago

I don't think that helps with JSR 380 when in BEAN_PROPERTIES mode (based on a very quick scan of the spec - happy to be corrected). If we were going to do that, I'd lean more towards adding a size() method to Array.