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

Fix Default or Empty Catch block in Java and Groovy files

    Details

    • Type: Bug
    • Status: Reopened
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: Upcoming Release
    • Component/s: ALL COMPONENTS
    • Labels:
      None

      Description

      In many Java and Groovy files we have auto generated catch blocks or empty catch blocks.
      To avoid such exception swallowing this should be improved to at least log the error and also return error in case of service.
      As of now I found following classes with such patterns -

      InventoryServices.java
      FreeMarkerWorker.java
      QRCodeEvents.java
      QRCodeServices.java

        Activity

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

        Thanks Harsh for this Jira,

        I don't believe it's a task but a bug in case of swallowed exceptions. It's also major IMO.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Harsh for this Jira, I don't believe it's a task but a bug in case of swallowed exceptions. It's also major IMO.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Fixed some in revisions: 1787911, 1787910, 1787909, 1787908, 1787906, 1789043

        I'll not backport any

        Show
        jacques.le.roux Jacques Le Roux added a comment - Fixed some in revisions: 1787911, 1787910, 1787909, 1787908, 1787906, 1789043 I'll not backport any
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Another at revision: 1789047. I also fixed a bug I put in with r1789043 at 1789046

        Show
        jacques.le.roux Jacques Le Roux added a comment - Another at revision: 1789047. I also fixed a bug I put in with r1789043 at 1789046
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Seems that ebaystore has a lot of swallowed exceptions...

        Show
        jacques.le.roux Jacques Le Roux added a comment - Seems that ebaystore has a lot of swallowed exceptions...
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        After my last commits this morning, I checked if there were still swallowed exceptions, using the regexp below (note the white space ahead)

         +\} catch \(.*\) \{\R\R +\}\R
        

        and some variations, like

         +\} catch \(.*\) \{\R +\R +\}\R
         +\} catch \(.*\) \{\R\R +\}\R
        

        And found only 3 PatternSyntaxException which are from pre Apache era and seems OK because it's always the same pattern

                                try {
                                    String queryStringEncoded = queryString.replaceAll("&", "%26");
                                    context.put("queryStringEncoded", queryStringEncoded);
                                } catch (PatternSyntaxException e) {
        
        

        and obviously a PatternSyntaxException should not occur there. So they simply miss a comment to document the fact even it it seems obvious. I put it at revision: 1789163
        I note though that the repeated pattern is a smell for refactoring. I created OFBIZ-9287 for that.

        I also begin to have a look into the ebaystore component which is a can of worms when you look at its exception handling. But, according to my research, there are no longer swallowed exceptions.

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited After my last commits this morning, I checked if there were still swallowed exceptions, using the regexp below (note the white space ahead) +\} catch \(.*\) \{\R\R +\}\R and some variations, like +\} catch \(.*\) \{\R +\R +\}\R +\} catch \(.*\) \{\R\R +\}\R And found only 3 PatternSyntaxException which are from pre Apache era and seems OK because it's always the same pattern try { String queryStringEncoded = queryString.replaceAll( "&" , "%26" ); context.put( "queryStringEncoded" , queryStringEncoded); } catch (PatternSyntaxException e) { and obviously a PatternSyntaxException should not occur there. So they simply miss a comment to document the fact even it it seems obvious. I put it at revision: 1789163 I note though that the repeated pattern is a smell for refactoring. I created OFBIZ-9287 for that. I also begin to have a look into the ebaystore component which is a can of worms when you look at its exception handling. But, according to my research, there are no longer swallowed exceptions.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        At r1789710 I fixed a swallowed numberFormatException in GetContentLookupList.groovy.

        I checked there are no other swallowed exceptions in Groovy files.

        Show
        jacques.le.roux Jacques Le Roux added a comment - At r1789710 I fixed a swallowed numberFormatException in GetContentLookupList.groovy. I checked there are no other swallowed exceptions in Groovy files.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        After Taher's and Michael's review in dev ML, I fixed a small issue from r1789710 in 1789736, it was fortunately a functional change.

        Show
        jacques.le.roux Jacques Le Roux added a comment - After Taher's and Michael's review in dev ML, I fixed a small issue from r1789710 in 1789736, it was fortunately a functional change.
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        We still have 88 swallowed exceptions in Java and Groovy files look for

        catch (*) {}

        I'll check and fix when necessary.

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited We still have 88 swallowed exceptions in Java and Groovy files look for catch (*) {} I'll check and fix when necessary.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I'll sometimes create subtasks or new Jira issues to differentiate cases that need to be discussed. It would be good for instance for a type of exception and a type of file (service, event, helper/handler/test/etc.) to use and adopt a same type of exception handling.

        Show
        jacques.le.roux Jacques Le Roux added a comment - I'll sometimes create subtasks or new Jira issues to differentiate cases that need to be discussed. It would be good for instance for a type of exception and a type of file (service, event, helper/handler/test/etc.) to use and adopt a same type of exception handling.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        At r1807162 I fixed 2 swallowed numberFormatException in UploadContentAndImage.java and StatBinsHistory.groovy

        Show
        jacques.le.roux Jacques Le Roux added a comment - At r1807162 I fixed 2 swallowed numberFormatException in UploadContentAndImage.java and StatBinsHistory.groovy
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        At r1807240 I fixed a swallowed GenericEntityException in OrderReadHelper.java

        Show
        jacques.le.roux Jacques Le Roux added a comment - At r1807240 I fixed a swallowed GenericEntityException in OrderReadHelper.java
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        The OFBIZ-8341- OrderReadHelper.patch reverts r1807240 and rather throw exceptions
        Thanks to Scott's suggestion on dev ML, if we agree another type of replacement for swallowed exceptions

        Show
        jacques.le.roux Jacques Le Roux added a comment - The OFBIZ-8341 - OrderReadHelper.patch reverts r1807240 and rather throw exceptions Thanks to Scott's suggestion on dev ML , if we agree another type of replacement for swallowed exceptions
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        BTW there is a large usage of GeneralException in OrderServices.java. And the usage is not consistent. This is the kind of things we should thought about if we want to refactor a monster like that (6411 lines).

        Show
        jacques.le.roux Jacques Le Roux added a comment - BTW there is a large usage of GeneralException in OrderServices.java. And the usage is not consistent. This is the kind of things we should thought about if we want to refactor a monster like that (6411 lines).
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        Also it's worth to note here that arguably the same kind of exception handling that I did in r1807240 is done in getShippableTotal() (even worse since in a loop):

            public BigDecimal getShippableTotal(String shipGroupSeqId) {
                BigDecimal shippableTotal = ZERO;
                List<GenericValue> validItems = getValidOrderItems(shipGroupSeqId);
                if (validItems != null) {
                    for (GenericValue item : validItems) {
                        GenericValue product = null;
                        try {
                            product = item.getRelatedOne("Product", false);
                        } catch (GenericEntityException e) {
                            Debug.logError(e, "Problem getting Product from OrderItem; returning 0", module);
                            return ZERO;
                        }
                        if (product != null) {
                            if (ProductWorker.shippingApplies(product)) {
                                shippableTotal = shippableTotal.add(OrderReadHelper.getOrderItemSubTotal(item, getAdjustments(), false, true)).setScale(scale, rounding);
                            }
                        }
                    }
                }
                return shippableTotal.setScale(scale, rounding);
            }
        

        Theoretically it's also better here to throw an exception rather than hiding it by returning an unknown value (here ZERO, we already spoke about this "ZERO" BTW)

        I say theoretically because as Scott stated in his message on dev ML

        Also worth noting the reality in this case, if your system is throwing EntityExceptions from a simple db read then whatever you're trying accomplish is already going to error out somewhere along the chain before or after this method is called.

        But this should not prevent us to write the best possible code, even if it's theoretically in case of EntityException (something worse is happening underneath anyway)

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited Also it's worth to note here that arguably the same kind of exception handling that I did in r1807240 is done in getShippableTotal() (even worse since in a loop): public BigDecimal getShippableTotal( String shipGroupSeqId) { BigDecimal shippableTotal = ZERO; List<GenericValue> validItems = getValidOrderItems(shipGroupSeqId); if (validItems != null ) { for (GenericValue item : validItems) { GenericValue product = null ; try { product = item.getRelatedOne( "Product" , false ); } catch (GenericEntityException e) { Debug.logError(e, "Problem getting Product from OrderItem; returning 0" , module); return ZERO; } if (product != null ) { if (ProductWorker.shippingApplies(product)) { shippableTotal = shippableTotal.add(OrderReadHelper.getOrderItemSubTotal(item, getAdjustments(), false , true )).setScale(scale, rounding); } } } } return shippableTotal.setScale(scale, rounding); } Theoretically it's also better here to throw an exception rather than hiding it by returning an unknown value (here ZERO, we already spoke about this "ZERO" BTW) I say theoretically because as Scott stated in his message on dev ML Also worth noting the reality in this case, if your system is throwing EntityExceptions from a simple db read then whatever you're trying accomplish is already going to error out somewhere along the chain before or after this method is called. But this should not prevent us to write the best possible code, even if it's theoretically in case of EntityException (something worse is happening underneath anyway)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Fixed some ClassCastException at revision: 1815797

        Show
        jacques.le.roux Jacques Le Roux added a comment - Fixed some ClassCastException at revision: 1815797
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        29/Jun/17 13:13 I wrote

        We still have 88 swallowed exceptions in Java and Groovy

        Good news: remain only 58... but still 58...

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited 29/Jun/17 13:13 I wrote We still have 88 swallowed exceptions in Java and Groovy Good news: remain only 58... but still 58...
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Er no, 61 including Groovy files :/

        Show
        jacques.le.roux Jacques Le Roux added a comment - Er no, 61 including Groovy files :/

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            harshvijay Harsh Vijaywargiya
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development