Add checkstyle rule to Include Caught Exceptions as Cause for Subsequently thrown exceptions (Azure#26818)

Author: samvaity

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/BlacklistedWordsCheck.java

@@ -10,11 +10,9 @@
 import com.puppycrawl.tools.checkstyle.utils.CheckUtil;
 import com.puppycrawl.tools.checkstyle.utils.ScopeUtil;
 
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 /**
  * Ensure that code is not using words or abbreviations that are blacklisted by this Checkstyle.

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/JavadocThrowsChecks.java

@@ -17,6 +17,9 @@
 import java.util.HashSet;
 import java.util.Map;
 
+/**
+ * Verifies that all throws in the public API have JavaDocs explaining why and when they are thrown.
+ */
 public class JavadocThrowsChecks extends AbstractCheck {
     static final String MISSING_DESCRIPTION_MESSAGE =
         "@throws tag requires a description explaining when the error is thrown.";

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/NoImplInPublicAPI.java

@@ -17,6 +17,11 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+/**
+ * Verifies that:
+ * 1) no return classes are from the implementation package
+ * 2) no class of implementation package as method's parameters
+ */
 public class NoImplInPublicAPI extends AbstractCheck {
     private static final String ALTERNATIVE_MOVE_TO_PUBLIC_API = "Alternatively, it can be removed from the "
         + "implementation package and made public API, after appropriate API review.";

eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/UseCaughtExceptionCauseCheck.java

@@ -0,0 +1,201 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.tools.checkstyle.checks;
+
+import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
+import com.puppycrawl.tools.checkstyle.api.DetailAST;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+import java.util.LinkedList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * This check ensures that an exception thrown includes the current caught exception cause.
+ * New exception should use the original/cause exception object to provide the full stack trace for the problem.
+ *
+ * // DO
+ * try {
+ *     url = new URL(urlString);
+ * } catch (MalformedURLException ex) {
+ *     throw new RuntimeException(ex);
+ * }
+ *
+ * // DON'T
+ * try {
+ *     url = new URL(urlString);
+ * } catch (MalformedURLException ex) {
+ *     throw new RuntimeException("Invalid URL string was given."); // "ex" is ignored.
+ * }
+ */
+public class UseCaughtExceptionCauseCheck extends AbstractCheck {
+    static final String UNUSED_CAUGHT_EXCEPTION_ERROR = "Caught and rethrown exceptions should include the caught"
+        + " exception as the cause in the rethrown exception. Dropping the causal exception makes it more difficult"
+        + " to troubleshoot issues when they arise. Include the caught exception variable %s as the cause.";
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {TokenTypes.LITERAL_CATCH};
+    }
+
+    @Override
+    public void visitToken(DetailAST catchBlockToken) {
+        // get the caught exception variable name from the catch block
+        final DetailAST catchStatement = catchBlockToken.findFirstToken(TokenTypes.PARAMETER_DEF);
+        final String caughtExceptionVariableName = catchStatement.findFirstToken(TokenTypes.IDENT).getText();
+
+        // get all throw statements from current catch block
+        final List<DetailAST> throwStatements = getThrowStatements(catchBlockToken);
+
+        // get possible exception names to which the original exception might be assigned to
+        final List<String> wrappedExceptions =
+            getWrappedExceptions(catchBlockToken, catchBlockToken, caughtExceptionVariableName);
+
+        throwStatements.forEach(throwToken -> {
+            final List<String> throwParamNames = new LinkedList<>();
+            getThrowParamNames(throwToken, throwParamNames);
+            // include the original exception name to the list to look for in throw statements
+            wrappedExceptions.add(caughtExceptionVariableName);
+
+            // throwParamNames = [ex, p]
+            // exceptionsList = [ex, cause]
+            // Caught exception variable is used if an intersection is between the throw statements param names
+            // used and the actual exception names being thrown.
+            List<String> intersect =
+                wrappedExceptions.stream().filter(throwParamNames::contains).collect(Collectors.toList());
+            if (intersect.size() == 0) {
+                log(throwToken, String.format(UNUSED_CAUGHT_EXCEPTION_ERROR, caughtExceptionVariableName));
+            }
+        });
+    }
+
+    /**
+     * Returns the list of exceptions that wrapped the current exception tokens
+     *
+     * @param currentCatchAST current catch block token
+     * @param detailAST catch block throw parent token
+     * @param caughtExceptionVariableName list containing the exception tokens
+     * @return list of wrapped exception tokens
+     */
+    private List<String> getWrappedExceptions(DetailAST currentCatchAST, DetailAST detailAST,
+                                              String caughtExceptionVariableName) {
+
+        final List<String> wrappedExceptionNames = new LinkedList<>();
+
+        for (DetailAST currentNode : getChildrenNodes(detailAST)) {
+            // Recursively traverse through the children of the parent node to collect references where the
+            // caught exception variable is used.
+            if (currentNode.getType() == TokenTypes.IDENT
+                && currentNode.getText().equals(caughtExceptionVariableName)) {
+                getWrappedExceptionVariable(currentCatchAST, wrappedExceptionNames, currentNode);
+            }
+
+            // add collection in case of last node on this level
+            if (currentNode.getFirstChild() != null) {
+                wrappedExceptionNames.addAll(
+                    getWrappedExceptions(currentCatchAST, currentNode, caughtExceptionVariableName));
+            }
+        }
+        return wrappedExceptionNames;
+    }
+
+    /**
+     * Returns the wrapped exception variable name
+     */
+    private void getWrappedExceptionVariable(DetailAST currentCatchBlock, List<String> wrappedExceptionNames,
+                                             DetailAST currentToken) {
+        DetailAST temp = currentToken;
+
+        // Get the node assigning the caught exception variable, traversing upwards starting from the current node.
+        while (!temp.equals(currentCatchBlock) && temp.getType() != TokenTypes.ASSIGN) {
+            temp = temp.getParent();
+        }
+
+        if (temp.getType() == TokenTypes.ASSIGN) {
+            final DetailAST wrappedException;
+            // Get the variable definition param name to which the caught exception variable is assigned.
+            if (temp.getParent().getType() == TokenTypes.VARIABLE_DEF) {
+                wrappedException = temp.getParent().findFirstToken(TokenTypes.IDENT);
+            } else if (temp.findFirstToken(TokenTypes.DOT) != null) {
+                // Get the variable name if assigned to a 'this' variable
+                wrappedException = temp.findFirstToken(TokenTypes.DOT).findFirstToken(TokenTypes.IDENT);
+            } else {
+                wrappedException = temp.findFirstToken(TokenTypes.IDENT);
+            }
+            if (wrappedException != null) {
+                wrappedExceptionNames.add(wrappedException.getText());
+            }
+        }
+    }
+
+    /**
+     * Returns the parameter names for current throw keyword.
+     *
+     * @param throwParent The parent throw token
+     * @param paramNames The list containing the parameter names
+     * @return list of throw param names
+     */
+    private List<String> getThrowParamNames(DetailAST throwParent, List<String> paramNames) {
+        // get all param names by recursively going through all the throw statements retrieving the token type IDENT
+        // for the text
+        getChildrenNodes(throwParent).forEach(currentNode -> {
+            if (currentNode.getType() == TokenTypes.IDENT) {
+                paramNames.add(currentNode.getText());
+            }
+            if (currentNode.getFirstChild() != null) {
+                getThrowParamNames(currentNode, paramNames);
+            }
+        });
+        return paramNames;
+    }
+
+    /**
+     * Recursive method that searches for all the LITERAL_THROW on the current catch token.
+     *
+     * @param catchBlockToken A start token.
+     * @return list of throw tokens
+     */
+    private List<DetailAST> getThrowStatements(DetailAST catchBlockToken) {
+        final List<DetailAST> throwStatements = new LinkedList<>();
+        getChildrenNodes(catchBlockToken).forEach(currentNode -> {
+            if (TokenTypes.LITERAL_THROW == currentNode.getType()) {
+                throwStatements.add(currentNode);
+            }
+            if (currentNode.getFirstChild() != null) {
+                throwStatements.addAll(getThrowStatements(currentNode));
+            }
+        });
+        return throwStatements;
+    }
+
+    /**
+     * Gets all the children by traversing the tree generated from the current parent node.
+     *
+     * @param token parent node.
+     * @return List of children of the current node.
+     */
+    private static List<DetailAST> getChildrenNodes(DetailAST token) {
+        final List<DetailAST> result = new LinkedList<>();
+
+        DetailAST currNode = token.getFirstChild();
+
+        // Add all the nodes on the current level of the tree and then move to the next level
+        while (currNode != null) {
+            result.add(currNode);
+            currNode = currNode.getNextSibling();
+        }
+
+        return result;
+    }
+}

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml

@@ -109,6 +109,8 @@ the main ServiceBusClientBuilder. -->
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="ServiceBusSessionReceiverAsyncClient.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="ServiceBusSessionReceiverClient.java"/>
 
+  <suppress checks="com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck" files="ServiceBusMessageBatch.java"/>
+
   <!-- Suppress warning for ServiceClients to have Client in its name as Azure Container Registry has Clientlets which do not follow the same convention. -->
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="ContainerRepository.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="ContainerRepositoryAsync.java"/>
@@ -141,6 +143,8 @@ the main ServiceBusClientBuilder. -->
   <!-- Suppress IO exception in the NIO package because they are required by the interface -->
   <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
             files="com.azure.storage.blob.nio/*"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck"
+            files="com.azure.storage.blob.nio.AzureFileSystemProvider.java"/>
 
   <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"
             files="com.azure.storage.blob.batch.BlobBatchOperationResponse.java"
@@ -348,11 +352,12 @@ the main ServiceBusClientBuilder. -->
 
   <!-- Checkstyle suppression for Event Hubs client APIs that use Flux instead of PagedFlux for methods that return a collection -->
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientCheck" files="com.azure.messaging.eventhubs.(EventHubConsumerClient|EventHubConsumerAsyncClient|EventHubProducerClient|EventHubProducerAsyncClient).java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck" files="com.azure.messaging.eventhubs.EventDataBatch.java"/>
 
   <!-- ### begin: Spring related suppression -->
   <!-- TODO: (https://github.com/Azure/azure-sdk-for-java/issues/18291)  -->
   <!-- GoodLoggingCheck: Spring as a framework, will not directly use ClientLogger-->
-  <suppress checks="com.azure.tools.checkstyle.checks.(GoodLoggingCheck|ThrowFromClientLoggerCheck)" files=".*[/\\]com[/\\]azure[/\\]spring[/\\].*"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.(GoodLoggingCheck|ThrowFromClientLoggerCheck|UseCaughtExceptionCauseCheck)" files=".*[/\\]com[/\\]azure[/\\]spring[/\\].*"/>
 
   <!-- ExternalDependencyExposedCheck: Spring will directly use classes from spring dependencies in methods-->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]com[/\\]azure[/\\]spring[/\\].*"/>

eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml

@@ -398,5 +398,9 @@ page at http://checkstyle.sourceforge.net/config.html -->
     <!-- <module name="com.azure.tools.checkstyle.checks.FluentMethodNameCheck">-->
     <!--    <property name="avoidStartWords" value="with,set"/>-->
     <!-- </module>-->
+
+    <!-- CUSTOM CHECKS -->
+    <!-- Ensures caught exceptions are included as exception cause in subsequently thrown exception -->
+    <module name="com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck"/>
   </module>
 </module>

eng/code-quality-reports/src/test/java/com/azure/tools/checkstyle/checks/UseCaughtExceptionTest.java

@@ -0,0 +1,50 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.azure.tools.checkstyle.checks;
+
+import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
+import com.puppycrawl.tools.checkstyle.Checker;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.Collections;
+
+import static com.azure.tools.checkstyle.checks.UseCaughtExceptionCauseCheck.UNUSED_CAUGHT_EXCEPTION_ERROR;
+
+/**
+ * Tests for using the caught exception variable, UseCaughtExceptionTest.
+ */
+public class UseCaughtExceptionTest extends AbstractModuleTestSupport {
+    private Checker checker;
+
+    @Before
+    public void prepare() throws Exception {
+        checker = createChecker(createModuleConfig(UseCaughtExceptionCauseCheck.class));
+    }
+
+    @After
+    public void cleanup() {
+        checker.destroy();
+    }
+
+    @Override
+    protected String getPackageLocation() {
+        return "com/azure/tools/checkstyle/checks/UseCaughtExceptionCheck";
+    }
+
+
+    @Test
+    public void unusedCaughtExceptionTestData() throws Exception {
+        String[] expected = {
+            expectedErrorMessage(17, 13, String.format(UNUSED_CAUGHT_EXCEPTION_ERROR, "e"))
+        };
+        verify(checker, getPath("UseCaughtExceptionTestData.java"), expected);
+    }
+
+    private String expectedErrorMessage(int line, int column, String errorMessage) {
+        return String.format("%d:%d: %s", line, column, errorMessage);
+    }
+}