Commons Lang
  1. Commons Lang
  2. LANG-75

NumberUtils.createBigDecimal("") NPE in Sun 1.3.1_08

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Hello,

      The NumberUtils.createXXX methods all have the following pattern:

      public static XXX createXXX(String str) {
      if (str == null)

      { return null; }

      In the case of BigDecimal, passing in a "" to new BigDecimal(String) in Sun
      1.3.1_08 blows up like this:

      java.lang.StringIndexOutOfBoundsException: String index out of range: 0
      at java.lang.String.charAt(String.java:582)
      at java.math.BigDecimal.<init>(BigDecimal.java:124)
      at org.apache.commons.lang.math.NumberUtils.createBigDecimal(NumberUtils.java:4
      78)
      at org.apache.commons.lang.math.NumberUtilsTest.testCreateBigDecimal(NumberUtil
      sTest.java:209)

      Under Sun 1.4.2, you get a NumberFormatException if the length of the string is
      0 (no trim()). The unit tests expect a NumberFormatException when you pass in "".

      So... to make this all nice on 1.3, should all of the guard clauses become:

      (1)

      if (StringUtil.isEmpty(str)) { return null; }

      (2)

      if (StringUtil.isBlank(str))

      { return null; }

      (3)

      if (str == null) { return null; }

      if (StringUtil.isEmpty(str))

      { return str; }

      ?

      I think (2) would be good since it would not blow up on "" AND " " but I am not
      familiar with the various invocation contexts, so, please opine.

      Thanks,
      Gary

        Activity

        Hide
        ggregory@seagullsw.com added a comment -

        Great, thanks for tidying things up.

        Show
        ggregory@seagullsw.com added a comment - Great, thanks for tidying things up.
        Hide
        Stephen Colebourne added a comment -

        OK, I've had a look at this.

        I agree with Phil's last comments that the extra checking is generally not
        required, and that we should treat this as a JDK bug workaround. (Performance
        matters too)

        So, I've removed the validate method and retested on JDK1.2.2, 1.3.1 and 1.4.1
        and all is fine.

        Show
        Stephen Colebourne added a comment - OK, I've had a look at this. I agree with Phil's last comments that the extra checking is generally not required, and that we should treat this as a JDK bug workaround. (Performance matters too) So, I've removed the validate method and retested on JDK1.2.2, 1.3.1 and 1.4.1 and all is fine.
        Hide
        Phil Steitz added a comment -

        Created an attachment (id=7530)
        NumberUtils patch fixing only createBigDecimal

        Show
        Phil Steitz added a comment - Created an attachment (id=7530) NumberUtils patch fixing only createBigDecimal
        Hide
        Phil Steitz added a comment -

        Sorry to keep going back and forth on this, but I think that just patching
        createBigDecimal is sufficient. I am attaching a patch to do this. The new
        (better!) tests all succeed with this patch under 1.4.2, 1.3.1_08 and 1.2_2_016.

        I am OK with either Gary's mods or the more limited patch attached below. Both
        solve the problem.

        Show
        Phil Steitz added a comment - Sorry to keep going back and forth on this, but I think that just patching createBigDecimal is sufficient. I am attaching a patch to do this. The new (better!) tests all succeed with this patch under 1.4.2, 1.3.1_08 and 1.2_2_016. I am OK with either Gary's mods or the more limited patch attached below. Both solve the problem.
        Hide
        ggregory@seagullsw.com added a comment -

        Ah! It looks like we are stepping on each others Bugzilla toes

        Feel free to change the NumberUtils code as you best see fit but do note the
        added bullet-proofing I've added to the unit tests, I believe these changes
        should stay in.

        Show
        ggregory@seagullsw.com added a comment - Ah! It looks like we are stepping on each others Bugzilla toes Feel free to change the NumberUtils code as you best see fit but do note the added bullet-proofing I've added to the unit tests, I believe these changes should stay in.
        Hide
        ggregory@seagullsw.com added a comment -

        Fixes commited as suggested but with the guard clause factored in a method.
        The tests pass on:

        java version "1.3.1_08"
        Java(TM) 2 Runtime Environment, Standard Edition (build 1.3.1_08-b03)
        Java HotSpot(TM) Client VM (build 1.3.1_08-b03, mixed mode)

        java version "1.4.2"
        Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2-b28)
        Java HotSpot(TM) Client VM (build 1.4.2-b28, mixed mode)

        Show
        ggregory@seagullsw.com added a comment - Fixes commited as suggested but with the guard clause factored in a method. The tests pass on: java version "1.3.1_08" Java(TM) 2 Runtime Environment, Standard Edition (build 1.3.1_08-b03) Java HotSpot(TM) Client VM (build 1.3.1_08-b03, mixed mode) java version "1.4.2" Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2-b28) Java HotSpot(TM) Client VM (build 1.4.2-b28, mixed mode)
        Hide
        Phil Steitz added a comment -

        I have added tests for " " locally and run against 1.4.2, 1.3.1_08 and 1.2_2_016
        and found that all tests (including " ") give expected results with current code
        in all cases with the one exception of createBigDecimal on 1.3.1_08. I see this
        as a JDK bug. Therefore I think that my option (4) is overkill, introducing a
        needless function call and string search for each activation to handle an
        isolated JDK bug.

        I would recommend option (5), possibly applied only to createBigDecimal (i.e.
        leave the other methods alone).

        Show
        Phil Steitz added a comment - I have added tests for " " locally and run against 1.4.2, 1.3.1_08 and 1.2_2_016 and found that all tests (including " ") give expected results with current code in all cases with the one exception of createBigDecimal on 1.3.1_08. I see this as a JDK bug. Therefore I think that my option (4) is overkill, introducing a needless function call and string search for each activation to handle an isolated JDK bug. I would recommend option (5), possibly applied only to createBigDecimal (i.e. leave the other methods alone).
        Hide
        Stephen Colebourne added a comment -

        I agree with Phil's comments, and option 4

        Show
        Stephen Colebourne added a comment - I agree with Phil's comments, and option 4
        Hide
        Phil Steitz added a comment -

        I would favor keeping the current contract in place, patching to make things
        work in JDK < 1.4. My understanding of the current contract is

        null |-> null

        (ii) valid representable numeric string |-> value represented

        (iii) invalid string |-> NumberFormatException

        "" and " " fall under (iii). (Need to add tests for " ".)

        One way to achieve this would be to change the guard on the createXxx methods to:

        (4)

        if (str == null)

        { return null; }
        if (StringUtils.isBlank(str)) { throw new NumberFormatException("A blank string is not a valid Xxx."); }

        Or even (following createNumber)

        (5)

        if (str == null) { return null; }

        if (str.length() == 0)

        { throw new NumberFormatException("\"\" is not a valid Xxx."); }

        This should work, but we would have to add tests to verify that " " throws
        NumberformatException in all cases, all JDKs when the conversion is attempted.

        Phil

        Show
        Phil Steitz added a comment - I would favor keeping the current contract in place, patching to make things work in JDK < 1.4. My understanding of the current contract is null |-> null (ii) valid representable numeric string |-> value represented (iii) invalid string |-> NumberFormatException "" and " " fall under (iii). (Need to add tests for " ".) One way to achieve this would be to change the guard on the createXxx methods to: (4) if (str == null) { return null; } if (StringUtils.isBlank(str)) { throw new NumberFormatException("A blank string is not a valid Xxx."); } Or even (following createNumber) (5) if (str == null) { return null; } if (str.length() == 0) { throw new NumberFormatException("\"\" is not a valid Xxx."); } This should work, but we would have to add tests to verify that " " throws NumberformatException in all cases, all JDKs when the conversion is attempted. Phil

          People

          • Assignee:
            Unassigned
            Reporter:
            Gary Gregory
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development