[Fix] CS-Rule: No external library in public API

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

@@ -21,9 +21,7 @@
  *  No external dependency exposed in public API
  */
 public class ExternalDependencyExposedCheck extends AbstractCheck {
-    private static final String EXTERNAL_DEPENDENCY_ERROR =
-        "Class ''%s'', is a class from external dependency. You should not use it as a return or method argument type.";
-
+    private static final String EXTERNAL_DEPENDENCY_ERROR = "Class ''%s'', is a class from external dependency. You should not use it as a %s type.";
     private static final Set<String> VALID_DEPENDENCY_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
         "java", "com.azure", "reactor", "io.netty.buffer.ByteBuf"
     )));
@@ -51,6 +49,7 @@ public int[] getAcceptableTokens() {
     public int[] getRequiredTokens() {
         return new int[] {
             TokenTypes.IMPORT,
+            TokenTypes.CLASS_DEF,
             TokenTypes.METHOD_DEF
         };
     }
@@ -68,7 +67,7 @@ public void visitToken(DetailAST token) {
                 // CLASS_DEF always has MODIFIERS
                 final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(
                     token.findFirstToken(TokenTypes.MODIFIERS));
-                isPublicClass = accessModifier.equals(AccessModifier.PUBLIC);
+                isPublicClass = accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED);
                 break;
             case TokenTypes.METHOD_DEF:
                 if (!isPublicClass) {
@@ -101,14 +100,14 @@ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) {
         final DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE);
         if (typeToken != null) {
             getInvalidReturnTypes(typeToken).forEach(
-                (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)));
+                (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName, "return")));
         }
 
         // Checks for the parameters of the method
         final DetailAST parametersToken = methodDefToken.findFirstToken(TokenTypes.PARAMETERS);
         if (parametersToken != null) {
             getInvalidParameterTypes(parametersToken).forEach(
-                (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)));
+                (token, parameterTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, parameterTypeName, "method argument")));
         }
     }
 
@@ -134,7 +133,7 @@ private Map<DetailAST, String> getInvalidReturnTypes(DetailAST typeToken) {
         // TYPE_ARGUMENTS, add all invalid external types to the map
         final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS);
         if (typeArgumentsToken != null) {
-            invalidReturnTypeMap.putAll(getInvalidTypeFromTypeArguments(typeArgumentsToken));
+            getInvalidParameterType(typeArgumentsToken, invalidReturnTypeMap);
         }
 
         return invalidReturnTypeMap;
@@ -150,61 +149,42 @@ private Map<DetailAST, String> getInvalidParameterTypes(DetailAST parametersType
         final Map<DetailAST, String> invalidParameterTypesMap = new HashMap<>();
         for (DetailAST ast = parametersTypeToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) {
             if (ast.getType() == TokenTypes.PARAMETER_DEF) {
-                invalidParameterTypesMap.putAll(getInvalidTypeFromTypeArguments(ast.findFirstToken(TokenTypes.TYPE)));
+                getInvalidParameterType(ast.findFirstToken(TokenTypes.TYPE), invalidParameterTypesMap);
             }
         }
         return invalidParameterTypesMap;
     }
 
     /**
-     *  A helper function that checks TYPE AST node. Since both return type and input parameter argument type has
-     *  TYPE AST node under. This function applied to both.
+     *  Get all invalid AST nodes from a given token. DFS tree traversal used to find all invalid nodes.
      *
-     * @param typeArgumentsToken TYPE_ARGUMENTS AST node
-     * @return a map that maps all the invalid TYPE_ARGUMENT node and the type name
+     * @param token TYPE_ARGUMENT, TYPE_ARGUMENTS or TYPE AST node
+     * @return a map that maps all the invalid node and the type name
      */
-    private Map<DetailAST, String> getInvalidTypeFromTypeArguments(DetailAST typeArgumentsToken) {
-        final Map<DetailAST, String> invalidTypesMap = new HashMap<>();
-        if (typeArgumentsToken == null) {
+    private Map<DetailAST, String> getInvalidParameterType(DetailAST token, Map<DetailAST, String> invalidTypesMap) {
+        if (token == null) {
             return invalidTypesMap;
         }
-        // Checks multiple type arguments
-        for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) {
-            if (ast.getType() != TokenTypes.TYPE_ARGUMENT) {
-                continue;
-            }
 
-            final String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast);
-            if (invalidTypeName != null) {
-                invalidTypesMap.put(ast, invalidTypeName);
+        for (DetailAST ast = token.getFirstChild(); ast != null; ast = ast.getNextSibling()) {
+            final int tokenType = ast.getType();
+            if (tokenType == TokenTypes.IDENT) {
+                final String identName = ast.getText();
+                if (!isValidClassDependency(identName)) {
+                    invalidTypesMap.put(ast, identName);
+                }
+            } else if (tokenType == TokenTypes.TYPE_ARGUMENT || tokenType == TokenTypes.TYPE_ARGUMENTS) {
+                getInvalidParameterType(ast, invalidTypesMap);
             }
         }
         return invalidTypesMap;
     }
 
-    /**
-     * Get invalid type name from TYPE_ARGUMENT
-     *
-     * @param typeArgumentToken TYPE_ARGUMENT AST node
-     * @return an invalid type name if it is an invalid library. Otherwise, returns null.
-     */
-    private String getInvalidTypeNameFromTypeArgument(DetailAST typeArgumentToken) {
-        final DetailAST identToken = typeArgumentToken.findFirstToken(TokenTypes.IDENT);
-        // if there is no IDENT token, implies the token is default java types.
-        if (identToken == null) {
-            return null;
-        }
-
-        final String typeName = identToken.getText();
-        // if not exist in the classPathMap, that implies the type is java default types, such as int.
-        return isValidClassDependency(typeName) ? null : typeName;
-    }
-
     /**
      * A helper function that checks for whether a class is from a valid internal dependency or is a suppression class
      *
      * @param typeName the type name of class
-     * @return true if the class  is a suppression class, otherwise, return false.
+     * @return true if the class is a suppression class, otherwise, return false.
      */
     private boolean isValidClassDependency(String typeName) {
         // If the qualified class name does not exist in the map,

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

@@ -94,11 +94,20 @@
   <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files=".*[/\\]com[/\\]microsoft[/\\].*"/>
   <suppress checks="com.azure.tools.checkstyle.checks.ThrownClientLoggerCheck" files=".*[/\\]com[/\\]microsoft[/\\].*"/>
 
-  <!-- Custom checkstyle rules that don't apply to files under test or implementation package -->
-  <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files="(.*[/\\]src[/\\]test[/\\]java[/\\].*)|(.*[/\\]implementation[/\\].*)\.java"/>
-  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientInstantiationCheck" files="(.*[/\\]src[/\\]test[/\\]java[/\\].*)|(.*[/\\]implementation[/\\].*)\.java"/>
-  <suppress checks="com.azure.tools.checkstyle.checks.ServiceClientBuilderCheck" files="(.*[/\\]src[/\\]test[/\\]java[/\\].*)|(.*[/\\]implementation[/\\].*)\.java"/>
-  <suppress checks="com.azure.tools.checkstyle.checks.ServiceInterfaceCheck" files="(.*[/\\]src[/\\]test[/\\]java[/\\].*)|(.*[/\\]implementation[/\\].*)\.java"/>
+  <!-- Custom checkstyle rules that don't apply 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.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"/>
+
+  <!-- Custom checkstyle rules that don't apply to files under implementation package -->
+  <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" 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"/>
+
+  <!-- Custom checkstyle rules that don't apply to files under samples package -->
+  <suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files=".*[/\\]samples[/\\].*\.java"/>
 
   <!-- OpenCensus should only be depended on from within the tracing-opencensus module -->
   <suppress checks="IllegalImport" files=".*[/\\]com[/\\]azure[/\\]tracing[/\\]opentelemetry[/\\]*"/>