Bug 43656 - ELSupport.coerceToType won't handle null for java.lang.Number
ELSupport.coerceToType won't handle null for java.lang.Number
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Servlet & JSP API
6.0.14
Other other
: P2 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
: 45953 46325 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2007-10-18 18:02 UTC by Nils Eckert
Modified: 2009-12-19 17:15 UTC (History)
3 users (show)



Attachments
Testcase against ELSupport.coerceToNumber (1.58 KB, text/plain)
2008-04-18 20:19 UTC, Konstantin Kolinko
Details
Testcase and fix for the Integer to Number issue (1.61 KB, patch)
2008-08-03 06:56 UTC, Nils Eckert
Details | Diff
Removed unnecessary Object-creation in coerceToNumber() (1.09 KB, patch)
2008-08-03 07:05 UTC, Nils Eckert
Details | Diff
TLD for tag (1.02 KB, application/octet-stream)
2009-08-18 09:33 UTC, Luke Kolin
Details
JSP and Tag source (2.35 KB, application/x-zip-compressed)
2009-08-18 09:34 UTC, Luke Kolin
Details
zip containing JSP, TLD and java file (1.44 KB, application/x-zip-compressed)
2009-12-11 07:09 UTC, Adam Hardy
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nils Eckert 2007-10-18 18:02:47 UTC
If ELSupport.coerceToType is called with an BigDecimal instance as the first Parameter 'obj' and 
BigDecimal.class as type it should return the inputted value.

Instead the BigDecimal is converted into a double and back into a BigDecimal which leads to a diffenent 
value. This happens in Line 228:

        if (BigInteger.class.equals(type)) {
            if (number instanceof BigDecimal) {
                return ((BigDecimal) number).toBigInteger();
            }
            return BigInteger.valueOf(number.longValue()); // This leads to a different value.
        }

Example: Run the Method with new BigDecimal("0.19"), BigDecimal.class as parameters.
This should result in a BigDecimal with value 0.19. Instead i got 
0.190000000000000002220446049250313080847263336181640625.

 The easiest solution is to compare the class of the object to coerce with the target class and return the 
inputted value if the are equal in ELSupport.coerceToType Method:

public final static Object coerceToType(final Object obj, final Class type) throws 
IllegalArgumentException {
		if (type == null || Object.class.equals(type) || type.equals(obj.getClass())) {
			return obj;
		}
...
Comment 1 Mark Thomas 2008-04-18 11:09:54 UTC
This has been fixed in trunk and proposed for 6.0.x.
Comment 2 Konstantin Kolinko 2008-04-18 20:03:29 UTC
Well, the patch (as proposed in rev. 649639) fixes ELSupport.coerceToType(Object, Class), but the bug itself is in ELSupport.coerceToNumber(Number, Class).

There do exist other ways how coerceToNumber() is called from public methods of ELSupport.

For example, ELSupport.compare(Object, Object) calls it. I suppose, that those methods may suffer until coerceToNumber() itself is fixed.

The fix is OK for coerceToType() though.
Comment 3 Konstantin Kolinko 2008-04-18 20:19:52 UTC
Created attachment 21831 [details]
Testcase against ELSupport.coerceToNumber

It is a test case against ELSupport.coerceToNumber().

The tests with BigDecimal and BigInteger classes are failing.
Comment 4 Mark Thomas 2008-04-19 02:51:39 UTC
Thanks for the test case. I have added the unit test and the necessary fixes to trunk. They have also been proposed for 6.0.x
Comment 5 Mark Thomas 2008-05-01 10:38:10 UTC
This has now been committed to 6.0.x and will be in 6.0.17 onwards.
Comment 6 Luke Kolin 2008-08-02 09:27:15 UTC
I think that this fix may have introduced a regression error, since after upgrading from 6.0.16 to 6.0.18 I am getting this error:

Cannot convert 4,390,241 of type class java.lang.Integer to class java.lang.Number

java.lang.IllegalArgumentException

org.apache.el.lang.ELSupport.coerceToNumber(ELSupport.java:252)
org.apache.el.lang.ELSupport.coerceToNumber(ELSupport.java:265)
org.apache.el.lang.ELSupport.coerceToType(ELSupport.java:353)
org.apache.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:188)
org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate(PageContextImpl.java:925)
org.deltava.jsp.main.home_jsp._jspx_meth_fmt_005fint_005f0(home_jsp.java)
Comment 7 Nils Eckert 2008-08-03 06:56:10 UTC
Created attachment 22354 [details]
Testcase and fix for the Integer to Number issue

This fix returns the input-value if it is assignable to the given type.
Comment 8 Nils Eckert 2008-08-03 07:05:12 UTC
Created attachment 22355 [details]
Removed unnecessary Object-creation in coerceToNumber()

The method protected final static Number coerceToNumber(final Number number, final Class type) created new BigInteger or BigDecimal Objects if the input-value was of the given type. As far as BigInteger and BigDecimal are immutable we can return the input value.

Another possible risk is the number.doubleValue() in the BigDecimal if clause. This behaves different to the coerceToType Method and was replaced with number.toString().
Comment 9 Mark Thomas 2008-10-03 09:22:13 UTC
Thanks for the patch.

I have committed it to trunk and proposed it for 6.0.x
Comment 10 Mark Thomas 2008-10-06 05:09:35 UTC
*** Bug 45953 has been marked as a duplicate of this bug. ***
Comment 11 Mark Thomas 2008-10-07 13:53:18 UTC
(In reply to comment #8)
> Another possible risk is the number.doubleValue() in the BigDecimal if clause.
> This behaves different to the coerceToType Method and was replaced with
> number.toString().

This breaks the spec. I have reverted this part of the patch.
Comment 12 Nils Eckert 2008-10-07 14:13:04 UTC
    public final static Object coerceToType(final Object obj, final Class type)
            throws IllegalArgumentException {
        if (type == null || Object.class.equals(type) ||
                (obj != null && type.isAssignableFrom(obj.getClass()))) {
            return obj;
        }

        ....

    }

behaves in the same way and returns the object if the Type matches.
Comment 13 Mark Thomas 2008-10-08 03:27:33 UTC
I only reverted the change to number.doubleValue()
Comment 14 Mark Thomas 2008-10-27 06:00:40 UTC
Thanks for the fix. It has been applied to 6.0.x and will be included in 6.0.19 onwards.
Comment 15 Mark Thomas 2008-12-02 10:22:21 UTC
*** Bug 46325 has been marked as a duplicate of this bug. ***
Comment 16 Luke Kolin 2009-08-15 11:43:16 UTC
This does not appear to be fixed, but I'm not sure if the problem rests with the code here or JSPC. I get the following error when executing a JSP:

Cannot convert 0 of type class java.lang.Long to class java.lang.Number

java.lang.IllegalArgumentException

org.apache.el.lang.ELSupport.coerceToNumber(ELSupport.java:250)
org.apache.el.lang.ELSupport.coerceToNumber(ELSupport.java:257)
org.apache.el.lang.ELSupport.coerceToType(ELSupport.java:351)
org.apache.el.ValueExpressionImpl.getValue(ValueExpressionImpl.java:188)
org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate(PageContextImpl.java:935)
org.deltava.jsp.admin.diagnostics_jsp._jspx_meth_fmt_005fint_005f8(diagnostics_jsp.java)
org.deltava.jsp.admin.diagnostics_jsp._jspx_meth_el_005ftable_005f0(diagnostics_jsp.java)


Looking at the the method call, I see this in the JSP source:

_jspx_th_fmt_005fint_005f8.setValue((java.lang.Number) org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate("${mapsAPIUsage}", java.lang.Number.class, (PageContext)_jspx_page_context, null, false));

The ELSupport.coerceToNumber(Number, Class) method appears to fail if the Class specified is java.lang.Number; there's no way it can execute without throwing an exception.
Comment 17 Mark Thomas 2009-08-15 12:55:36 UTC
Can you attach the simplest JSP that exhibits this error?
Comment 18 Mark Thomas 2009-08-16 14:04:03 UTC
Mark as needing info
Comment 19 Luke Kolin 2009-08-17 12:09:29 UTC
I don't think I can do this with just a JSP.

My best guess is that it gets triggered when JSP includes a tag that has a parameter which is defined in the TLD as being of type java.lang.Number; then when JSPC compiles the page it takes the type defined in the TLD and passes it to coerceToNumber.

Can I send you a JSP, TLD and a JAR with the tag?
Comment 20 Mark Thomas 2009-08-17 12:25:43 UTC
It is easier if we have access to the source. Can you provide source files for the simplest possible jsp, tld and tag combination that demonstrates the issue?
Comment 21 Luke Kolin 2009-08-18 09:33:36 UTC
Created attachment 24144 [details]
TLD for tag

This is the TLD for the JSP where we pass in Number.class to coerceToNumber.
Comment 22 Luke Kolin 2009-08-18 09:34:49 UTC
Created attachment 24145 [details]
JSP and Tag source

This is the JSP and Tag sources for the JSP where we pass in Number.class to coerceToNumber.
Comment 23 Luke Kolin 2009-08-18 09:35:45 UTC
This is a simple JSP I have created that should exhibit the behavior, along with a stripped-down TLD and tag sources.

The key I believe is to look at the compiled JSP's source and see what is getting passed in the setValue() method call.
Comment 24 Mark Thomas 2009-09-09 04:11:34 UTC
The test case isn't clean (it references a class that doe snot exist) but once I clean it up this works for me with 6.0.20, 6.0.x and trunk.

If you still see this issue with the 6.0.x or later, please provide a working test case that demonstrates the issue.
Comment 25 Luke Kolin 2009-09-14 11:40:12 UTC
Could you send me the compiled JSP source code?
Comment 26 Adam Hardy 2009-12-11 07:09:30 UTC
Created attachment 24692 [details]
zip containing JSP, TLD and java file

I came across this bug today - at least it looks a lot like it. 

If I try to pass in a null to the function, it falls over giving me the following:

java.lang.IllegalArgumentException: Cannot convert 0 of type class java.lang.Long to class java.lang.Number
	org.apache.el.lang.ELSupport.coerceToNumber(ELSupport.java:250)

Which is at best misleading and at worst, just the wrong way to handle nulls. Hopefully this is reproducible everywhere.
Comment 27 Adam Hardy 2009-12-11 07:11:44 UTC
The attached zip causes the stack trace in 6.0.20
Comment 28 Mark Thomas 2009-12-13 14:51:18 UTC
Thanks for the test case. This has been fixed in trunk and proposed for 6.0.x
Comment 29 Mark Thomas 2009-12-19 17:15:58 UTC
Fixed in 6.0.x and will be included in 6.0.21 onwards.