Custom CheckStyle Rule #9: Thrown exception use ClientLogger

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

@@ -18,8 +18,6 @@ public class NoImplInPublicAPI extends AbstractCheck {
     private static final String PARAM_TYPE_ERROR = "\"%s\" class is in an implementation package, and it should not be used as a parameter type in public API. Alternatively, it can be removed from the implementation package and made public API, after appropriate API review.";
     private static final String RETURN_TYPE_ERROR = "\"%s\" class is in an implementation package, and it should not be a return type from public API. Alternatively, it can be removed from the implementation package and made public API.";
 
-    private static boolean isTrackTwo;
-    private static boolean isImplPackage;
     private Set<String> implementationClassSet = new HashSet<>();
 
     @Override
@@ -43,28 +41,11 @@ public int[] getRequiredTokens() {
 
     @Override
     public void beginTree(DetailAST root) {
-        this.isImplPackage = false;
-        this.isTrackTwo = false;
         this.implementationClassSet.clear();
     }
 
     @Override
     public void visitToken(DetailAST ast) {
-        if (ast.getType() == TokenTypes.PACKAGE_DEF) {
-            String packageName = FullIdent.createFullIdent(ast.findFirstToken(TokenTypes.DOT)).getText();
-            this.isTrackTwo = packageName.startsWith(COM_AZURE);
-            this.isImplPackage = packageName.contains(DOT_IMPLEMENTATION);
-            return;
-        } else {
-            if (this.isTrackTwo) {
-                if (this.isImplPackage) {
-                    return;
-                }
-            } else {
-                return;
-            }
-        }
-
         switch (ast.getType()) {
             case TokenTypes.IMPORT:
                 String importClassPath = FullIdent.createFullIdentBelow(ast).getText();

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

@@ -0,0 +1,110 @@
+// 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.ArrayDeque;
+import java.util.Deque;
+
+/**
+ * To throw an exception, Must throw it through a 'logger.logExceptionAsError', rather than by directly calling 'throw exception'.
+ *
+ * Skip check if throwing exception from
+ * <ol>
+ *   <li>Static method</li>
+ *   <li>Static class</li>
+ *   <li>Constructor</li>
+ * </ol>
+ */
+public class ThrowFromClientLoggerCheck extends AbstractCheck {
+    private static final String LOGGER_LOG_EXCEPTION_AS_ERROR = "logger.logExceptionAsError";
+    private static final String LOGGER_LOG_EXCEPTION_AS_WARNING = "logger.logExceptionAsWarning";
+    private static final String THROW_lOGGER_EXCEPTION_MESSAGE = "Directly throwing an exception is disallowed. Must throw through either ''%s'' or ''%s''.";
+
+    // A container stores the static status of class, skip this ThrowFromClientLoggerCheck if the class is static
+    private final Deque<Boolean> classStaticDeque = new ArrayDeque<>();
+    // A container stores the static status of method, skip this ThrowFromClientLoggerCheck if the method is static
+    private final Deque<Boolean> methodStaticDeque = new ArrayDeque<>();
+    // The variable is used to indicate if current node is still inside of constructor.
+    private boolean isInConstructor = false;
+
+    @Override
+    public int[] getDefaultTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getAcceptableTokens() {
+        return getRequiredTokens();
+    }
+
+    @Override
+    public int[] getRequiredTokens() {
+        return new int[] {
+            TokenTypes.CLASS_DEF,
+            TokenTypes.CTOR_DEF,
+            TokenTypes.LITERAL_THROW,
+            TokenTypes.METHOD_DEF
+        };
+    }
+
+    @Override
+    public void leaveToken(DetailAST token) {
+        switch (token.getType()) {
+            case TokenTypes.CLASS_DEF:
+                classStaticDeque.pop();
+                break;
+            case TokenTypes.CTOR_DEF:
+                isInConstructor = false;
+                break;
+            case TokenTypes.METHOD_DEF:
+                methodStaticDeque.pop();
+                break;
+            default:
+                // Checkstyle complains if there's no default block in switch
+                break;
+        }
+    }
+
+    @Override
+    public void visitToken(DetailAST token) {
+        switch (token.getType()) {
+            case TokenTypes.CLASS_DEF:
+                DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS);
+                classStaticDeque.addLast(modifiersToken.branchContains(TokenTypes.LITERAL_STATIC));
+                break;
+            case TokenTypes.CTOR_DEF:
+                isInConstructor = true;
+                break;
+            case TokenTypes.METHOD_DEF:
+                DetailAST methodModifiersToken = token.findFirstToken(TokenTypes.MODIFIERS);
+                methodStaticDeque.addLast(methodModifiersToken.branchContains(TokenTypes.LITERAL_STATIC));
+                break;
+            case TokenTypes.LITERAL_THROW:
+                // Skip check if the throw exception from static class, constructor or static method
+                if (classStaticDeque.isEmpty() || classStaticDeque.peekLast() || isInConstructor
+                    || methodStaticDeque.isEmpty() || methodStaticDeque.peekLast()) {
+                    return;
+                }
+                DetailAST methodCallToken = token.findFirstToken(TokenTypes.EXPR).findFirstToken(TokenTypes.METHOD_CALL);
+                if (methodCallToken == null) {
+                    log(token, String.format(THROW_lOGGER_EXCEPTION_MESSAGE, LOGGER_LOG_EXCEPTION_AS_ERROR, LOGGER_LOG_EXCEPTION_AS_WARNING));
+                    return;
+                }
+
+                String methodCallName = FullIdent.createFullIdent(methodCallToken.findFirstToken(TokenTypes.DOT)).getText();
+                if (!LOGGER_LOG_EXCEPTION_AS_ERROR.equals(methodCallName) && !LOGGER_LOG_EXCEPTION_AS_WARNING.equals(methodCallName)) {
+                    log(token, String.format(THROW_lOGGER_EXCEPTION_MESSAGE, LOGGER_LOG_EXCEPTION_AS_ERROR, LOGGER_LOG_EXCEPTION_AS_WARNING));
+                }
+                break;
+            default:
+                // Checkstyle complains if there's no default block in switch
+                break;
+        }
+    }
+}

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

@@ -95,22 +95,31 @@
   <!-- Custom checkstyle rules only check track 2 libraries -->
   <suppress checks="com\.azure\.tools\.checkstyle\.checks\..+" files=".*[/\\]com[/\\]microsoft[/\\].*"/>
 
-  <!-- Custom checkstyle rules that don't apply to files under test package -->
+  <!-- Don't apply custom Checkstyle rules to files under test package -->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.NoImplInPublicAPI" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicyCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
 
-  <!-- Custom checkstyle rules that don't apply to files under implementation package -->
+  <!-- Don't apply custom Checkstyle rules to files under implementation package -->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]implementation[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.NoImplInPublicAPI" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]implementation[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files=".*[/\\]implementation[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]implementation[/\\].*\.java"/>
 
-  <!-- Custom checkstyle rules that don't apply to files under samples package -->
+  <!-- Don't apply custom Checkstyle rules to files under samples package -->
   <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]samples[/\\].*\.java"/>
   <suppress checks="com.azure.tools.checkstyle.checks.HttpPipelinePolicyCheck" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.NoImplInPublicAPI" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files=".*[/\\]samples[/\\].*\.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files=".*[/\\]samples[/\\].*\.java"/>
 
   <!-- Don't apply custom Checkstyle rules to files under checkstyle package. -->
   <suppress checks="com\.azure\.tools\.checkstyle\.checks\..+" files=".*[/\\]tools[/\\]checkstyle[/\\].*"/>
@@ -127,9 +136,16 @@
   <!-- Any code in any package, it should never be a 'throw' keyword in the client library codebase except for in the client logger -->
 <!--  <suppress checks="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]util[/\\]logging[/\\]*"/>-->
 
-  <!-- Suppression for throws IOException Class, Will remove these classes after be able to define the error is IOException -->
-<!--  <suppress checks="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck" files=".*[/\\]src[/\\]test[/\\]java[/\\]com[/\\]azure[/\\]core[/\\]implementation[/\\](.*RestProxyStressTests[/\\].*)|(.*util[/\\]FluxUtilTests[/\\].*)\.java"/>-->
+  <!-- Skip ThrowFromClientLoggerCheck on ClientLogger class, ClientLogger is allowed to throw exception directly -->
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.core.util.logging.ClientLogger.java"/>
+
+  <!-- Suppress IO exception for now, which need code owner's attention -->
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.core.implementation.util.ClientLoggerJavaDocCodeSnippets.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlobInputStream.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlobOutputStream.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlockBlobAsyncClient.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.BlockBlobClient.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.PageBlobAsyncClient.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.file.FileAsyncClient.java"/>
 
-<!--  Storage still depends on 1.0.0-preview.2 azure core. suppress this rule for now until storage has upgraded to preview-3 azure core-->
-  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*[/\\]storage[/\\].*\.java"/>
 </suppressions>

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

@@ -319,7 +319,7 @@ page at http://checkstyle.sourceforge.net/config.html -->
 
     <!--CUSTOM CHECKS-->
     <!-- Must use 'logger.logAndThrow' but not directly calling 'throw exception' -->
-<!--    <module name="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck"/>-->
+    <module name="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck"/>
 
     <!--CUSTOM CHECKS-->
     <!-- Any class that implements the HttpPipelinePolicy interface should:

sdk/appconfiguration/azure-data-appconfiguration/src/main/java/com/azure/data/appconfiguration/credentials/ConfigurationClientCredentials.java

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 package com.azure.data.appconfiguration.credentials;
 
+import com.azure.core.util.logging.ClientLogger;
 import com.azure.data.appconfiguration.ConfigurationClientBuilder;
 import com.azure.data.appconfiguration.policy.ConfigurationCredentialsPolicy;
 import com.azure.core.implementation.util.ImplUtils;
@@ -37,6 +38,8 @@
  * @see ConfigurationClientBuilder
  */
 public class ConfigurationClientCredentials {
+    private final ClientLogger logger = new ClientLogger(ConfigurationClientCredentials.class);
+
     private static final String HOST_HEADER = "Host";
     private static final String DATE_HEADER = "Date";
     private static final String CONTENT_HASH_HEADER = "x-ms-content-sha256";
@@ -81,7 +84,7 @@ public Mono<Map<String, String>> getAuthorizationHeadersAsync(URL url, String ht
                 try {
                     return MessageDigest.getInstance("SHA-256");
                 } catch (NoSuchAlgorithmException e) {
-                    throw Exceptions.propagate(e);
+                    throw logger.logExceptionAsError(Exceptions.propagate(e));
                 }
             }, (messageDigest, byteBuffer) -> {
                     if (messageDigest != null) {

sdk/core/azure-core-management/src/main/java/com/azure/core/management/PagedList.java

@@ -4,6 +4,7 @@
 package com.azure.core.management;
 
 import com.azure.core.exception.HttpResponseException;
+import com.azure.core.util.logging.ClientLogger;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -21,6 +22,8 @@
  * @param <E> the element type.
  */
 public abstract class PagedList<E> implements List<E> {
+    private final ClientLogger logger = new ClientLogger(PagedList.class);
+
     /** The actual items in the list. */
     private List<E> items;
     /** Stores the latest page fetched. */
@@ -67,7 +70,7 @@ private void cachePage(String nextPageLink) {
                 }
             }
         } catch (IOException ex) {
-            throw new RuntimeException(ex);
+            throw logger.logExceptionAsError(new RuntimeException(ex));
         }
     }
 
@@ -137,6 +140,8 @@ protected void setCurrentPage(Page<E> currentPage) {
      * The implementation of {@link ListIterator} for PagedList.
      */
     private class ListItr implements ListIterator<E> {
+        private final ClientLogger logger = new ClientLogger(ListItr.class);
+
         /**
          * index of next element to return.
          */
@@ -164,7 +169,7 @@ public boolean hasNext() {
         public E next() {
             if (this.nextIndex >= items.size()) {
                 if (!hasNextPage()) {
-                    throw new NoSuchElementException();
+                    throw logger.logExceptionAsError(new NoSuchElementException());
                 } else {
                     loadNextPage();
                 }
@@ -179,22 +184,22 @@ public E next() {
                 } catch (IndexOutOfBoundsException ex) {
                     // The nextIndex got invalid means a different instance of iterator
                     // removed item from this index.
-                    throw new ConcurrentModificationException();
+                    throw logger.logExceptionAsError(new ConcurrentModificationException(ex));
                 }
             }
         }
 
         @Override
         public void remove() {
             if (this.lastRetIndex < 0) {
-                throw new IllegalStateException();
+                throw logger.logExceptionAsError(new IllegalStateException());
             } else {
                 try {
                     items.remove(this.lastRetIndex);
                     this.nextIndex = this.lastRetIndex;
                     this.lastRetIndex = -1;
                 } catch (IndexOutOfBoundsException ex) {
-                    throw new ConcurrentModificationException();
+                    throw logger.logExceptionAsError(new ConcurrentModificationException(ex));
                 }
             }
         }
@@ -208,16 +213,16 @@ public boolean hasPrevious() {
         public E previous() {
             int i = this.nextIndex - 1;
             if (i < 0) {
-                throw new NoSuchElementException();
+                throw logger.logExceptionAsError(new NoSuchElementException());
             } else if (i >= items.size()) {
-                throw new ConcurrentModificationException();
+                throw logger.logExceptionAsError(new ConcurrentModificationException());
             } else {
                 try {
                     this.nextIndex = i;
                     this.lastRetIndex = i;
                     return items.get(this.lastRetIndex);
                 } catch (IndexOutOfBoundsException ex) {
-                    throw new ConcurrentModificationException();
+                    throw logger.logExceptionAsError(new ConcurrentModificationException(ex));
                 }
             }
         }
@@ -235,12 +240,12 @@ public int previousIndex() {
         @Override
         public void set(E e) {
             if (this.lastRetIndex < 0) {
-                throw new IllegalStateException();
+                throw logger.logExceptionAsError(new IllegalStateException());
             } else {
                 try {
                     items.set(this.lastRetIndex, e);
                 } catch (IndexOutOfBoundsException ex) {
-                    throw new ConcurrentModificationException();
+                    throw logger.logExceptionAsError(new ConcurrentModificationException(ex));
                 }
             }
         }
@@ -252,7 +257,7 @@ public void add(E e) {
                 this.nextIndex = this.nextIndex + 1;
                 this.lastRetIndex = -1;
             } catch (IndexOutOfBoundsException ex) {
-                throw new ConcurrentModificationException();
+                throw logger.logExceptionAsError(new ConcurrentModificationException(ex));
             }
         }
     }