1

Let me start by stating that I have read this post which does not address my particular issue.

According to SonarQube rule (java:S2147) - Catches should be combined.

I tested my code without my exceptions combined and with my exceptions combined but I am get a behaviour that I did not expect.

Here's my test code:

  • BusinessFault
  • SystemException
  • ExceptionUtility
  • ExceptionTester (incl. main method)
public class BusinessFault extends Exception {
    public BusinessFault(String message) {
        super(message);
    }
}

public class SystemException extends Exception {
    public SystemException(String message) {
        super(message);
    }
}

public class ExceptionUtility {
    private ExceptionUtility() {
        // private no args constructor
    }

    public static void logException(Logger logger, BusinessFault bf) {
         logger.log(Level.SEVERE, " BUSINESS FAULT THROWN {0}", bf.getMessage());
    }

    public static void logException(Logger logger, SystemException se) {
        logger.log(Level.SEVERE, " SYSTEM FAULT THROWN {0}", se.getMessage());
    }

    public static void logException(Logger logger, Exception e) {
        logger.log(Level.SEVERE, " GENERAL EXCEPTION {0}", e.getMessage());
    }
}

public class ExceptionTester {

    private static final Logger logger = Logger.getLogger(ExceptionTester.class.getName());

    public static void main(String[] args) {

        ExceptionTester et = new ExceptionTester();

        try {
            et.throwMyCustomException("SE");
//          et.throwMyCustomException("BF");
        } catch (BusinessFault e) {
            logger.log(Level.SEVERE, " ##### (ExceptionTester) exception caught - msg: {0} ", e.getMessage());
            ExceptionUtility.logException(logger, e);
        } catch (SystemException e) {
            logger.log(Level.SEVERE, " ##### (ExceptionTester) exception caught - msg: {0} ", e.getMessage());
            ExceptionUtility.logException(logger, e);
        } catch (Exception e) {
            ExceptionUtility.logException(logger, e);
        }
    }

    public void throwMyCustomException(String exceptionType) throws SystemException, BusinessFault {

        System.out.println(" ::::::::::: throwMyCustomException() : " + exceptionType);

        if (exceptionType.equals("SE")) {
            throw new SystemException("  > **** SYSTEM EXCEPTION thrown... **** ");
        } else if (exceptionType.equals("BF")) {
            throw new BusinessFault("  > **** BUSINESS FAULT thrown... **** ");
        }
    }
}

When I throw my SystemException without BusinessFault and SystemException combined in the same catch statement, I get the behaviour that I expect which is the following method being called.

// in ExceptionUtility

public static void logException(Logger logger, SystemException se) {
    logger.log(Level.SEVERE, " SYSTEM FAULT THROWN {0}", se.getMessage());
}

Console output:

 ::::::::::: throwMyCustomException() : SE
May 12, 2020 19:03:56 AM com.exceptions.ExceptionTester main
SEVERE:  ##### (ExceptionTester) exception caught - msg:   > **** SYSTEM EXCEPTION thrown... ****  
May 12, 2020 19:03:56 AM com.exceptions.ExceptionUtility logException
SEVERE:  SYSTEM FAULT THROWN   > **** SYSTEM EXCEPTION thrown... ****

But when I combine the BusinessFault and SystemException in the same catch statement as shown below and throw the SystemException, I get a different behaviour.

// in ExceptionTester

try {
    et.throwMyCustomException("SE");
    //et.throwMyCustomException("BF");
} catch (BusinessFault | SystemException e) {  // <<<<<<<< COMBINED EXCEPTIONS
    logger.log(Level.SEVERE, " ##### (ExceptionTester) exception caught - msg: {0} ", e.getMessage());
    ExceptionUtility.logException(logger, e);
} catch (Exception e) {
    ExceptionUtility.logException(logger, e);
}

A different method in my ExceptionUtility is called instead. 

// in ExceptionUtility

public static void logException(Logger logger, Exception e) {
    logger.log(Level.SEVERE, " GENERAL EXCEPTION {0}", e.getMessage());
}

Console output: 

 ::::::::::: throwMyCustomException() : SE
May 12, 2020 19:10:30 AM com.exceptions.ExceptionTester main
SEVERE:  ##### (ExceptionTester) exception caught - msg:   > **** SYSTEM EXCEPTION thrown... ****  
May 12, 2020 19:10:30 AM com.exceptions.ExceptionUtility logException
SEVERE:  GENERAL EXCEPTION   > **** SYSTEM EXCEPTION thrown... ****

I can see that it's still the SystemException message being passed. But that's not really the method that I need to be called. For simplicity reasons, my logExceptions only log info. But my need is to handle SystemException and BusinessFault differently from a generic Exception.

Can anybody please shed some slight on why this behaviour occurs? More specifically, why the logException method for handling SystemException in ExceptionUtility class is not called in both cases?

Michael
  • 41,989
  • 11
  • 82
  • 128
BustedSanta
  • 1,368
  • 7
  • 28
  • 55

1 Answers1

3

The problem is caused by your use of operator overloading.

SonarQube sees the body of the two catch statements as identical. And, naively, they do appear to be. The text is identical. The problem is that the context of the surrounding catch block determines which signature will be chosen, and how that text is interpreted. When you combine the catch blocks, the generic Exception overload is selected instead of one of the more specific versions. This changes your behaviour in an undesirable way.

It appears SonarQube is not sophisticated enough to make the distinction.

I see 2 solutions:

  1. Label the SonarQube error as a false positive. This would be perfectly valid, since SonarQube is wrong in this case. However, future readers of your code may make the same mistake as SonarQube and may attempt to conflate the blocks. You could add a test case to make sure such a change would not go unnoticed.
  2. Change the method names so that you are no longer relying on operator overloading, so that SonarQube can tell that the catch blocks are different.

For example:

public static void logBusinessException(Logger logger, BusinessFault bf) {
     logger.log(Level.SEVERE, " BUSINESS FAULT THROWN {0}", bf.getMessage());
}

public static void logSystemException(Logger logger, SystemException se) {
    logger.log(Level.SEVERE, " SYSTEM FAULT THROWN {0}", se.getMessage());
}

public static void logException(Logger logger, Exception e) {
    logger.log(Level.SEVERE, " GENERAL EXCEPTION {0}", e.getMessage());
}
Michael
  • 41,989
  • 11
  • 82
  • 128