Client core: new logging exceptions checks

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

@@ -0,0 +1,70 @@
+// 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.FullIdent;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Checks that the exception is created and logged is also thrown.
+ */
+public class ExceptionCreatedButNotThrownCheck extends AbstractCheck {
+    static final String ERROR_MESSAGE = "An exception is created and logged, but not thrown. Ensure the exception is either thrown or not created at all. "
+        + "See https://github.com/Azure/azure-sdk-for-java/wiki/Client-core:-logging-exceptions-best-practices for more details.";
+    private static final List<String> THROWABLE_AT_LOGGING_METHODS = Arrays.asList(
+        ".throwableAtError",
+        ".throwableAtWarning");
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.METHOD_CALL
+        };
+    }
+
+    @Override
+    public void visitToken(DetailAST token) {
+        String name = FullIdent.createFullIdentBelow(token).getText();
+        if (THROWABLE_AT_LOGGING_METHODS.stream().anyMatch(name::endsWith) && !isInsideThrow(token)) {
+            DetailAST logMethodCall = token.getParent().getParent();
+            if (logMethodCall == null || logMethodCall.getType() != TokenTypes.METHOD_CALL) {
+                return;
+            }
+
+            String nextName = FullIdent.createFullIdentBelow(logMethodCall).getText();
+            if (nextName.endsWith(".log")) {
+                log(token, ERROR_MESSAGE);
+            }
+        }
+    }
+
+    private boolean isInsideThrow(DetailAST methodCallAst) {
+        DetailAST parent = methodCallAst.getParent();
+
+        // Walk up to skip DOT or EXPR
+        while (parent != null && (parent.getType() == TokenTypes.DOT
+            || parent.getType() == TokenTypes.EXPR
+            || parent.getType() == TokenTypes.TYPECAST
+            || parent.getType() == TokenTypes.METHOD_CALL)) {
+            parent = parent.getParent();
+        }
+
+        return parent != null && parent.getType() == TokenTypes.LITERAL_THROW;
+    }
+}

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

@@ -0,0 +1,74 @@
+// 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.FullIdent;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Check that verifies that exceptions are logged when they are created.
+ * The check is skipped if:
+ * - the exception is created in a generated method or service interface
+ * - the exception is a NullPointerException, IllegalArgumentException or UnsupportedOperationException that are used for immediate input validation
+ */
+public class RawExceptionThrowCheck extends AbstractCheck {
+    static final String ERROR_MESSAGE = "Directly throwing a new exception is disallowed. Use the \"io.clientcore.core.instrumentation.logging.ClientLogger\" API instead, "
+        + "such as \"logger.throwableAtError()\" or \"logger.throwableAtWarning()\"."
+        + "See https://github.com/Azure/azure-sdk-for-java/wiki/Client-core:-logging-exceptions-best-practices for more details.";
+
+    private static final List<String> IGNORED_EXCEPTIONS = Arrays.asList("NullPointerException",
+        "IllegalArgumentException",
+        "IllegalStateException",
+        "UnsupportedOperationException");
+
+    private static final String CORE_EXCEPTION_FACTORY_NAME = "CoreException.from";
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.LITERAL_THROW
+        };
+    }
+
+    @Override
+    public void visitToken(DetailAST token) {
+        DetailAST expr = token.findFirstToken(TokenTypes.EXPR);
+        if (expr == null) {
+            return;
+        }
+
+        DetailAST newToken = expr.findFirstToken(TokenTypes.LITERAL_NEW);
+        if (newToken != null) {
+            String name = FullIdent.createFullIdentBelow(newToken).getText();
+            if (IGNORED_EXCEPTIONS.stream().noneMatch(name::endsWith)) {
+                log(newToken, ERROR_MESSAGE);
+            }
+
+            return;
+        }
+
+        DetailAST methodCallToken = expr.findFirstToken(TokenTypes.METHOD_CALL);
+        if (methodCallToken != null) {
+            if (CORE_EXCEPTION_FACTORY_NAME.equals(FullIdent.createFullIdentBelow(methodCallToken).getText()))
+            {
+                log(methodCallToken, ERROR_MESSAGE);
+            }
+        }
+    }
+}

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

@@ -77,9 +77,9 @@ public class ServiceClientCheck extends AbstractCheck {
         "''%s'' service client with ''%s'', should always use return type ''%s'' if method name ends with ''%s'' or "
             + "should always named method name ends with ''%s'' if the return type is ''%s''.";
     private static final String ASYNC_CONTEXT_ERROR =
-        "Asynchronous method with annotation @ServiceMethod must not has ''%s'' as a method parameter.";
+        "Asynchronous method with annotation @ServiceMethod must not have ''%s'' as a method parameter.";
     private static final String SYNC_CONTEXT_ERROR =
-        "Synchronous method with annotation @ServiceMethod must has ''%s'' or ''%s'' as a method parameter.";
+        "Synchronous method with annotation @ServiceMethod must have ''%s'' or ''%s'' as a method parameter.";
 
     // Add all imported classes into a map, key is the name of class and value is the full package path of class.
     private final Map<String, String> simpleClassNameToQualifiedNameMap = new HashMap<>();

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

@@ -0,0 +1,85 @@
+// 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.FullIdent;
+import com.puppycrawl.tools.checkstyle.api.TokenTypes;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Checks that the message provided to "logger.throwableAt*().log" is static (not created using String.format).
+ */
+public class StringFormattedExceptionMessageCheck extends AbstractCheck {
+    static final String ERROR_MESSAGE = "Short message passed to \"logger.throwableAt*().log\" should be static. "
+        + "Provide dynamic components using the \"addKeyValue(key, value)\" method instead. "
+        + "See https://github.com/Azure/azure-sdk-for-java/wiki/Client-core:-logging-exceptions-best-practices for more details.";
+    private static final List<String> THROWABLE_AT_LOGGING_METHODS = Arrays.asList(
+        ".throwableAtError",
+        ".throwableAtWarning");
+
+    private static final String LOG_METHOD_NAME = ".log";
+    private static final String STRING_FORMAT_METHOD_NAME = "String.format";
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.METHOD_CALL
+        };
+    }
+
+    @Override
+    public void visitToken(DetailAST token) {
+        String name = FullIdent.createFullIdentBelow(token).getText();
+        if (THROWABLE_AT_LOGGING_METHODS.stream().anyMatch(name::endsWith)) {
+            DetailAST logMethodCall = token.getParent().getParent();
+            if (logMethodCall == null || logMethodCall.getType() != TokenTypes.METHOD_CALL) {
+                return;
+            }
+
+            String nextName = FullIdent.createFullIdentBelow(logMethodCall).getText();
+            if (nextName.endsWith(LOG_METHOD_NAME)) {
+                DetailAST elist = logMethodCall.findFirstToken(TokenTypes.ELIST);
+                if (elist == null) {
+                    return;
+                }
+
+                // first param of `log()` method
+                DetailAST logExpr = elist.findFirstToken(TokenTypes.EXPR);
+                if (logExpr == null) {
+                    return;
+                }
+
+                DetailAST firstParam = logExpr.getFirstChild();
+                // flag String.format
+                if (firstParam.getType() == TokenTypes.METHOD_CALL) {
+                    String logFirstArgMethod = FullIdent.createFullIdentBelow(logExpr.getFirstChild()).getText();
+                    if (logFirstArgMethod.endsWith(STRING_FORMAT_METHOD_NAME)) {
+                        log(logExpr, ERROR_MESSAGE);
+                    }
+                } else if (firstParam.getType() == TokenTypes.PLUS) {
+                    // flag if dynamic, i.e. a combination of string literal and ident
+                    DetailAST firstIdent = logExpr.getFirstChild().findFirstToken(TokenTypes.IDENT);
+                    DetailAST firstLiteral = logExpr.getFirstChild().findFirstToken(TokenTypes.STRING_LITERAL);
+                    if (firstIdent != null && firstLiteral != null) {
+                        log(firstParam, ERROR_MESSAGE);
+                    }
+                }
+            }
+        }
+    }
+}

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

@@ -279,11 +279,11 @@ page at http://checkstyle.sourceforge.net/config.html -->
     <module name="ArrayTypeStyle"/>
 
     <!--Checks for package naming and name length limit. Package names have to starting with either-->
-    <!--'com.microsoft.azure', 'com.azure', or 'io.clientcore' and then the extension names start with-->
+    <!--'com.azure.v2', or 'io.clientcore' and then the extension names start with-->
     <!--characters 'a' to 'z' and only contains 'a' to 'z' or '0' to '9' with no more than 32 characters-->
     <!--and the package names should be no more than 80 characters.-->
     <module name="PackageName">
-      <property name="format" value="^(?=.{9,80}$)((io.clientcore)(\.[a-z][a-z0-9]{1,31})*)+$"/>
+      <property name="format" value="^(?=.{9,80}$)((io.clientcore|com.azure.v2)(\.[a-z][a-z0-9]{1,31})*)+$"/>
     </module>
 
     <!-- Javadoc checks                                   -->
@@ -357,8 +357,16 @@ page at http://checkstyle.sourceforge.net/config.html -->
     <module name="com.azure.tools.checkstyle.checks.JavadocFormatting"/>
 
     <!-- CUSTOM CHECKS -->
-    <!-- Must use 'logger.logAndThrow' but not directly calling 'throw exception' -->
-    <module name="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"/>
+    <!-- Don't format exception messages -->
+    <module name="com.azure.tools.checkstyle.checks.StringFormattedExceptionMessageCheck"/>
+
+    <!-- CUSTOM CHECKS -->
+    <!-- Log exceptions at creation time -->
+    <module name="com.azure.tools.checkstyle.checks.RawExceptionThrowCheck"/>
+
+    <!-- CUSTOM CHECKS -->
+    <!-- Throw created and logged exceptions -->
+    <module name="com.azure.tools.checkstyle.checks.ExceptionCreatedButNotThrownCheck"/>
 
     <!-- CUSTOM CHECKS -->
     <!-- Any class that implements the HttpPipelinePolicy interface should:

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

@@ -0,0 +1,41 @@
+// 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.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class ExceptionCreatedButNotThrownCheckTest extends AbstractModuleTestSupport {
+    private Checker checker;
+
+    @BeforeEach
+    public void prepare() throws Exception {
+        checker = createChecker(createModuleConfig(ExceptionCreatedButNotThrownCheck.class));
+    }
+
+    @AfterEach
+    public void cleanup() {
+        checker.destroy();
+    }
+
+    @Override
+    protected String getPackageLocation() {
+        return "com/azure/tools/checkstyle/checks/ExceptionLoggingChecks";
+    }
+
+    @Test
+    public void throwCreatedException() throws Exception {
+        String[] expected = {
+            expectedErrorMessage(8, 32, ExceptionCreatedButNotThrownCheck.ERROR_MESSAGE),
+        };
+        verify(checker, getPath("ThrowCreatedExceptionCheckTestData.java"), expected);
+    }
+
+    private String expectedErrorMessage(int line, int column, String message) {
+        return String.format("%d:%d: %s", line, column, message);
+    }
+}

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

@@ -0,0 +1,42 @@
+// 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.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class RawExceptionThrowCheckTest extends AbstractModuleTestSupport {
+    private Checker checker;
+
+    @BeforeEach
+    public void prepare() throws Exception {
+        checker = createChecker(createModuleConfig(RawExceptionThrowCheck.class));
+    }
+
+    @AfterEach
+    public void cleanup() {
+        checker.destroy();
+    }
+
+    @Override
+    protected String getPackageLocation() {
+        return "com/azure/tools/checkstyle/checks/ExceptionLoggingChecks";
+    }
+
+    @Test
+    public void logNewExceptionTestData() throws Exception {
+        String[] expected = {
+            expectedErrorMessage(8, 15, RawExceptionThrowCheck.ERROR_MESSAGE),
+            expectedErrorMessage(13, 33, RawExceptionThrowCheck.ERROR_MESSAGE),
+        };
+        verify(checker, getPath("LogNewExceptionCheckTestData.java"), expected);
+    }
+
+    private String expectedErrorMessage(int line, int column, String message) {
+        return String.format("%d:%d: %s", line, column, message);
+    }
+}