Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Jul 30, 2019
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
mssfang marked this conversation as resolved.
Show resolved Hide resolved
*/
private boolean isValidClassDependency(String typeName) {
// If the qualified class name does not exist in the map,
Expand Down
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.databind.JsonSerializer;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.jsontype.TypeSerializer;
import com.fasterxml.jackson.databind.module.SimpleModule;

import java.io.IOException;
Expand Down