groovy
  1. groovy
  2. GROOVY-4841

Null BigDecimals converted to String with add operator

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.10
    • Fix Version/s: 2.0-beta-3
    • Component/s: None
    • Labels:
    • Testcase included:
      yes
    • Number of attachments :
      0

      Description

      Using the add operator on null BigDecimal objects concatenates them as Strings

        @Test
        void testNullBigDecimalAddOperator() {
          BigDecimal a = null
          BigDecimal b = null
          assert null == a+b
        }
      
      Assertion failed: 
      
      assert null == a+b
                  |  |||
                  |  ||null
                  |  |nullnull
                  |  null
                  false
      

      I would expect a NullPointer would be thrown as the divide operator behaves.

        @Test(expected=NullPointerException)
        void testNullBigDecimalDivOperator() {
          BigDecimal a = null
          BigDecimal b = null
          assert null == a/b
        }
      
      java.lang.NullPointerException: Cannot invoke method div() on null object
      

        Activity

        Hide
        blackdrag blackdrag added a comment -

        at the time of invocation of the plus method it does not matter what type you statically declared for the types. We always take the runtime types and since null has none really, we take NullObject instead. So this issue is not about BigDecimal#plus, but about NullObject#plus. This method now basically does: "null"+b leading to the "nullnull" you have observed.
        We can of course do a special case for null+null, but you actually want a NPE for the cases of any of the operands null, or not? That would break code.
        So I kind of tend to close this issue as "Won't Fix".

        Show
        blackdrag blackdrag added a comment - at the time of invocation of the plus method it does not matter what type you statically declared for the types. We always take the runtime types and since null has none really, we take NullObject instead. So this issue is not about BigDecimal#plus, but about NullObject#plus. This method now basically does: "null"+b leading to the "nullnull" you have observed. We can of course do a special case for null+null, but you actually want a NPE for the cases of any of the operands null, or not? That would break code. So I kind of tend to close this issue as "Won't Fix".
        Hide
        Mike Cantrell added a comment -

        I think you're correct. A NPE probably wouldn't be the best option. I still think that the conversion to a string is problematic. Maybe null + null = null would be better?

        Show
        Mike Cantrell added a comment - I think you're correct. A NPE probably wouldn't be the best option. I still think that the conversion to a string is problematic. Maybe null + null = null would be better?
        Hide
        Mike Cantrell added a comment -

        I think a better testcase would be

          @Test
          void testNullBigDecimalAddOperator() {
            BigDecimal a = null
            BigDecimal b = null
            BigDecimal c = a+b
          }
        

        GroovyCastException: Cannot cast object 'nullnull' with class 'java.lang.String' to class 'java.math.BigDecimal'

        Show
        Mike Cantrell added a comment - I think a better testcase would be @Test void testNullBigDecimalAddOperator() { BigDecimal a = null BigDecimal b = null BigDecimal c = a+b } GroovyCastException: Cannot cast object 'nullnull' with class 'java.lang.String' to class 'java.math.BigDecimal'
        Hide
        Dirk Weber added a comment -

        I've added a bugfix and opened a pull-request in github. The solution will extend the plus-method of NullObject: if another NullObject has been passed, a null-reference will be returned. Also added the test of Mike Cantrell to NullObjectTest.

        Show
        Dirk Weber added a comment - I've added a bugfix and opened a pull-request in github. The solution will extend the plus-method of NullObject: if another NullObject has been passed, a null-reference will be returned. Also added the test of Mike Cantrell to NullObjectTest.
        Hide
        blackdrag blackdrag added a comment -

        I have been thinking about this one for a while and I think we should go a different way. Why is there a plus(String) method? It is there because we want null+"X" return something for us useful. But since it is the only plus method it will be called even if that null value is supposed to be a BigDecimal. This is surely not what we want. The alternative solution would be to provide a plus(Object) method, which then for example throws an NullPointerException. Then Mike's example would cause a NPE instead of producing a strange String. It would also cause a+b not to work anymore if both are supposed to be String. But maybe that is not too bad.

        Show
        blackdrag blackdrag added a comment - I have been thinking about this one for a while and I think we should go a different way. Why is there a plus(String) method? It is there because we want null+"X" return something for us useful. But since it is the only plus method it will be called even if that null value is supposed to be a BigDecimal. This is surely not what we want. The alternative solution would be to provide a plus(Object) method, which then for example throws an NullPointerException. Then Mike's example would cause a NPE instead of producing a strange String. It would also cause a+b not to work anymore if both are supposed to be String. But maybe that is not too bad.
        Hide
        blackdrag blackdrag added a comment -

        since the nature of the change I am going to do has a potential to break 1.8 code I will do this fix for 2.0 only.

        Show
        blackdrag blackdrag added a comment - since the nature of the change I am going to do has a potential to break 1.8 code I will do this fix for 2.0 only.
        Hide
        blackdrag blackdrag added a comment -

        I implemented it that way now

        Show
        blackdrag blackdrag added a comment - I implemented it that way now

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Mike Cantrell
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: