eclipse-ee4j / expressly

Expressly, a Jakarta Expression Language implementation
Other
9 stars 5 forks source link

Better Message for Invalid + Operation #15

Open hantsy opened 1 month ago

hantsy commented 1 month ago

I tried to use EL to access the Optional<PhoneNubmer> in a record.

public record PhoneNumber(String countryCode, String number) {
}

And the Customer:

public record Customer(
        String firstName,
        String lastName,
        Optional<PhoneNumber> phoneNumber,
        EmailAddress[] emailAddresses,
        Address address
) {
}

And evaluate the following expression.

 String phoneNumber = elProcessor.eval("customer.phoneNumber.map(t-> t.countryCode+t.number).orElse('NotFound')");
        System.out.println(phoneNumber);

It throws an exception,

java.lang.ClassCastException: class java.lang.Long cannot be cast to 
class java.lang.String (java.lang.Long and java.lang.String are in 
module java.base of loader 'bootstrap')

But there is no Long type field in EL expression.

As suggested by @OndroMih, it is better to show a message to indicate what is wrong, which will help developers to locate the issues quickly.

 customer.phoneNumber.map(t-> t.countryCode[>>> + ]t.number).orElse('NotFound')

BTW: Replace the map() function with map(t -> t.contryCode.concat(t.number)), and it works.

OndroMih commented 1 month ago

Hi, this is not related to Optional at all. It happens for any expression that tries to concatenate strings with the + operator. EL expressions do not support everything that Java syntax does. The + opearator is only supported for numbers, see in the spec. Instead. you should use the += operator, which is only supported for Strings, in the spec. Or the concat method as in your second example.

OndroMih commented 1 month ago

Even if there's nothing to fix as the original expression is invalid, I think the error message should clarify the error and why it happens. Throwing a ClassCastException doesn't clarify much, it doesn't even mention which operator caused the exception.

An exception should mention the operator and expressions on which it operates. Or even mention the whole expression and mark the problematic operator, for example as customer.phoneNumber.map(t-> t.countryCode[>>> + ]t.number).orElse('NotFound'). It should probably be a custom runtime exception that provides these values as fields, not just as in a message, so that client code (e.g. in Mojarra, GlassFish), can handle it better and propagate the error message only in development mode, for example.

OndroMih commented 1 month ago

@hantsy , I suggest that you change this issue so that it is about a better error for expressions with + on string values. Or close it if it's OK and you don't think the error message needs to be changed.

hantsy commented 1 month ago

you should use the += operator, which is only supported for Strings, in the spec. Or the concat method as in your second example.

OMG, contact will make the EL look tedious.

OndroMih commented 1 month ago

Instead of concat, you can use +=, such as:

customer.phoneNumber.map(t-> t.countryCode += t.number).orElse('NotFound')

Just replace + in your original expression with +=, which is for strings.