Skip to content

Commit

Permalink
[Fix] CS-Rule: No external library in public API (#4624)
Browse files Browse the repository at this point in the history 
* remove java code isImple check but move to suppression and add only check for public class for external Dependency check

* external dependency check fix

* skip check for sample

* remove unused import

* remove test code

* remove comments

* refactor error message

* refactor the sections of suppression for easier to review

* wording

* generic model
  • Loading branch information
@mssfang
mssfang authored Jul 30, 2019
1 parent 7ec7ffa commit 2db7c54
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 47 deletions.
64 changes: 22 additions & 42 deletions ...ports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)));
Expand Down Expand Up @@ -51,6 +49,7 @@ public int[] getAcceptableTokens() {
public int[] getRequiredTokens() {
return new int[] {
TokenTypes.IMPORT,
TokenTypes.CLASS_DEF,
TokenTypes.METHOD_DEF
};
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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")));
}
}

Expand All @@ -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;
Expand All @@ -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,
Expand Down
19 changes: 14 additions & 5 deletions eng/code-quality-reports/src/main/resources/checkstyle/checkstyle-suppressions.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[/\\]*"/>
Expand Down

0 comments on commit 2db7c54

Please sign in to comment.