Bug 57148 - EL type conversion of empty string when there is a PropertyEditor (ELSupport.coerceToType())
Summary: EL type conversion of empty string when there is a PropertyEditor (ELSupport....
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Jasper (show other bugs)
Version: trunk
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-26 22:10 UTC by Konstantin Kolinko
Modified: 2015-02-09 10:53 UTC (History)
0 users



Attachments
2014-10-27_tc8_57148_v1.patch (1.33 KB, patch)
2014-10-26 22:49 UTC, Konstantin Kolinko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2014-10-26 22:10:28 UTC
(Noted when reviewing org.apache.el.lang.ELSupport.coerceToType())

My expectations are that ELSupport.coerceToType() method follows the type conversion rules specified in Expression Language specification.

There is the following phrase in specification 3.0 ch.1.23.7 Coerce A to Any Other Type T:

"If A is a String and T's PropertyEditor throws an exception:"
"If A is "", return null"
"Otherwise, error"

The same phrase in present in EL 2.2 and EL 2.1 specifications (ch.1.18.7) as well.

Consider the following case:
- A is an empty string ("")
- T has a PropertyEditor

Expected behaviour:
- Call PropertyEditor with value of "".
- If it throws an exception, return null.
- Otherwise use the converted value.
try {
 propertyEditor.setAsText("").getValue();
} catch (RuntimeException e) {
 return null;
}

Actual implementation:
- It skips calling the PropertyEditor and immediately returns null.

 if (obj instanceof String) {
   if ("".equals(obj))
        return null;
   PropertyEditor editor = PropertyEditorManager.findEditor(type);
 ...

The PropertyEditor.setAsText(String) method is documented to throw IllegalArgumentException on invalid values. I think that EL API expects an ELException, so it is to be wrapped somewhere.
Comment 1 Konstantin Kolinko 2014-10-26 22:49:27 UTC
Created attachment 32147 [details]
2014-10-27_tc8_57148_v1.patch

Patch for current trunk. Not tested.
Comment 2 Mark Thomas 2014-10-29 23:03:07 UTC
Added some unit tests that confirm the patch is correct and applied to 8.0.x for 8.0.15 owards.

This needs to be back-ported so changing version.
Comment 3 Mark Thomas 2015-02-09 10:53:00 UTC
Fixed in 7.0.x for 7.0.6 onwards.
Comment 4 Mark Thomas 2015-02-09 10:53:21 UTC
Correction. 7.0.60 onwards.