Pivot
  1. Pivot
  2. PIVOT-704

JSONSerializer can't deserialize to BigDecimal

    Details

      Description

      JSONSerializer would throw an IllegalArgumentException "Unable to coerce double to BigDecimal" when the object class contains at least one BigDecimal field.
      Exception is traced to BeanAdapter.java , line 916, where coerce(Object, Class) has no else-if for BigDecimal class.

      Re: http://apache-pivot-users.399431.n3.nabble.com/JSONSerializer-can-t-deserialize-to-BigDecimal-td2464302.html

      1. patch_old.patch
        1 kB
        Sandro Martini
      2. patch_new.patch
        2 kB
        Sandro Martini

        Activity

        Hide
        Sandro Martini added a comment -

        Just checked in the fix (and the updated JUnit Test case).
        Be free to reopen the issue in case of errors.

        Sandro

        Show
        Sandro Martini added a comment - Just checked in the fix (and the updated JUnit Test case). Be free to reopen the issue in case of errors. Sandro
        Hide
        Sandro Martini added a comment - - edited

        Greg, thanks for the hint, I'll try to follow your suggestions ... maybe I could as first try commit the last patch and then after see how to make the other change (based on your suggestions).

        This should solve this ticket (one time test case and related test class will be ok, in next days), so the other (patch_old.patch) has been re-attached here but only as a reference. I'll apply this patch in one/max two days (as soon as possible).

        Note that I deleted previous attachments because with some changes they are part of Pivot.
        Than you very much to all for the help.

        Show
        Sandro Martini added a comment - - edited Greg, thanks for the hint, I'll try to follow your suggestions ... maybe I could as first try commit the last patch and then after see how to make the other change (based on your suggestions). This should solve this ticket (one time test case and related test class will be ok, in next days), so the other (patch_old.patch) has been re-attached here but only as a reference. I'll apply this patch in one/max two days (as soon as possible). Note that I deleted previous attachments because with some changes they are part of Pivot. Than you very much to all for the help.
        Hide
        Greg Brown added a comment -

        That will work but will result in an unnecessary conversion to String when the value is already a number. You just need to fix the code within the "value instanceof Number" block.

        Moving the String check up seems fine.

        Show
        Greg Brown added a comment - That will work but will result in an unnecessary conversion to String when the value is already a number. You just need to fix the code within the "value instanceof Number" block. Moving the String check up seems fine.
        Hide
        Sandro Martini added a comment -

        Hi Greg,
        yes, I've just find the right solution (I hope ), in the way you suggested.
        But without calling the if instanceof (otherwise during deserialization is will not be able to find the right parameter type for the setter). Shortly this is an extract of the fix:

        } else if (type == BigInteger.class)

        { coercedValue = new BigInteger(value.toString()); }

        else if (type == BigDecimal.class)

        { coercedValue = new BigDecimal(value.toString()); // TODO: move this as first choice ... // currently this is here just as a reminder }

        else if (type == String.class) {
        coercedValue = value.toString();

        And last, given the (usually high) frequency of String attributes, I'm thinking to move the if (instanceof String) block as first choice, to have a little speedup, ok ?

        Thank you for the moment.
        Today/tomorrow I'll commit this fix and related changes to the test case (and resolve the issue), tell me if something is still not working (and/or reopen the ticket).

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - Hi Greg, yes, I've just find the right solution (I hope ), in the way you suggested. But without calling the if instanceof (otherwise during deserialization is will not be able to find the right parameter type for the setter). Shortly this is an extract of the fix: } else if (type == BigInteger.class) { coercedValue = new BigInteger(value.toString()); } else if (type == BigDecimal.class) { coercedValue = new BigDecimal(value.toString()); // TODO: move this as first choice ... // currently this is here just as a reminder } else if (type == String.class) { coercedValue = value.toString(); And last, given the (usually high) frequency of String attributes, I'm thinking to move the if (instanceof String) block as first choice, to have a little speedup, ok ? Thank you for the moment. Today/tomorrow I'll commit this fix and related changes to the test case (and resolve the issue), tell me if something is still not working (and/or reopen the ticket). Bye, Sandro
        Hide
        Greg Brown added a comment -

        The problem is simple to fix. In the following code, you are generating a Long when the value is of type Number:

        if (value instanceof Number)

        { coercedValue = ((Number)value).longValue(); }

        else

        { coercedValue = new BigInteger(value.toString()); }

        You should be generating a BigInteger. Same for Double.

        Show
        Greg Brown added a comment - The problem is simple to fix. In the following code, you are generating a Long when the value is of type Number: if (value instanceof Number) { coercedValue = ((Number)value).longValue(); } else { coercedValue = new BigInteger(value.toString()); } You should be generating a BigInteger. Same for Double.
        Hide
        Sandro Martini added a comment - - edited

        Yesterday I have committed (under core/test/...) a JUnit Test case to demonstrate the problem (even with my patch).
        I've just seen that in next days I have to fix even the test case.
        As soon as possible I hope to fix the problem, but I'll post here a new patch before committing.

        Show
        Sandro Martini added a comment - - edited Yesterday I have committed (under core/test/...) a JUnit Test case to demonstrate the problem (even with my patch). I've just seen that in next days I have to fix even the test case. As soon as possible I hope to fix the problem, but I'll post here a new patch before committing.
        Hide
        Sandro Martini added a comment -

        Hi Augustus, thank you very much for your tests .

        This evening I'll try them and post a new patch so you and others can try and see ...
        Let's update later.

        Bye

        Show
        Sandro Martini added a comment - Hi Augustus, thank you very much for your tests . This evening I'll try them and post a new patch so you and others can try and see ... Let's update later. Bye
        Hide
        Augustus Thoo added a comment -

        This are my JUnit test case files. Run it against pivot-core-2.0.jar to test for Issue PIVOT-704.

        Show
        Augustus Thoo added a comment - This are my JUnit test case files. Run it against pivot-core-2.0.jar to test for Issue PIVOT-704 .
        Hide
        Greg Brown added a comment -

        > Ok, so I have to make some extensive tests on different cases.

        Well yes, but first you need to fix the bugs.

        Show
        Greg Brown added a comment - > Ok, so I have to make some extensive tests on different cases. Well yes, but first you need to fix the bugs.
        Hide
        Sandro Martini added a comment -

        Hi Greg,
        Ok, so I have to make some extensive tests on different cases.
        In the meantime if someone has some complex json files please attach here.

        Show
        Sandro Martini added a comment - Hi Greg, Ok, so I have to make some extensive tests on different cases. In the meantime if someone has some complex json files please attach here.
        Hide
        Greg Brown added a comment -

        That logic will fail when value is an instance of Number (it will convert to long or double instead of BigInteger and BigDecimal).

        Show
        Greg Brown added a comment - That logic will fail when value is an instance of Number (it will convert to long or double instead of BigInteger and BigDecimal).
        Hide
        Sandro Martini added a comment -

        patch to fix the behavior, handling BigInteger and BigDecimal.
        Note that I guess if it would be better to move the test on type = String as first option, to avoid many if when not needed (assuming the String type it's a common case here).

        Tell me what you think.

        Bye,
        Sandro

        Show
        Sandro Martini added a comment - patch to fix the behavior, handling BigInteger and BigDecimal. Note that I guess if it would be better to move the test on type = String as first option, to avoid many if when not needed (assuming the String type it's a common case here). Tell me what you think. Bye, Sandro

          People

          • Assignee:
            Sandro Martini
            Reporter:
            Augustus Thoo
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development