Custom CheckStyle Rule #9: Thrown exception use ClientLogger (Azure#4566

)

(1) the implementation for Rule 9: Thrown exception use ClientLogger
To throw an exception, Must throw it through a 'logger.logExceptionAsError', rather than by directly calling 'throw exception'.
Skip check if throw exception from
Static method
Static class
Constructor
(2) code fixes based on the new rule: ThrowFromClientLoggerCheck.java
(3) a minor fixes for NoImplInPublicAPI.java checker:
Removes track 2 and implementation check in Java Code instead of suppressing the rule
(4) fixes for ServiceClientBuilder checker for Storage service.

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,111 @@
+// 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 ''ClientLogger'' API, either of ''%s'' or ''%s'' where ''logger'' is type of ClientLogger from Azure Core package.";
+
+    // 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

@@ -96,19 +96,20 @@
 
   <!-- Remove this after fixing: https://github.com/Azure/azure-sdk-for-java/issues/5021 -->
   <suppress checks="com.azure.tools.checkstyle.checks.EnforceFinalFieldsCheck" files=".*\BlobOutputStream.java"/>
-
+
+  <!-- Don't apply custom Checkstyle rules to files under test package -->
   <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|NoImplInPublicAPI|ServiceClientInstantiationCheck
   |ServiceClientBuilderCheck|ServiceInterfaceCheck|HttpPipelinePolicyCheck|JavaDocFormatting|JavadocThrowsChecks
-  |EnforceFinalFieldsCheck)" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
+  |ThrowFromClientLoggerCheck|EnforceFinalFieldsCheck)" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/>
 
-  <!-- Custom checkstyle rules that don't apply to files under implementation package -->
-  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|ServiceClientInstantiationCheck
-  |ServiceClientBuilderCheck|ServiceInterfaceCheck|JavaDocFormatting|JavadocThrowsChecks|EnforceFinalFieldsCheck)"
+  <!-- Don't apply custom Checkstyle rules to files under implementation package -->
+  <suppress checks="com\.azure\.tools\.checkstyle\.checks\.(ExternalDependencyExposedCheck|NoImplInPublicAPI|ServiceClientInstantiationCheck
+  |ServiceClientBuilderCheck|ServiceInterfaceCheck|JavaDocFormatting|JavadocThrowsChecks|EnforceFinalFieldsCheck|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|HttpPipelinePolicyCheck|JavadocPackage
-  |EnforceFinalFieldsCheck)" files=".*[/\\]samples[/\\].*\.java"/>
+  |EnforceFinalFieldsCheck|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[/\\].*"/>
@@ -118,4 +119,14 @@
 
   <!-- Suppress warnings for Event Processor until the usage of "Client" is discussed and resolved: https://github.com/Azure/azure-sdk/issues/321 -->
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files=".*EventHubClientBuilder.java"/>
+
+  <!-- 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.ThrowFromClientLoggerCheck" files=".*[/\\]com[/\\]azure[/\\]core[/\\]util[/\\]logging[/\\]*"/>
+
+  <!-- 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.BlockBlobClient.java"/>
+  <suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLoggerCheck" files="com.azure.storage.blob.PageBlobAsyncClient.java"/>
 </suppressions>

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

@@ -329,7 +329,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 final 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));
             }
         }
     }

sdk/core/azure-core-management/src/main/java/com/azure/core/management/implementation/AzureAsyncOperationPollStrategy.java

@@ -11,6 +11,7 @@
 import com.azure.core.management.AsyncOperationResource;
 import com.azure.core.management.CloudException;
 import com.azure.core.management.OperationState;
+import com.azure.core.util.logging.ClientLogger;
 import reactor.core.publisher.Mono;
 
 import java.io.IOException;
@@ -23,6 +24,8 @@
  * running operation.
  */
 public final class AzureAsyncOperationPollStrategy extends PollStrategy {
+    private final ClientLogger logger = new ClientLogger(AzureAsyncOperationPollStrategy.class);
+
     private AzureAsyncOperationPollStrategyData data;
 
     /**
@@ -100,7 +103,7 @@ public HttpRequest createPollRequest() {
                 pollUrl = data.originalResourceUrl;
             }
         } else {
-            throw new IllegalStateException("Polling is completed and did not succeed. Cannot create a polling request.");
+            throw logger.logExceptionAsError(new IllegalStateException("Polling is completed and did not succeed. Cannot create a polling request."));
         }
 
         return new HttpRequest(HttpMethod.GET, pollUrl);
@@ -122,7 +125,7 @@ public Mono<HttpResponse> updateFromAsync(HttpResponse httpPollResponse) {
                                     } catch (IOException ignored) { }
                                     //
                                     if (operationResource == null || operationResource.status() == null) {
-                                        throw new CloudException("The polling response does not contain a valid body", bufferedHttpPollResponse, null);
+                                        throw logger.logExceptionAsError(new CloudException("The polling response does not contain a valid body", bufferedHttpPollResponse, null));
                                     } else {
                                         final String status = operationResource.status();
                                         setStatus(status);
@@ -133,7 +136,7 @@ public Mono<HttpResponse> updateFromAsync(HttpResponse httpPollResponse) {
                                             clearDelayInMilliseconds();
 
                                             if (!data.pollingSucceeded) {
-                                                throw new CloudException("Async operation failed with provisioning state: " + status, bufferedHttpPollResponse);
+                                                throw logger.logExceptionAsError(new CloudException("Async operation failed with provisioning state: " + status, bufferedHttpPollResponse));
                                             }
 
                                             if (operationResource.id() != null) {