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 8 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,16 +21,13 @@
* 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 Set<String> VALID_DEPENDENCY_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
"java", "com.azure", "reactor", "io.netty.buffer.ByteBuf"
)));

private final Map<String, String> simpleClassNameToQualifiedNameMap = new HashMap<>();

private boolean isPublicClass;
private boolean isPublicAPI;
mssfang marked this conversation as resolved.
Show resolved Hide resolved

@Override
public void beginTree(DetailAST rootAST) {
Expand All @@ -51,6 +48,7 @@ public int[] getAcceptableTokens() {
public int[] getRequiredTokens() {
return new int[] {
TokenTypes.IMPORT,
TokenTypes.CLASS_DEF,
TokenTypes.METHOD_DEF
};
}
Expand All @@ -68,10 +66,10 @@ public void visitToken(DetailAST token) {
// CLASS_DEF always has MODIFIERS
final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(
token.findFirstToken(TokenTypes.MODIFIERS));
isPublicClass = accessModifier.equals(AccessModifier.PUBLIC);
isPublicAPI = accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED);
break;
case TokenTypes.METHOD_DEF:
if (!isPublicClass) {
if (!isPublicAPI) {
return;
}
checkNoExternalDependencyExposed(token);
Expand Down Expand Up @@ -101,14 +99,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("Class ''%s'', is a class from external dependency. You should not use it as a return type.", returnTypeName)));
}

// 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("Class ''%s'', is a class from external dependency. You should not use it as a method argument type.", parameterTypeName)));
mssfang marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -149,10 +147,12 @@ private Map<DetailAST, String> getInvalidReturnTypes(DetailAST typeToken) {
private Map<DetailAST, String> getInvalidParameterTypes(DetailAST parametersTypeToken) {
final Map<DetailAST, String> invalidParameterTypesMap = new HashMap<>();
for (DetailAST ast = parametersTypeToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) {
// log(ast, "ast type (getInvalidParameterTypes) ==== " + ast.getText());
if (ast.getType() == TokenTypes.PARAMETER_DEF) {
invalidParameterTypesMap.putAll(getInvalidTypeFromTypeArguments(ast.findFirstToken(TokenTypes.TYPE)));
}
}
// log(parametersTypeToken, "Invalid Type (parametersTypeToken) MAP size = " + invalidParameterTypesMap.size());
mssfang marked this conversation as resolved.
Show resolved Hide resolved
return invalidParameterTypesMap;
}

Expand All @@ -170,13 +170,16 @@ private Map<DetailAST, String> getInvalidTypeFromTypeArguments(DetailAST typeArg
}
// 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);
if (ast.getType() == TokenTypes.IDENT) {
final String identName = ast.getText();
if (!isValidClassDependency(identName)) {
invalidTypesMap.put(ast, identName);
}
} else if (ast.getType() == TokenTypes.TYPE_ARGUMENT) {
final String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast);
if (invalidTypeName != null) {
invalidTypesMap.put(ast, invalidTypeName);
}
}
}
return invalidTypesMap;
Expand Down Expand Up @@ -204,7 +207,7 @@ private String getInvalidTypeNameFromTypeArgument(DetailAST typeArgumentToken) {
* 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,8 +94,10 @@
<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 or samples -->
<suppress checks="com.azure.tools.checkstyle.checks.ExternalDependencyExposedCheck" files="(.*[/\\]src[/\\]test[/\\]java[/\\].*)|(.*[/\\]implementation[/\\].*)|(.*[/\\]samples[/\\].*)\.java"/>
mssfang marked this conversation as resolved.
Show resolved Hide resolved

<!-- 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"/>
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