expression-language icon indicating copy to clipboard operation
expression-language copied to clipboard

Add special case for length in ArrayELResolver

Open Thihup opened this issue 4 years ago • 1 comments

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

Thihup avatar Nov 30 '21 20:11 Thihup

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 avatar Dec 01 '21 11:12 markt-asf

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:

  • provide a length property for arrays and lists but not maps
  • do nothing; use .stream().count() or if performance is an issue, a static method on an imported class.

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.

markt-asf avatar Mar 20 '23 17:03 markt-asf

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.

SQB avatar Mar 21 '23 14:03 SQB

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.

markt-asf avatar Mar 21 '23 15:03 markt-asf

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.)

Thihup avatar Mar 21 '23 15:03 Thihup

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.

markt-asf avatar Mar 21 '23 16:03 markt-asf