Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-5927

Issue reported while performing Refund & Void (java.lang.ClassCastException: java.lang.String cannot be cast to java.math.BigDecimal)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Trunk, 12.04.05, 13.07.01
    • Fix Version/s: 14.12.01, 12.04.06, 13.07.03, 16.11.01
    • Component/s: None
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015, Community Day 3 - 2015

      Description

      Issue reported while performing Refund & Void (java.lang.ClassCastException: java.lang.String cannot be cast to java.math.BigDecimal)

      Change needs to be done in AIMPaymentServices.java. Please look at the following small patch:-

      //BigDecimal amount = (BigDecimal) request.get("x_Amount");
      String newAmt = request.get("x_Amount").toString();
      BigDecimal amount = new BigDecimal(newAmt);

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Prateek,

        Your fix is in
        trunk r1647522
        R13.07 r1647523
        R12.04 r1647524

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Prateek, Your fix is in trunk r1647522 R13.07 r1647523 R12.04 r1647524
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        This is not the right solution said Scott on dev ML

        Reverted at
        trunk r1647542
        R13.07 r1647543
        R12.04 r1647544

        Show
        jacques.le.roux Jacques Le Roux added a comment - This is not the right solution said Scott on dev ML Reverted at trunk r1647542 R13.07 r1647543 R12.04 r1647544
        Hide
        lektran Scott Gray added a comment -

        Reposting my comments here so that it's easier to follow:
        I know it looks like this makes sense at first glance but sometimes it's useful to look at the history of changes. It should be clear that something is amiss because of the useless try/catch block directly below your change. It points out that perhaps this code previously did something similar to what you're trying to do here.

        So I have a look and we find this commit:
        http://svn.apache.org/viewvc?view=revision&revision=1303329

        Maybe these changes warrant further investigation instead of committers switching it back and forth every few years.

        Because neither Jacques nor Hans have bothered to fully review the issue, you've both missed that the problem is that ccRelease(...) is passing a BigDecimal into the x_Amount map value while every other method is putting a String on that value. The getXAmount method need to be restored properly back to what it was before Hans' commit (i.e. actually use the try/catch block when creating the BigDecimal) and ccRelease(...) needs to convert the BigDecimal to a String before putting it in the map.

        Show
        lektran Scott Gray added a comment - Reposting my comments here so that it's easier to follow: I know it looks like this makes sense at first glance but sometimes it's useful to look at the history of changes. It should be clear that something is amiss because of the useless try/catch block directly below your change. It points out that perhaps this code previously did something similar to what you're trying to do here. So I have a look and we find this commit: http://svn.apache.org/viewvc?view=revision&revision=1303329 Maybe these changes warrant further investigation instead of committers switching it back and forth every few years. Because neither Jacques nor Hans have bothered to fully review the issue, you've both missed that the problem is that ccRelease(...) is passing a BigDecimal into the x_Amount map value while every other method is putting a String on that value. The getXAmount method need to be restored properly back to what it was before Hans' commit (i.e. actually use the try/catch block when creating the BigDecimal) and ccRelease(...) needs to convert the BigDecimal to a String before putting it in the map.
        Hide
        prateek111 Prateek added a comment -

        I do agree to what your're saying, but from java coding side, the following line will always create exception because you're assigning a reference of Map request Object to BigDecimal object, when this line is executing catch block on line 727 and showing printstack trace and getting continue there.
        BigDecimal amount = (BigDecimal) request.get("x_Amount");

        I've debug this issue on my local for Apache OFbiz 13.07.01 and find that when getXAmount(request) method is getting called in processAuthTransResult, processCaptureTransResult etc.., but when I put suggested patch then code is executing fine and there is no catch block is executing. I've seen same problem while executing Void, Refund & Authorization transactions.

        I understand its not big issue, but logs showing error related to above mentioned code for most of transactions. Could you please let me know your decision on this issue? Many Thanks.

        Show
        prateek111 Prateek added a comment - I do agree to what your're saying, but from java coding side, the following line will always create exception because you're assigning a reference of Map request Object to BigDecimal object, when this line is executing catch block on line 727 and showing printstack trace and getting continue there. BigDecimal amount = (BigDecimal) request.get("x_Amount"); I've debug this issue on my local for Apache OFbiz 13.07.01 and find that when getXAmount(request) method is getting called in processAuthTransResult, processCaptureTransResult etc.., but when I put suggested patch then code is executing fine and there is no catch block is executing. I've seen same problem while executing Void, Refund & Authorization transactions. I understand its not big issue, but logs showing error related to above mentioned code for most of transactions. Could you please let me know your decision on this issue? Many Thanks.
        Hide
        lektran Scott Gray added a comment -

        Sorry if my comment was unclear, here is what I am saying should be done:

        Index: applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java
        ===================================================================
        --- applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java	(revision 1647466)
        +++ applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java	(working copy)
        @@ -253,7 +253,7 @@
                     return reply;
                 }
                 Map<String, Object> results = ServiceUtil.returnSuccess();
        -        context.put("x_Amount", context.get("releaseAmount")); // hack for releaseAmount
        +        context.put("x_Amount", ((BigDecimal) context.get("releaseAmount")).toPlainString()); // hack for releaseAmount
                 results.putAll(processReleaseTransResult(context, reply));
                 return results;
             }
        @@ -824,8 +824,8 @@
             private static BigDecimal getXAmount(Map<String, Object> request) {
                 BigDecimal amt = BigDecimal.ZERO;
                 if (request.get("x_Amount") != null) {
        -            BigDecimal amount = (BigDecimal) request.get("x_Amount");
                     try {
        +                BigDecimal amount = new BigDecimal((String) request.get("x_Amount"));
                         amt = amount;
                     } catch (NumberFormatException e) {
                         Debug.logWarning(e, e.getMessage(), module);
        

        The change in the first hunk is important otherwise we're fixing one issue but creating another.

        Show
        lektran Scott Gray added a comment - Sorry if my comment was unclear, here is what I am saying should be done: Index: applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java =================================================================== --- applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java (revision 1647466) +++ applications/accounting/src/org/ofbiz/accounting/thirdparty/authorizedotnet/AIMPaymentServices.java (working copy) @@ -253,7 +253,7 @@ return reply; } Map< String , Object > results = ServiceUtil.returnSuccess(); - context.put( "x_Amount" , context.get( "releaseAmount" )); // hack for releaseAmount + context.put( "x_Amount" , ((BigDecimal) context.get( "releaseAmount" )).toPlainString()); // hack for releaseAmount results.putAll(processReleaseTransResult(context, reply)); return results; } @@ -824,8 +824,8 @@ private static BigDecimal getXAmount(Map< String , Object > request) { BigDecimal amt = BigDecimal.ZERO; if (request.get( "x_Amount" ) != null ) { - BigDecimal amount = (BigDecimal) request.get( "x_Amount" ); try { + BigDecimal amount = new BigDecimal(( String ) request.get( "x_Amount" )); amt = amount; } catch (NumberFormatException e) { Debug.logWarning(e, e.getMessage(), module); The change in the first hunk is important otherwise we're fixing one issue but creating another.
        Hide
        prateek111 Prateek added a comment - - edited

        If this issue is tested successfully from your end then it's great. The following line will be also worked.
        BigDecimal amount = new BigDecimal((String) request.get("x_Amount"));

        My suggested patch also working fine and its not causing any problem. I've done more than hundred's of transactions for Refund, Void, Auth etc., & don't see any issue on logs (no exception's etc..,). This was also a simple change.

        I'll be modify my code for the following line
        context.put("x_Amount", ((BigDecimal) context.get("releaseAmount")).toPlainString());

        Many Thanks!

        Show
        prateek111 Prateek added a comment - - edited If this issue is tested successfully from your end then it's great. The following line will be also worked. BigDecimal amount = new BigDecimal((String) request.get("x_Amount")); My suggested patch also working fine and its not causing any problem. I've done more than hundred's of transactions for Refund, Void, Auth etc., & don't see any issue on logs (no exception's etc..,). This was also a simple change. I'll be modify my code for the following line context.put("x_Amount", ((BigDecimal) context.get("releaseAmount")).toPlainString()); Many Thanks!
        Hide
        lektran Scott Gray added a comment -

        Were you performing auth releases against cancelled orders? Because prior to the additional change (first hunk) I've suggested above, it certainly looks to me like it would result in an error for that situation.

        Show
        lektran Scott Gray added a comment - Were you performing auth releases against cancelled orders? Because prior to the additional change (first hunk) I've suggested above, it certainly looks to me like it would result in an error for that situation.
        Hide
        prateek111 Prateek added a comment -

        No, I've not tested that yet. I've configured Authorize.net as a payment gateway & don't see AIM Authorize.net has "release" API, but I see code is implemented for release functionality. I will be try for this but not sure how to execute it. If you've check already then OK.
        Could you please attach patch in the ticket for all our reference?

        Show
        prateek111 Prateek added a comment - No, I've not tested that yet. I've configured Authorize.net as a payment gateway & don't see AIM Authorize.net has "release" API, but I see code is implemented for release functionality. I will be try for this but not sure how to execute it. If you've check already then OK. Could you please attach patch in the ticket for all our reference?
        Hide
        prateek111 Prateek added a comment -

        Jacques - Is this issue being committed in trunk?
        Scott - Have you provided patch for commit?
        Please do the needful.

        Show
        prateek111 Prateek added a comment - Jacques - Is this issue being committed in trunk? Scott - Have you provided patch for commit? Please do the needful.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Nothing is committed, when I commit something I always say, in a comment of the Jira issue, where (trunk, branches) and at which revision.

        Since Scott is a committer I believe he would have committed something if he was really sure about it. I guess he is still waiting for you answer...

        Show
        jacques.le.roux Jacques Le Roux added a comment - Nothing is committed, when I commit something I always say, in a comment of the Jira issue, where (trunk, branches) and at which revision. Since Scott is a committer I believe he would have committed something if he was really sure about it. I guess he is still waiting for you answer...
        Hide
        prateek111 Prateek added a comment -

        I think solution looks good

        Show
        prateek111 Prateek added a comment - I think solution looks good
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Scott Gray, will you handle it?

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Scott Gray , will you handle it?
        Hide
        lektran Scott Gray added a comment -

        My contribution was the code review, I don't have time right now to do any more. You're more than welcome to commit the patch I provided.

        Show
        lektran Scott Gray added a comment - My contribution was the code review, I don't have time right now to do any more. You're more than welcome to commit the patch I provided.
        Hide
        arunpati Arun Patidar added a comment -

        On the basis of Scott's suggestion, attached is the patch with similar changes.

        Show
        arunpati Arun Patidar added a comment - On the basis of Scott's suggestion, attached is the patch with similar changes.
        Hide
        arunpati Arun Patidar added a comment -

        Committed the changes at rev-1703981 in trunk.

        Show
        arunpati Arun Patidar added a comment - Committed the changes at rev-1703981 in trunk.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Backported in
        R14.12 r1704007
        R13.07 r1704008
        R12.04 r1704009

        Show
        jacques.le.roux Jacques Le Roux added a comment - Backported in R14.12 r1704007 R13.07 r1704008 R12.04 r1704009

          People

          • Assignee:
            arunpati Arun Patidar
            Reporter:
            prateek111 Prateek
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile