Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: accounting
    • Labels:
      None

      Description

      SagePayPaymentServices.java:133, NP_NULL_ON_SOME_PATH

      • NP: Possible null pointer dereference of processAmount in org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayPaymentServices.buildCustomerBillingInfo(Map)
        There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed. Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs.

      SagePayPaymentServices.java:159, NP_LOAD_OF_KNOWN_NULL_VALUE

      • NP: Load of known null value in org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayPaymentServices.ccAuth(DispatchContext, Map)
        The variable referenced at this point is known to be null due to an earlier check against null. Although this is valid, it might be a mistake (perhaps you intended to refer to a different variable, or perhaps the earlier check to see if the variable is null should have been a check to see if it was non-null).

      SagePayServices.java:64, WMI_WRONG_MAP_ITERATOR

      • WMI: org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayServices.buildSagePayProperties(Map, Delegator) makes inefficient use of keySet iterator instead of entrySet iterator
        This method accesses the value of a Map entry, using a key that was retrieved from a keySet iterator. It is more efficient to use an iterator on the entrySet of the map, to avoid the Map.get(key) lookup.

      SagePayUtil.java:124, NP_NULL_ON_SOME_PATH

      • NP: Possible null pointer dereference of hostUrl in org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayUtil.getHost(Map)
        There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed. Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs.

      SagePayUtil.java:141, DM_DEFAULT_ENCODING

      • Dm: Found reliance on default encoding in org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayUtil.getResponseData(HttpResponse): new java.io.InputStreamReader(InputStream)
        Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behaviour to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

      SagePayUtil.java:141, OS_OPEN_STREAM

      • OS: org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayUtil.getResponseData(HttpResponse) may fail to close stream
        The method creates an IO stream object, does not assign it to any fields, pass it to other methods that might close it, or return it, and does not appear to close the stream on all paths out of the method. This may result in a file descriptor leak. It is generally a good idea to use a finally block to ensure that streams are closed.

      SagePayUtil.java:164, WMI_WRONG_MAP_ITERATOR

      • WMI: org.apache.ofbiz.accounting.thirdparty.sagepay.SagePayUtil.getHttpPost(String, Map) makes inefficient use of keySet iterator instead of entrySet iterator
        This method accesses the value of a Map entry, using a key that was retrieved from a keySet iterator. It is more efficient to use an iterator on the entrySet of the map, to avoid the Map.get(key) lookup.

      While doing the changes some potential problems with the current behavior of the SagePayPaymentAuthentication occurred which I will describe in the following comment.

        Activity

        Hide
        Karsten Tymann Karsten Tymann added a comment - - edited

        The patch fixes the problems given by the FindBugs tool.

        However while refactoring the code a potential NPE was found that might be problematic for the SagePayPayment Transaction. Maybe someone with more knowledge about the SagePay Transaction can help and provide insight for the patch and whether the implementation is correct.

        The concerning classes/files are the following:

        • SagePayServices.java
        • services_sagepay.xml

        SagePayServices

        buildSagePayProperties(Map<String, Object> context, Delegator delegator)
        

        The method constructs the sagepay-configuration as a map via the "PaymentGatewaySagePay" entity.
        While iterating through the map the value is not checked for being initialized and therefore a NullPointerException can occur when the .toString() operation is called.
        I fixed this behavior by checking the Object being null.

        This lead to the following problem which I need help on.

        paymentAuthentication(DispatchContext ctx, Map<String, Object> context)
        

        The method states with an inline-comment that the following parameters are required:

        //start - required parameters
        String vpsProtocol = props.get("protocolVersion");
        String vendor = props.get("vendor");
        String txType = props.get("authenticationTransType");
        ...
        resultMap.put("transactionType", txType);
        

        And checking back on the Service-Method inside services_sagepay.xml

        SagePayPaymentAuthentication
        

        one can see that especially the transactionType is expected as an OUT parameter.
        Because of this I figured that if one of the 3 parameters stated as required is null, an error should be returned and the service should stop.

        In my opinion the thought process and the patch is correct but someone with a better knowledge of the SagePayTransaction should check the patch for being correct.

        Show
        Karsten Tymann Karsten Tymann added a comment - - edited The patch fixes the problems given by the FindBugs tool. However while refactoring the code a potential NPE was found that might be problematic for the SagePayPayment Transaction. Maybe someone with more knowledge about the SagePay Transaction can help and provide insight for the patch and whether the implementation is correct. The concerning classes/files are the following: SagePayServices.java services_sagepay.xml SagePayServices buildSagePayProperties(Map< String , Object > context, Delegator delegator) The method constructs the sagepay-configuration as a map via the "PaymentGatewaySagePay" entity. While iterating through the map the value is not checked for being initialized and therefore a NullPointerException can occur when the .toString() operation is called. I fixed this behavior by checking the Object being null. This lead to the following problem which I need help on. paymentAuthentication(DispatchContext ctx, Map< String , Object > context) The method states with an inline-comment that the following parameters are required: //start - required parameters String vpsProtocol = props.get( "protocolVersion" ); String vendor = props.get( "vendor" ); String txType = props.get( "authenticationTransType" ); ... resultMap.put( "transactionType" , txType); And checking back on the Service-Method inside services_sagepay.xml SagePayPaymentAuthentication one can see that especially the transactionType is expected as an OUT parameter. Because of this I figured that if one of the 3 parameters stated as required is null, an error should be returned and the service should stop. In my opinion the thought process and the patch is correct but someone with a better knowledge of the SagePayTransaction should check the patch for being correct.
        Hide
        mbrohl Michael Brohl added a comment -

        Fellow developers,

        is there someone who can help out and check if the patch is correct from a business/process perspective?
        Thank you!

        Show
        mbrohl Michael Brohl added a comment - Fellow developers, is there someone who can help out and check if the patch is correct from a business/process perspective? Thank you!
        Hide
        mbrohl Michael Brohl added a comment -

        Thanks, Karsten,

        since there were no obligations against your patch and no responses to your questions, I assume the solution is correct.

        Your patch ist in trunk r1812895.

        Show
        mbrohl Michael Brohl added a comment - Thanks, Karsten, since there were no obligations against your patch and no responses to your questions, I assume the solution is correct. Your patch ist in trunk r1812895.

          People

          • Assignee:
            mbrohl Michael Brohl
            Reporter:
            Karsten Tymann Karsten Tymann
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development