diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/NoImplInPublicAPI.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/NoImplInPublicAPI.java index a21ed603ab72..99e9ee2ce794 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/NoImplInPublicAPI.java +++ b/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 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(); diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrowFromClientLoggerCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrowFromClientLoggerCheck.java new file mode 100644 index 000000000000..fb8018d62286 --- /dev/null +++ b/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 + *
    + *
  1. Static method
  2. + *
  3. Static class
  4. + *
  5. Constructor
  6. + *
+ */ +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 classStaticDeque = new ArrayDeque<>(); + // A container stores the static status of method, skip this ThrowFromClientLoggerCheck if the method is static + private final Deque 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; + } + } +} diff --git a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml index e5c597c93aaa..186bbb917d2f 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml @@ -96,19 +96,20 @@ - + + + |ThrowFromClientLoggerCheck|EnforceFinalFieldsCheck)" files=".*[/\\]src[/\\]test[/\\]java[/\\].*\.java"/> - - + - + + |EnforceFinalFieldsCheck|ThrowFromClientLoggerCheck)" files=".*[/\\]samples[/\\].*\.java"/> @@ -118,4 +119,14 @@ + + + + + + + + + + diff --git a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml index d948fe1d7454..9e868e047b27 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -329,7 +329,7 @@ page at http://checkstyle.sourceforge.net/config.html --> - +