groovy

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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: