jakartaee / expression-language

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

ELResolver.getType() expected to return null if property is read-only #168

Closed markt-asf closed 2 years ago

markt-asf commented 2 years ago

This isn't explicitly documented anywhere but while working on expanding the TCK to cover the changes in EL 5.0 I found some code that checks ELResolver.getType() returns null if the property is read-only. This makes sense to me given the purpose of getType() but it is not documented anywhere.

It appears that, although the code is in the TCK, it hasn't been used to date.

We could remove the code that check this from the TCK but I think it makes more sense to explicitly document this in the Javadoc for ELResolver.getType() and update the TCK to use the code.

markt-asf commented 2 years ago

Hmm. As I looked into this more, there is an inconsistency with StaticFieldELResolver. It is explicitly documented to a) be read-only and b) have getType() return the type of the field. I'm currently leaning towards changing this to have StaticFieldELResolver.getType() return null instead but would appreciate some community feedback on this.

markt-asf commented 2 years ago

Having disabled the TCK code that was checking for StaticFieldELResolver.getType() returning null for read-only values, I have discovered that there are some TCK tests that do check this. I think this adds weight the argument for making this behaviour consistent for all ELResolvers.

For additional clarity, I think we should be explicit that ELResolvers that are constructed as "read-only" behave as all values are read-only irrespective of whether the underlying value is writable or not. That does mean a change to StaticFieldELResolver.

markt-asf commented 2 years ago

In the absence of any other comments I intend to go ahead with making the getType() behaviour consistent for all ELResolvers in the next day or so.

markt-asf commented 2 years ago

Just a heads up I am starting to work on this.