From 67fb33df20679dd631f364acb4b21c46c57e7809 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 2 Jul 2019 11:35:12 -0700 Subject: [PATCH 01/35] Service Client Instantiation Check rule --- eng/code-quality-reports/pom.xml | 127 ++++++++++--- .../ServiceClientInstantiationCheck.java | 169 ++++++++++++++++++ 2 files changed, 275 insertions(+), 21 deletions(-) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java diff --git a/eng/code-quality-reports/pom.xml b/eng/code-quality-reports/pom.xml index 78fe8ebfdc4b6..03f889fadc0b7 100755 --- a/eng/code-quality-reports/pom.xml +++ b/eng/code-quality-reports/pom.xml @@ -3,8 +3,15 @@ - 4.0.0 + + + + + + + + 4.0.0 com.azure sdk-build-tools Maven Build Tools @@ -23,7 +30,9 @@ 1.8 8.18 - 4.12 + + 1.0.0-SNAPSHOT + 5.0.0-preview.1 @@ -38,25 +47,13 @@ - - com.puppycrawl.tools - checkstyle - ${checkstyle.version} - - - - com.puppycrawl.tools - checkstyle - ${checkstyle.version} - test-jar - test - - - junit - junit - ${junit.version} - test - + + + + + + + @@ -84,7 +81,95 @@ maven-project-info-reports-plugin ${maven-project-info-reports-plugin.version} + + + org.codehaus.mojo + build-helper-maven-plugin + + + add-client-source + ${client.phase} + + add-source + + + + + + + + + + + ..\..\eventhubs\client\azure-eventhubs\src\main\java + ..\..\eventhubs\client\azure-eventhubs\src\samples\java + + + + + + + + client-modules + + + !data-plane + + true + + + + com.puppycrawl.tools + checkstyle + ${checkstyle.version} + + + + com.puppycrawl.tools + checkstyle + ${checkstyle.version} + test-jar + test + + + + + + + + + + + + + + + + + + + + + + + + + + + + com.azure + azure-messaging-eventhubs + ${azure-messaging-eventhubs.version} + + + + generate-sources + none + + + + diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java new file mode 100644 index 0000000000000..22659ac6f5c22 --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -0,0 +1,169 @@ +// 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.TokenTypes; + +/** + * Verify the classes with annotation @ServiceClient should have following rules: + * <0l> + *
  • No public or protected constructors
    • + *
  • No public static method named 'builder'
    • + *
  • Since these classes are supposed to be immutable, all fields in the service client classes should be final.
    • + * + */ +public class ServiceClientInstantiationCheck extends AbstractCheck { + private static final String SERVICE_CLIENT = "ServiceClient"; + private static final String BUILDER = "builder"; + private static final String ASYNC_CLIENT ="AsyncClient"; + private static final String CLIENT = "Client"; + + private static boolean hasServiceClientAnnotation; + private static boolean notServiceClass; + + @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.METHOD_DEF, + TokenTypes.LITERAL_CLASS, + TokenTypes.OBJBLOCK + }; + } + + @Override + public void beginTree(DetailAST root) { + notServiceClass = false; + } + + @Override + public void visitToken(DetailAST token) { + if (notServiceClass) { + return; + } + + switch (token.getType()) { + case TokenTypes.CLASS_DEF: + hasServiceClientAnnotation = checkServiceClientAnnotation(token); + if (!hasServiceClientAnnotation) { + notServiceClass = true; + } + break; + case TokenTypes.CTOR_DEF: + checkNonPublicOrProtectedConstructor(token); + break; + case TokenTypes.METHOD_DEF: + checkNonPublicStaticBuilderMethodName(token); + break; + case TokenTypes.LITERAL_CLASS: + checkServiceClientNaming(token); + break; + case TokenTypes.OBJBLOCK: + checkClassFieldFinal(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + /** + * Check if the class is annotated with @ServiceClient + * + * @param token the CLASS_DEF AST node + * @return true if the class is annotated with @ServiceClient, false otherwise. + */ + private boolean checkServiceClientAnnotation(DetailAST token) { + DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken != null) { + DetailAST annotationAST = modifiersToken.findFirstToken(TokenTypes.ANNOTATION); + if (annotationAST != null) { + DetailAST annotationIdent = annotationAST.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { + return true; + } + } + } + return false; + } + + /** + * Check fo public or protected constructor for the service client class. + * Log error if the service client has public or protected constructor. + * + * @param token the CTOR_DEF AST node + */ + private void checkNonPublicOrProtectedConstructor(DetailAST token) { + DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken != null) { + if (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) + || modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED)) { + log(modifiersToken, "Subclass of ServiceClient should not have any public or protected constructor."); + } + } + } + + /** + * Check for public static method named 'builder'. Should avoid to use method name, 'builder'. + * + * @param token the METHOD_DEF AST node + */ + private void checkNonPublicStaticBuilderMethodName(DetailAST token) { + DetailAST methodNameToken = token.findFirstToken(TokenTypes.IDENT); + if (methodNameToken != null && BUILDER.equals(methodNameToken.getText())) { + DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) + && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { + log(modifiersToken, "Subclass of ServiceClient should not have a public static method named ''builder''."); + } + } + } + + /** + * Check for the variable field of the subclass of ServiceClient. + * These fields should be final because these classes supposed to be immutable class. + * + * @param token the MODIFIERS AST node + */ + private void checkClassFieldFinal(DetailAST token) { + for (DetailAST ast = token.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (TokenTypes.VARIABLE_DEF == ast.getType()) { + DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); + if (!modifiersToken.branchContains(TokenTypes.FINAL)) { + log(modifiersToken, "The variable field of the subclass of ServiceClient should be final. These classes supposed to be immutable."); + } + } + } + } + + /** + * Check for the class name of Service Client. It class should be named AsyncClient or Client. + * + * @param token the LITERAL_CLASS AST node + */ + private void checkServiceClientNaming(DetailAST token) { + for (DetailAST ast = token; ast != null; ast = ast.getNextSibling()) { + if (TokenTypes.IDENT == ast.getType()) { + String className = ast.getText(); + if (!className.endsWith(ASYNC_CLIENT) && !className.endsWith(CLIENT)) { + log(ast, String.format("Class name %s should named AsyncClient or Client.", className)); + } + break; + } + } + } +} From e9db7bba75a6e1c4ad485a0719afb30cd7625c77 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 2 Jul 2019 23:29:10 -0700 Subject: [PATCH 02/35] add Extenal Dependentcy Exposed checks and checkstyle xml --- .../ExternalDependencyExposedCheck.java | 201 ++++++++++++++++++ .../main/resources/checkstyle/checkstyle.xml | 16 +- 2 files changed, 215 insertions(+), 2 deletions(-) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java new file mode 100644 index 0000000000000..324880d242251 --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java @@ -0,0 +1,201 @@ +// 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.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +/** + * No external dependency exposed in the public API + */ +public class ExternalDependencyExposedCheck extends AbstractCheck { + private static final String JAVA = "java"; + private static final String COM_AZURE = "com.azure"; + private static final String REACTOR = "reactor"; + private static final String DOT = "."; + + private static final String EXTERNAL_DEPENDENCY_ERROR = + "Class ''%s'', is a class from external dependency. You should not use it as a return or parameter argument type"; + + private static final Set validLibrarySet = new HashSet<>(); + private static Map classPathMap = new HashMap<>(); + + @Override + public void init() { + validLibrarySet.add(JAVA); + validLibrarySet.add(COM_AZURE); + validLibrarySet.add(REACTOR); + } + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.IMPORT, + TokenTypes.METHOD_DEF + }; + } + + @Override + public void visitToken(DetailAST token) { + switch (token.getType()) { + case TokenTypes.IMPORT: + collectAllImportedClassPath(token); + break; + case TokenTypes.METHOD_DEF: + checkNoExternalDependencyExposed(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + /** + * Collect all imported classes into a map, key is the name of class and value is the full package path of class. + * + * @param token the IMPORT AST node + */ + private void collectAllImportedClassPath(DetailAST token) { + final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); + final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); + classPathMap.put(className, importClassPath); + } + + /** + * A Check for external dependency, log the error if it is an invalid external dependency. + * + * @param methodDefToken METHOD_DEF AST node + */ + private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { + final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + // Public API + if (modifiersToken != null && + (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) + || modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED))) { + checkReturnType(methodDefToken.findFirstToken(TokenTypes.TYPE)); + checkParametersType(methodDefToken.findFirstToken(TokenTypes.PARAMETERS)); + } + } + + /** + * Checks for return type + * + * @param typeToken TYPE AST node + */ + private void checkReturnType(DetailAST typeToken) { + final int typeCount = typeToken.getChildCount(); + // The TYPE node has more than one child, such as Map + if (typeCount == 1) { + final String returnTypeName = typeToken.getFirstChild().getText(); + // if not exist in the classPathMap, that implies the type is java default types, such as int. + if (classPathMap.containsKey(returnTypeName)) { + if (!isValidLibrary(classPathMap.get(returnTypeName))) { + log(typeToken, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)); + } + } + } else { + checkTypeArguments(typeToken); + } + } + + /** + * Checks for input parameters + * + * @param parametersTypeToken PARAMETERS AST node + */ + private void checkParametersType(DetailAST parametersTypeToken) { + for (DetailAST ast = parametersTypeToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.PARAMETER_DEF) { + checkTypeArguments(ast.findFirstToken(TokenTypes.TYPE)); + } + } + } + + /** + * 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. + * + * @param typeToken TYPE AST node + */ + private void checkTypeArguments(DetailAST typeToken) { + final DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); + // if there is no IDENT token, implies the token is default java types. + if (identToken != null) { + final String typeName = identToken.getText(); + // if not exist in the classPathMap, that implies the type is java default types, such as int. + if (classPathMap.containsKey(typeName)) { + if (!isValidLibrary(classPathMap.get(typeName))) { + log(typeToken, String.format(EXTERNAL_DEPENDENCY_ERROR, typeName)); + } else { + // Checks multiple type arguments + final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS); + if (typeArgumentsToken != null) { + for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { + checkSingleTypeArgument(ast); + } + } + } + // Checks single type argument + final DetailAST typeArgumentToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENT); + if (typeArgumentToken != null) { + checkSingleTypeArgument(typeArgumentToken); + } + } + } + } + } + + /** + * A helper function that checks for single type arguments scenario. + * + * @param typeArgumentToken TYPE_ARGUMENT AST node + */ + private void checkSingleTypeArgument(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) { + final String typeName = identToken.getText(); + // if not exist in the classPathMap, that implies the type is java default types, such as int. + if (classPathMap.containsKey(typeName)) { + if (!isValidLibrary(classPathMap.get(typeName))) { + log(typeArgumentToken, String.format(EXTERNAL_DEPENDENCY_ERROR, typeName)); + } + } + } + } + + /** + * A helper function that checks for whether a class is from a valid internal dependency or is a suppression class + * + * @param classPath the full class path for a given class + * @return true if the class is a suppression class, otherwise, return false. + */ + private boolean isValidLibrary(String classPath) { + boolean isValidLibrary = false; + for (String lib : validLibrarySet) { + if (classPath.startsWith(lib)) { + isValidLibrary = true; + } + } + return isValidLibrary; + } +} 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 5594540a232fe..0e37ecfac62cf 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -274,9 +274,21 @@ page at http://checkstyle.sourceforge.net/config.html --> + 1) not return classes in the implementation package + 2) no class of implementation package as method's parameters --> + + + + + + + From d8633dc816b8408cea73e1647a0dceced5330926 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 2 Jul 2019 23:41:48 -0700 Subject: [PATCH 03/35] revert back pom file --- eng/code-quality-reports/pom.xml | 127 +++++-------------------------- 1 file changed, 21 insertions(+), 106 deletions(-) diff --git a/eng/code-quality-reports/pom.xml b/eng/code-quality-reports/pom.xml index 03f889fadc0b7..78fe8ebfdc4b6 100755 --- a/eng/code-quality-reports/pom.xml +++ b/eng/code-quality-reports/pom.xml @@ -3,15 +3,8 @@ - - - - - - - - 4.0.0 + com.azure sdk-build-tools Maven Build Tools @@ -30,9 +23,7 @@ 1.8 8.18 - - 1.0.0-SNAPSHOT - 5.0.0-preview.1 + 4.12 @@ -47,13 +38,25 @@ - - - - - - - + + com.puppycrawl.tools + checkstyle + ${checkstyle.version} + + + + com.puppycrawl.tools + checkstyle + ${checkstyle.version} + test-jar + test + + + junit + junit + ${junit.version} + test + @@ -81,95 +84,7 @@ maven-project-info-reports-plugin ${maven-project-info-reports-plugin.version} - - - org.codehaus.mojo - build-helper-maven-plugin - - - add-client-source - ${client.phase} - - add-source - - - - - - - - - - - ..\..\eventhubs\client\azure-eventhubs\src\main\java - ..\..\eventhubs\client\azure-eventhubs\src\samples\java - - - - - - - - client-modules - - - !data-plane - - true - - - - com.puppycrawl.tools - checkstyle - ${checkstyle.version} - - - - com.puppycrawl.tools - checkstyle - ${checkstyle.version} - test-jar - test - - - - - - - - - - - - - - - - - - - - - - - - - - - - com.azure - azure-messaging-eventhubs - ${azure-messaging-eventhubs.version} - - - - generate-sources - none - - - - From ab7e6a2c4d3b91d1769f54e5907233a9525b9a23 Mon Sep 17 00:00:00 2001 From: shafang Date: Fri, 5 Jul 2019 14:09:38 -0700 Subject: [PATCH 04/35] refactor 1 --- .../ExternalDependencyExposedCheck.java | 147 +++++++++++------- .../ServiceClientInstantiationCheck.java | 17 +- 2 files changed, 100 insertions(+), 64 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java index 324880d242251..72c16b488bea7 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java @@ -8,6 +8,8 @@ import com.puppycrawl.tools.checkstyle.api.FullIdent; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -25,14 +27,15 @@ 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 parameter argument type"; - private static final Set validLibrarySet = new HashSet<>(); - private static Map classPathMap = new HashMap<>(); + private static final Set VALID_DEPENDENCY_PACKAGE_NAMES = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( + JAVA, COM_AZURE, REACTOR + ))); + + private Map classPathMap = new HashMap<>(); @Override - public void init() { - validLibrarySet.add(JAVA); - validLibrarySet.add(COM_AZURE); - validLibrarySet.add(REACTOR); + public void beginTree(DetailAST rootAST) { + classPathMap.clear(); } @Override @@ -57,7 +60,7 @@ public int[] getRequiredTokens() { public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.IMPORT: - collectAllImportedClassPath(token); + addImportedClassPath(token); break; case TokenTypes.METHOD_DEF: checkNoExternalDependencyExposed(token); @@ -69,64 +72,81 @@ public void visitToken(DetailAST token) { } /** - * Collect all imported classes into a map, key is the name of class and value is the full package path of class. + * Add all imported classes into a map, key is the name of class and value is the full package path of class. * * @param token the IMPORT AST node */ - private void collectAllImportedClassPath(DetailAST token) { + private void addImportedClassPath(DetailAST token) { final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); classPathMap.put(className, importClassPath); } /** - * A Check for external dependency, log the error if it is an invalid external dependency. + * Checks for external dependency, log the error if it is an invalid external dependency. * * @param methodDefToken METHOD_DEF AST node */ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); - // Public API - if (modifiersToken != null && - (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) - || modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED))) { - checkReturnType(methodDefToken.findFirstToken(TokenTypes.TYPE)); - checkParametersType(methodDefToken.findFirstToken(TokenTypes.PARAMETERS)); + if (modifiersToken == null) { + return; + } + + if (!modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) + && !modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED)) { + return; + } + + DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); + if (typeToken != null) { + getInvalidReturnTypes(typeToken).forEach( + (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName))); + } + + DetailAST parametersToken = methodDefToken.findFirstToken(TokenTypes.PARAMETERS); + if (parametersToken != null) { + getInvalidParameterTypes(parametersToken).forEach( + (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)));; } } /** - * Checks for return type + * Get invalid return types from a given TYPE node. * * @param typeToken TYPE AST node + * @return a map that maps the invalid TYPE node and the type name. */ - private void checkReturnType(DetailAST typeToken) { + private Map getInvalidReturnTypes(DetailAST typeToken) { + Map invalidReturnTypeMap = new HashMap<>(); final int typeCount = typeToken.getChildCount(); - // The TYPE node has more than one child, such as Map + // The TYPE node could has more than one child, such as Map if (typeCount == 1) { final String returnTypeName = typeToken.getFirstChild().getText(); // if not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(returnTypeName)) { - if (!isValidLibrary(classPathMap.get(returnTypeName))) { - log(typeToken, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)); - } + if (classPathMap.containsKey(returnTypeName) && !isValidLibrary(classPathMap.get(returnTypeName))) { + invalidReturnTypeMap.put(typeToken, returnTypeName); } } else { - checkTypeArguments(typeToken); + invalidReturnTypeMap = getInvalidTypeFromTypeArguments(typeToken); } + return invalidReturnTypeMap; } /** - * Checks for input parameters + * Get invalid parameter types from a given PARAMETERS node. * * @param parametersTypeToken PARAMETERS AST node + * @return a map that maps all the invalid TYPE_ARGUMENT node and the type name */ - private void checkParametersType(DetailAST parametersTypeToken) { + private Map getInvalidParameterTypes(DetailAST parametersTypeToken) { + final Map invalidParameterTypesMap = new HashMap<>(); for (DetailAST ast = parametersTypeToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { if (ast.getType() == TokenTypes.PARAMETER_DEF) { - checkTypeArguments(ast.findFirstToken(TokenTypes.TYPE)); + invalidParameterTypesMap.putAll(getInvalidTypeFromTypeArguments(ast.findFirstToken(TokenTypes.TYPE))); } } + return invalidParameterTypesMap; } /** @@ -134,53 +154,72 @@ private void checkParametersType(DetailAST parametersTypeToken) { * TYPE AST node under. This function applied to both. * * @param typeToken TYPE AST node + * @return a map that maps all the invalid TYPE_ARGUMENT node and the type name */ - private void checkTypeArguments(DetailAST typeToken) { + private Map getInvalidTypeFromTypeArguments(DetailAST typeToken) { + final Map invalidTypesMap = new HashMap<>(); + + if (typeToken == null) { + return invalidTypesMap; + } + final DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); - // if there is no IDENT token, implies the token is default java types. - if (identToken != null) { - final String typeName = identToken.getText(); - // if not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(typeName)) { - if (!isValidLibrary(classPathMap.get(typeName))) { - log(typeToken, String.format(EXTERNAL_DEPENDENCY_ERROR, typeName)); - } else { - // Checks multiple type arguments - final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS); - if (typeArgumentsToken != null) { - for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { - checkSingleTypeArgument(ast); + // if there is no IDENT token, implies the token is a default java type. + if (identToken == null) { + return invalidTypesMap; + } + + final String typeName = identToken.getText(); + // if does not exist in the classPathMap, that implies the type is java default types, such as int. + if (classPathMap.containsKey(typeName)) { + if (!isValidLibrary(classPathMap.get(typeName))) { + invalidTypesMap.put(typeToken, typeName); + } else { + // Checks multiple type arguments + final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS); + if (typeArgumentsToken != null) { + for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { + String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast); + if (invalidTypeName != null) { + invalidTypesMap.put(ast, invalidTypeName); } } } + } else { // Checks single type argument final DetailAST typeArgumentToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENT); - if (typeArgumentToken != null) { - checkSingleTypeArgument(typeArgumentToken); + String invalidTypeName = getInvalidTypeNameFromTypeArgument(typeArgumentToken); + if (invalidTypeName != null) { + invalidTypesMap.put(typeArgumentToken, invalidTypeName); } } } } + return invalidTypesMap; } /** - * A helper function that checks for single type arguments scenario. + * Get invalid type name. * * @param typeArgumentToken TYPE_ARGUMENT AST node + * @return an invalid type name if it is an invalid library. Otherwise, returns null. */ - private void checkSingleTypeArgument(DetailAST typeArgumentToken) { + private String getInvalidTypeNameFromTypeArgument(DetailAST typeArgumentToken) { + if (typeArgumentToken == null) { + return null; + } + final DetailAST identToken = typeArgumentToken.findFirstToken(TokenTypes.IDENT); // if there is no IDENT token, implies the token is default java types. if (identToken != null) { final String typeName = identToken.getText(); // if not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(typeName)) { - if (!isValidLibrary(classPathMap.get(typeName))) { - log(typeArgumentToken, String.format(EXTERNAL_DEPENDENCY_ERROR, typeName)); - } + if (classPathMap.containsKey(typeName) && !isValidLibrary(classPathMap.get(typeName))) { + return typeName; } } + return null; } /** @@ -190,12 +229,6 @@ private void checkSingleTypeArgument(DetailAST typeArgumentToken) { * @return true if the class is a suppression class, otherwise, return false. */ private boolean isValidLibrary(String classPath) { - boolean isValidLibrary = false; - for (String lib : validLibrarySet) { - if (classPath.startsWith(lib)) { - isValidLibrary = true; - } - } - return isValidLibrary; + return VALID_DEPENDENCY_PACKAGE_NAMES.stream().anyMatch(validPackageName -> classPath.startsWith(validPackageName)); } } diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index 22659ac6f5c22..57d178dc081ac 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -7,6 +7,8 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import javax.xml.soap.Detail; + /** * Verify the classes with annotation @ServiceClient should have following rules: * <0l> @@ -58,7 +60,7 @@ public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: - hasServiceClientAnnotation = checkServiceClientAnnotation(token); + hasServiceClientAnnotation = hasServiceClientAnnotation(token); if (!hasServiceClientAnnotation) { notServiceClass = true; } @@ -87,14 +89,15 @@ public void visitToken(DetailAST token) { * @param token the CLASS_DEF AST node * @return true if the class is annotated with @ServiceClient, false otherwise. */ - private boolean checkServiceClientAnnotation(DetailAST token) { + private boolean hasServiceClientAnnotation(DetailAST token) { DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); if (modifiersToken != null) { - DetailAST annotationAST = modifiersToken.findFirstToken(TokenTypes.ANNOTATION); - if (annotationAST != null) { - DetailAST annotationIdent = annotationAST.findFirstToken(TokenTypes.IDENT); - if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { - return true; + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.ANNOTATION) { + DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { + return true; + } } } } From 5c9652e6608f39cc42a26879905ee6c2c2b7a340 Mon Sep 17 00:00:00 2001 From: shafang Date: Fri, 5 Jul 2019 14:56:39 -0700 Subject: [PATCH 05/35] Use CheckUtil to get modifier values --- .../checkstyle/checks/ExternalDependencyExposedCheck.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java index 72c16b488bea7..254086ab04ee9 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java @@ -7,6 +7,8 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.FullIdent; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import com.puppycrawl.tools.checkstyle.checks.naming.AccessModifier; +import com.puppycrawl.tools.checkstyle.utils.CheckUtil; import java.util.Arrays; import java.util.Collections; @@ -93,8 +95,8 @@ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { return; } - if (!modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) - && !modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED)) { + AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (!accessModifier.equals(AccessModifier.PUBLIC) && !accessModifier.equals(AccessModifier.PROTECTED)) { return; } From 74fc5835c31e22b4770ff43632116e5a71a5b4ef Mon Sep 17 00:00:00 2001 From: shafang Date: Fri, 5 Jul 2019 15:14:07 -0700 Subject: [PATCH 06/35] remove an extra useless boolean variable and remove unsued import --- .../checks/ServiceClientInstantiationCheck.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index 57d178dc081ac..cb7a24b5a7820 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -7,8 +7,6 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; -import javax.xml.soap.Detail; - /** * Verify the classes with annotation @ServiceClient should have following rules: * <0l> @@ -24,7 +22,6 @@ public class ServiceClientInstantiationCheck extends AbstractCheck { private static final String CLIENT = "Client"; private static boolean hasServiceClientAnnotation; - private static boolean notServiceClass; @Override public int[] getDefaultTokens() { @@ -49,21 +46,18 @@ public int[] getRequiredTokens() { @Override public void beginTree(DetailAST root) { - notServiceClass = false; + hasServiceClientAnnotation = true; } @Override public void visitToken(DetailAST token) { - if (notServiceClass) { + if (!hasServiceClientAnnotation) { return; } switch (token.getType()) { case TokenTypes.CLASS_DEF: hasServiceClientAnnotation = hasServiceClientAnnotation(token); - if (!hasServiceClientAnnotation) { - notServiceClass = true; - } break; case TokenTypes.CTOR_DEF: checkNonPublicOrProtectedConstructor(token); From a03a4a46e9e13b0b54e002d77059fffbcf237d80 Mon Sep 17 00:00:00 2001 From: shafang Date: Sat, 6 Jul 2019 22:08:57 -0700 Subject: [PATCH 07/35] refactor 2 --- .../ExternalDependencyExposedCheck.java | 108 ++++++++---------- .../ServiceClientInstantiationCheck.java | 29 ++--- 2 files changed, 65 insertions(+), 72 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java index 254086ab04ee9..a25c3597640b6 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java @@ -33,11 +33,11 @@ public class ExternalDependencyExposedCheck extends AbstractCheck { JAVA, COM_AZURE, REACTOR ))); - private Map classPathMap = new HashMap<>(); + private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); @Override public void beginTree(DetailAST rootAST) { - classPathMap.clear(); + simpleClassNameToQualifiedNameMap.clear(); } @Override @@ -81,7 +81,7 @@ public void visitToken(DetailAST token) { private void addImportedClassPath(DetailAST token) { final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); - classPathMap.put(className, importClassPath); + simpleClassNameToQualifiedNameMap.put(className, importClassPath); } /** @@ -109,7 +109,7 @@ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { DetailAST parametersToken = methodDefToken.findFirstToken(TokenTypes.PARAMETERS); if (parametersToken != null) { getInvalidParameterTypes(parametersToken).forEach( - (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName)));; + (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName))); } } @@ -121,17 +121,28 @@ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { */ private Map getInvalidReturnTypes(DetailAST typeToken) { Map invalidReturnTypeMap = new HashMap<>(); - final int typeCount = typeToken.getChildCount(); - // The TYPE node could has more than one child, such as Map - if (typeCount == 1) { - final String returnTypeName = typeToken.getFirstChild().getText(); - // if not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(returnTypeName) && !isValidLibrary(classPathMap.get(returnTypeName))) { - invalidReturnTypeMap.put(typeToken, returnTypeName); + + DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); + if (identToken != null) { + String typeName = identToken.getText(); + if (!isValidClassDependency(typeName)) { + invalidReturnTypeMap.put(typeToken, typeName); } - } else { - invalidReturnTypeMap = getInvalidTypeFromTypeArguments(typeToken); } + + final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS); + if (typeArgumentsToken != null) { + invalidReturnTypeMap.putAll(getInvalidTypeFromTypeArguments(typeArgumentsToken)); + } + + final DetailAST typeArgumentToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENT); + if (typeArgumentToken != null) { + String invalidTypeName = getInvalidTypeNameFromTypeArgument(typeArgumentToken); + if (invalidTypeName != null) { + invalidReturnTypeMap.put(typeArgumentToken, invalidTypeName); + } + } + return invalidReturnTypeMap; } @@ -155,46 +166,20 @@ private Map getInvalidParameterTypes(DetailAST parametersType * 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. * - * @param typeToken TYPE AST node + * @param typeArgumentsToken TYPE_ARGUMENTS AST node * @return a map that maps all the invalid TYPE_ARGUMENT node and the type name */ - private Map getInvalidTypeFromTypeArguments(DetailAST typeToken) { + private Map getInvalidTypeFromTypeArguments(DetailAST typeArgumentsToken) { final Map invalidTypesMap = new HashMap<>(); - - if (typeToken == null) { + if (typeArgumentsToken == null) { return invalidTypesMap; } - - final DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); - // if there is no IDENT token, implies the token is a default java type. - if (identToken == null) { - return invalidTypesMap; - } - - final String typeName = identToken.getText(); - // if does not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(typeName)) { - if (!isValidLibrary(classPathMap.get(typeName))) { - invalidTypesMap.put(typeToken, typeName); - } else { - // Checks multiple type arguments - final DetailAST typeArgumentsToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENTS); - if (typeArgumentsToken != null) { - for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { - String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast); - if (invalidTypeName != null) { - invalidTypesMap.put(ast, invalidTypeName); - } - } - } - } else { - // Checks single type argument - final DetailAST typeArgumentToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENT); - String invalidTypeName = getInvalidTypeNameFromTypeArgument(typeArgumentToken); - if (invalidTypeName != null) { - invalidTypesMap.put(typeArgumentToken, invalidTypeName); - } + // Checks multiple type arguments + for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { + String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast); + if (invalidTypeName != null) { + invalidTypesMap.put(ast, invalidTypeName); } } } @@ -214,23 +199,30 @@ 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) { - final String typeName = identToken.getText(); - // if not exist in the classPathMap, that implies the type is java default types, such as int. - if (classPathMap.containsKey(typeName) && !isValidLibrary(classPathMap.get(typeName))) { - return typeName; - } + if (identToken == null) { + return 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 classPath the full class path for a given class + * @param typeName the type name of class * @return true if the class is a suppression class, otherwise, return false. */ - private boolean isValidLibrary(String classPath) { - return VALID_DEPENDENCY_PACKAGE_NAMES.stream().anyMatch(validPackageName -> classPath.startsWith(validPackageName)); + private boolean isValidClassDependency(String typeName) { + // If the qualified class name does not exist in the map, + // it implies the type is a primitive Java type (ie. int, long, etc). + if (!simpleClassNameToQualifiedNameMap.containsKey(typeName)) { + return true; + } + + String qualifiedName = simpleClassNameToQualifiedNameMap.get(typeName); + return VALID_DEPENDENCY_PACKAGE_NAMES.stream() + .anyMatch(validPackageName -> qualifiedName.startsWith(validPackageName)); } } diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index cb7a24b5a7820..8c488611a5810 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -6,6 +6,8 @@ import com.puppycrawl.tools.checkstyle.api.AbstractCheck; import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import com.puppycrawl.tools.checkstyle.checks.naming.AccessModifier; +import com.puppycrawl.tools.checkstyle.utils.CheckUtil; /** * Verify the classes with annotation @ServiceClient should have following rules: @@ -78,7 +80,7 @@ public void visitToken(DetailAST token) { } /** - * Check if the class is annotated with @ServiceClient + * Checks if the class is annotated with @ServiceClient annotation * * @param token the CLASS_DEF AST node * @return true if the class is annotated with @ServiceClient, false otherwise. @@ -99,23 +101,21 @@ private boolean hasServiceClientAnnotation(DetailAST token) { } /** - * Check fo public or protected constructor for the service client class. + * Checks for public or protected constructor for the service client class. * Log error if the service client has public or protected constructor. * * @param token the CTOR_DEF AST node */ private void checkNonPublicOrProtectedConstructor(DetailAST token) { DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken != null) { - if (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) - || modifiersToken.branchContains(TokenTypes.LITERAL_PROTECTED)) { - log(modifiersToken, "Subclass of ServiceClient should not have any public or protected constructor."); - } + AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED)) { + log(modifiersToken, "Subclass of ServiceClient should not have any public or protected constructor."); } } - /** - * Check for public static method named 'builder'. Should avoid to use method name, 'builder'. + /**aa + * Checks for public static method named 'builder'. Should avoid to use method name, 'builder'. * * @param token the METHOD_DEF AST node */ @@ -123,7 +123,8 @@ private void checkNonPublicStaticBuilderMethodName(DetailAST token) { DetailAST methodNameToken = token.findFirstToken(TokenTypes.IDENT); if (methodNameToken != null && BUILDER.equals(methodNameToken.getText())) { DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken.branchContains(TokenTypes.LITERAL_PUBLIC) + AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (accessModifier.equals(AccessModifier.PUBLIC) && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { log(modifiersToken, "Subclass of ServiceClient should not have a public static method named ''builder''."); } @@ -131,16 +132,16 @@ private void checkNonPublicStaticBuilderMethodName(DetailAST token) { } /** - * Check for the variable field of the subclass of ServiceClient. + * Checks for the variable field of the subclass of ServiceClient. * These fields should be final because these classes supposed to be immutable class. * - * @param token the MODIFIERS AST node + * @param token the OBJBLOCK AST node */ private void checkClassFieldFinal(DetailAST token) { for (DetailAST ast = token.getFirstChild(); ast != null; ast = ast.getNextSibling()) { if (TokenTypes.VARIABLE_DEF == ast.getType()) { DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); - if (!modifiersToken.branchContains(TokenTypes.FINAL)) { + if (modifiersToken != null && !modifiersToken.branchContains(TokenTypes.FINAL)) { log(modifiersToken, "The variable field of the subclass of ServiceClient should be final. These classes supposed to be immutable."); } } @@ -148,7 +149,7 @@ private void checkClassFieldFinal(DetailAST token) { } /** - * Check for the class name of Service Client. It class should be named AsyncClient or Client. + * Checks for the class name of Service Client. It class should be named AsyncClient or Client. * * @param token the LITERAL_CLASS AST node */ From 6daf85c249c5e1a44244d34fe11393f700a31cdd Mon Sep 17 00:00:00 2001 From: shafang Date: Mon, 8 Jul 2019 17:16:43 -0700 Subject: [PATCH 08/35] updates based on J's feedback --- .../ExternalDependencyExposedCheck.java | 82 ++++----- .../ServiceClientInstantiationCheck.java | 159 ++++++++++++------ 2 files changed, 142 insertions(+), 99 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java index a25c3597640b6..31147c658ad56 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ExternalDependencyExposedCheck.java @@ -18,19 +18,14 @@ import java.util.Set; /** - * No external dependency exposed in the public API + * No external dependency exposed in public API */ public class ExternalDependencyExposedCheck extends AbstractCheck { - private static final String JAVA = "java"; - private static final String COM_AZURE = "com.azure"; - private static final String REACTOR = "reactor"; - private static final String DOT = "."; - private static final String EXTERNAL_DEPENDENCY_ERROR = - "Class ''%s'', is a class from external dependency. You should not use it as a return or parameter argument type"; + "Class ''%s'', is a class from external dependency. You should not use it as a return or method argument type."; private static final Set VALID_DEPENDENCY_PACKAGE_NAMES = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( - JAVA, COM_AZURE, REACTOR + "java", "com.azure", "reactor" ))); private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); @@ -62,7 +57,10 @@ public int[] getRequiredTokens() { public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.IMPORT: - addImportedClassPath(token); + // Add all imported classes into a map, key is the name of class and value is the full package path of class. + final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); + final String className = importClassPath.substring(importClassPath.lastIndexOf(".") + 1); + simpleClassNameToQualifiedNameMap.put(className, importClassPath); break; case TokenTypes.METHOD_DEF: checkNoExternalDependencyExposed(token); @@ -73,17 +71,6 @@ public void visitToken(DetailAST token) { } } - /** - * Add all imported classes into a map, key is the name of class and value is the full package path of class. - * - * @param token the IMPORT AST node - */ - private void addImportedClassPath(DetailAST token) { - final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); - final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); - simpleClassNameToQualifiedNameMap.put(className, importClassPath); - } - /** * Checks for external dependency, log the error if it is an invalid external dependency. * @@ -91,22 +78,23 @@ private void addImportedClassPath(DetailAST token) { */ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken == null) { - return; - } - AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + // Getting the modifier of the method to determine if it is 'public' or 'protected'. + // Ignore the check if it is neither of 'public' nor 'protected', + final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); if (!accessModifier.equals(AccessModifier.PUBLIC) && !accessModifier.equals(AccessModifier.PROTECTED)) { return; } - DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); + // Checks for the return type of method + final DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); if (typeToken != null) { getInvalidReturnTypes(typeToken).forEach( (token, returnTypeName) -> log(token, String.format(EXTERNAL_DEPENDENCY_ERROR, returnTypeName))); } - DetailAST parametersToken = methodDefToken.findFirstToken(TokenTypes.PARAMETERS); + // 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))); @@ -120,24 +108,28 @@ private void checkNoExternalDependencyExposed(DetailAST methodDefToken) { * @return a map that maps the invalid TYPE node and the type name. */ private Map getInvalidReturnTypes(DetailAST typeToken) { - Map invalidReturnTypeMap = new HashMap<>(); + final Map invalidReturnTypeMap = new HashMap<>(); - DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); - if (identToken != null) { - String typeName = identToken.getText(); - if (!isValidClassDependency(typeName)) { - invalidReturnTypeMap.put(typeToken, typeName); - } + // Add all invalid external return types to the map + final DetailAST identToken = typeToken.findFirstToken(TokenTypes.IDENT); + if (identToken == null) { + return invalidReturnTypeMap; + } + final String typeName = identToken.getText(); + if (!isValidClassDependency(typeName)) { + invalidReturnTypeMap.put(typeToken, typeName); } + // 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)); } + // TYPE_ARGUMENT, add the invalid external type to the map final DetailAST typeArgumentToken = typeToken.findFirstToken(TokenTypes.TYPE_ARGUMENT); if (typeArgumentToken != null) { - String invalidTypeName = getInvalidTypeNameFromTypeArgument(typeArgumentToken); + final String invalidTypeName = getInvalidTypeNameFromTypeArgument(typeArgumentToken); if (invalidTypeName != null) { invalidReturnTypeMap.put(typeArgumentToken, invalidTypeName); } @@ -176,27 +168,25 @@ private Map getInvalidTypeFromTypeArguments(DetailAST typeArg } // Checks multiple type arguments for (DetailAST ast = typeArgumentsToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.TYPE_ARGUMENT) { - String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast); - if (invalidTypeName != null) { - invalidTypesMap.put(ast, invalidTypeName); - } + if (ast.getType() != TokenTypes.TYPE_ARGUMENT) { + continue; + } + + final String invalidTypeName = getInvalidTypeNameFromTypeArgument(ast); + if (invalidTypeName != null) { + invalidTypesMap.put(ast, invalidTypeName); } } return invalidTypesMap; } /** - * Get invalid type name. + * 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) { - if (typeArgumentToken == null) { - return null; - } - final DetailAST identToken = typeArgumentToken.findFirstToken(TokenTypes.IDENT); // if there is no IDENT token, implies the token is default java types. if (identToken == null) { @@ -209,7 +199,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 + * 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. @@ -221,7 +211,7 @@ private boolean isValidClassDependency(String typeName) { return true; } - String qualifiedName = simpleClassNameToQualifiedNameMap.get(typeName); + final String qualifiedName = simpleClassNameToQualifiedNameMap.get(typeName); return VALID_DEPENDENCY_PACKAGE_NAMES.stream() .anyMatch(validPackageName -> qualifiedName.startsWith(validPackageName)); } diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index 8c488611a5810..cc5695ec0e9b7 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -22,8 +22,10 @@ public class ServiceClientInstantiationCheck extends AbstractCheck { private static final String BUILDER = "builder"; private static final String ASYNC_CLIENT ="AsyncClient"; private static final String CLIENT = "Client"; + private static final String IS_ASYNC = "isAsync"; private static boolean hasServiceClientAnnotation; + private static boolean isAsync; @Override public int[] getDefaultTokens() { @@ -41,7 +43,6 @@ public int[] getRequiredTokens() { TokenTypes.CLASS_DEF, TokenTypes.CTOR_DEF, TokenTypes.METHOD_DEF, - TokenTypes.LITERAL_CLASS, TokenTypes.OBJBLOCK }; } @@ -49,6 +50,7 @@ public int[] getRequiredTokens() { @Override public void beginTree(DetailAST root) { hasServiceClientAnnotation = true; + isAsync = false; } @Override @@ -60,18 +62,16 @@ public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: hasServiceClientAnnotation = hasServiceClientAnnotation(token); + checkServiceClientNaming(token); break; case TokenTypes.CTOR_DEF: - checkNonPublicOrProtectedConstructor(token); + checkConstructor(token); break; case TokenTypes.METHOD_DEF: - checkNonPublicStaticBuilderMethodName(token); - break; - case TokenTypes.LITERAL_CLASS: - checkServiceClientNaming(token); + checkMethodName(token); break; case TokenTypes.OBJBLOCK: - checkClassFieldFinal(token); + checkClassField(token); break; default: // Checkstyle complains if there's no default block in switch @@ -80,23 +80,27 @@ public void visitToken(DetailAST token) { } /** - * Checks if the class is annotated with @ServiceClient annotation + * Checks if the class is annotated with annotation @ServiceClient. A class could have multiple annotations. * - * @param token the CLASS_DEF AST node + * @param classDefToken the CLASS_DEF AST node * @return true if the class is annotated with @ServiceClient, false otherwise. */ - private boolean hasServiceClientAnnotation(DetailAST token) { - DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken != null) { - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.ANNOTATION) { - DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); - if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { - return true; - } - } + private boolean hasServiceClientAnnotation(DetailAST classDefToken) { + // Always has MODIFIERS node + final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); + + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + // One class could have multiple annotations, return true if found one. + final DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { + isAsync = isAsyncServiceClient(ast); + return true; } } + // If no @ServiceClient annotated with this class, return false return false; } @@ -104,30 +108,33 @@ private boolean hasServiceClientAnnotation(DetailAST token) { * Checks for public or protected constructor for the service client class. * Log error if the service client has public or protected constructor. * - * @param token the CTOR_DEF AST node + * @param ctorToken the CTOR_DEF AST node */ - private void checkNonPublicOrProtectedConstructor(DetailAST token) { - DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + private void checkConstructor(DetailAST ctorToken) { + final DetailAST modifiersToken = ctorToken.findFirstToken(TokenTypes.MODIFIERS); + // find constructor's modifier accessibility, no public or protected constructor + final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); if (accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED)) { - log(modifiersToken, "Subclass of ServiceClient should not have any public or protected constructor."); + log(modifiersToken, "@ServiceClient class should not have any public or protected constructor."); } } - /**aa + /** * Checks for public static method named 'builder'. Should avoid to use method name, 'builder'. * - * @param token the METHOD_DEF AST node + * @param methodDefToken the METHOD_DEF AST node */ - private void checkNonPublicStaticBuilderMethodName(DetailAST token) { - DetailAST methodNameToken = token.findFirstToken(TokenTypes.IDENT); - if (methodNameToken != null && BUILDER.equals(methodNameToken.getText())) { - DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); - if (accessModifier.equals(AccessModifier.PUBLIC) - && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { - log(modifiersToken, "Subclass of ServiceClient should not have a public static method named ''builder''."); - } + private void checkMethodName(DetailAST methodDefToken) { + final DetailAST methodNameToken = methodDefToken.findFirstToken(TokenTypes.IDENT); + if (!BUILDER.equals(methodNameToken.getText())) { + return; + } + + final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + // find method's modifier accessibility, should not have a public static method called 'builder' + final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (accessModifier.equals(AccessModifier.PUBLIC) && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { + log(modifiersToken, "@ServiceClient class should not have a public static method named ''builder''."); } } @@ -135,33 +142,79 @@ private void checkNonPublicStaticBuilderMethodName(DetailAST token) { * Checks for the variable field of the subclass of ServiceClient. * These fields should be final because these classes supposed to be immutable class. * - * @param token the OBJBLOCK AST node + * @param objBlockToken the OBJBLOCK AST node */ - private void checkClassFieldFinal(DetailAST token) { - for (DetailAST ast = token.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (TokenTypes.VARIABLE_DEF == ast.getType()) { - DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken != null && !modifiersToken.branchContains(TokenTypes.FINAL)) { - log(modifiersToken, "The variable field of the subclass of ServiceClient should be final. These classes supposed to be immutable."); - } + private void checkClassField(DetailAST objBlockToken) { + for (DetailAST ast = objBlockToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (TokenTypes.VARIABLE_DEF != ast.getType()) { + continue; + } + final DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); + //VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under + // MODIFIERS token + if (!modifiersToken.branchContains(TokenTypes.FINAL)) { + log(modifiersToken, String.format("The variable field ''%s'' of @ServiceClient should be final. The class annotated with @ServiceClient supposed to be immutable.", + ast.findFirstToken(TokenTypes.IDENT).getText())); } } } /** - * Checks for the class name of Service Client. It class should be named AsyncClient or Client. + * Checks for the class name of Service Client. It should be named AsyncClient or Client. * - * @param token the LITERAL_CLASS AST node + * @param classDefToken the CLASS_DEF AST node */ - private void checkServiceClientNaming(DetailAST token) { - for (DetailAST ast = token; ast != null; ast = ast.getNextSibling()) { - if (TokenTypes.IDENT == ast.getType()) { - String className = ast.getText(); - if (!className.endsWith(ASYNC_CLIENT) && !className.endsWith(CLIENT)) { - log(ast, String.format("Class name %s should named AsyncClient or Client.", className)); - } - break; + private void checkServiceClientNaming(DetailAST classDefToken) { + if (!hasServiceClientAnnotation) { + return; + } + + final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); + // Async service client + if (isAsync && !className.endsWith(ASYNC_CLIENT)) { + log(classDefToken, String.format("Async class ''%s'' should named AsyncClient ", className)); + } + // Sync service client + if (!isAsync && !className.endsWith(CLIENT)) { + log(classDefToken, String.format("Sync class %s should named Client.", className)); + } + } + + /** + * A function checks if the annotation node has a member key is {@code IS_ASYNC} with value equals to 'true'. + * If the value equals 'true', which indicates the @ServiceClient is an asynchronous client. + * If the member pair is missing. By default, it is a synchronous service client. + * + * @param annotationToken the ANNOTATION AST node + * @return true if the annotation has {@code IS_ASYNC} value 'true', otherwise, false. + */ + private boolean isAsyncServiceClient(DetailAST annotationToken) { + for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + continue; } + + // skip this annotation member value pair if no IDENT found, since we are looking for member, 'isAsync'. + final DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); + if (identToken == null) { + continue; + } + + // skip this annotation member value pair if the member is not 'isAsync'. + if (!IS_ASYNC.equals(identToken.getText())) { + continue; + } + + // skip this annotation member value pair if the member has no EXPR value + final DetailAST exprToken = ast.findFirstToken(TokenTypes.EXPR); + if (exprToken == null) { + continue; + } + + // true if isAsync = true, false otherwise. + return exprToken.branchContains(TokenTypes.LITERAL_TRUE); } + // By default, if the IS_ASYNC doesn't exist, the service client is a synchronous client. + return false; } } From d4b2e37cf6bd9a5a657456b1753c08e0114c56e4 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 9 Jul 2019 09:49:06 -0700 Subject: [PATCH 09/35] revise of wording --- .../checks/ServiceClientInstantiationCheck.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index cc5695ec0e9b7..95942e86dcdb9 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -150,11 +150,11 @@ private void checkClassField(DetailAST objBlockToken) { continue; } final DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); - //VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under - // MODIFIERS token + // VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under + // MODIFIERS token. Also the previous sibling of OBJBLOCK will always be class name IDENT node. if (!modifiersToken.branchContains(TokenTypes.FINAL)) { - log(modifiersToken, String.format("The variable field ''%s'' of @ServiceClient should be final. The class annotated with @ServiceClient supposed to be immutable.", - ast.findFirstToken(TokenTypes.IDENT).getText())); + log(modifiersToken, String.format("The variable field ''%s'' of class ''%s'' should be final. Classes annotated with @ServiceClient are supposed to be immutable.", + ast.findFirstToken(TokenTypes.IDENT).getText(), objBlockToken.getPreviousSibling().getText())); } } } @@ -172,11 +172,11 @@ private void checkServiceClientNaming(DetailAST classDefToken) { final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); // Async service client if (isAsync && !className.endsWith(ASYNC_CLIENT)) { - log(classDefToken, String.format("Async class ''%s'' should named AsyncClient ", className)); + log(classDefToken, String.format("Async class ''%s'' must be named AsyncClient ", className)); } // Sync service client if (!isAsync && !className.endsWith(CLIENT)) { - log(classDefToken, String.format("Sync class %s should named Client.", className)); + log(classDefToken, String.format("Sync class %s must be named Client.", className)); } } From 664a9509f8c54ec0048e74c52373e5b8ee1280c3 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 9 Jul 2019 10:13:17 -0700 Subject: [PATCH 10/35] draft version of rule 5, 6, 7 --- .../checks/ServiceClientBuilderCheck.java | 156 ++++++++++ .../checks/ServiceClientMethodCheck.java | 267 ++++++++++++++++++ .../checks/ServiceInterfaceCheck.java | 100 +++++++ .../main/resources/checkstyle/checkstyle.xml | 15 + 4 files changed, 538 insertions(+) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java new file mode 100644 index 0000000000000..61e156af35eaa --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -0,0 +1,156 @@ +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.TokenTypes; + +public class ServiceClientBuilderCheck extends AbstractCheck { + private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; + private static final String CLIENT_BUILDER = "ClientBuilder"; + private static final String SERVICE_CLIENT_PROPERTY_NAME = "serviceClients"; + private static final String BUILD_CLIENT = "buildClient"; + private static final String BUILD_ASYNC_CLIENT = "buildAsyncClient"; + + private static boolean hasServiceClientBuilderAnnotation; + private static boolean hasAsyncClientBuilder; + private static boolean hasClientBuilder; + + @Override + public void beginTree(DetailAST root) { + hasServiceClientBuilderAnnotation = true; + hasAsyncClientBuilder = false; + hasClientBuilder = false; + } + + @Override + public void finishTree(DetailAST root) { + if (hasServiceClientBuilderAnnotation) { + if (!hasAsyncClientBuilder) { + log(root, String.format("ServiceClientBuilder missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); + } + if (!hasClientBuilder) { + log(root, String.format("ServiceClientBuilder missing an synchronous method, ''%s''", BUILD_CLIENT)); + } + } + } + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.IMPORT, + TokenTypes.METHOD_DEF + }; + } + + @Override + public void visitToken(DetailAST token) { + if (!hasServiceClientBuilderAnnotation) { + return; + } + + switch (token.getType()) { + case TokenTypes.CLASS_DEF: + DetailAST annotationToken = getServiceClientAnnotation(token); + if (annotationToken != null) { + hasServiceClientBuilderAnnotation = true; + checkAnnotationMemberValuePair(annotationToken); + } else { + hasServiceClientBuilderAnnotation = false; + } + checkForClassNamingAndAnnotation(token); + break; + case TokenTypes.METHOD_DEF: + checkBuilderMethods(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + + /** + * Checks if the class is annotated with @ServiceClientBuilder + * + * @param classDefToken the CLASS_DEF AST node + * @return true if the class is annotated with @ServiceClientBuilder, false otherwise. + */ + private DetailAST getServiceClientAnnotation(DetailAST classDefToken) { + DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken != null) { + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.ANNOTATION) { + DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT_BUILDER.equals(annotationIdent.getText())) { + return ast; + } + } + } + } + return null; + } + + private void checkForClassNamingAndAnnotation(DetailAST classDefToken) { + String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); + if (!className.endsWith(CLIENT_BUILDER)) { + log(classDefToken, "Service client builder class should be named ClientBuilder."); + } else { + if (!hasServiceClientBuilderAnnotation) { + log(classDefToken, String.format("Class ''%s'' should be annotated with @ServiceClientBuilder.", className)); + } + } + } + + /** + * Checks if the {@code ServiceClientBuilder} annotation has a service client prop named 'serviceClients'. + * + * @param annotationToken the ANNOTATION AST node + */ + private void checkAnnotationMemberValuePair(DetailAST annotationToken) { + boolean hasServiceClientPropName = false; + + for (DetailAST ast = annotationToken.getNextSibling(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + DetailAST identAST = ast.findFirstToken(TokenTypes.IDENT); + if (identAST != null) { + String identName = identAST.getText(); + if (identName.equals(SERVICE_CLIENT_PROPERTY_NAME)) { + hasServiceClientPropName = true; + break; + } + } + } + } + + if (!hasServiceClientPropName) { + log(annotationToken, String.format( + "Annotation @%s should have ''serviceClients'' as property of annotation and should list all of the service clients it can build.", + SERVICE_CLIENT_BUILDER)); + } + } + + /** + * Every service client builder should have a sync client builder and async client builder, + * buildClient() and buildAsyncClient(), respectively. + * + * @param methodDefToken METHOD_DEF AST node + */ + private void checkBuilderMethods(DetailAST methodDefToken) { + String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); + if (methodName.equals(BUILD_ASYNC_CLIENT)) { + hasAsyncClientBuilder = true; + } else if (methodName.equals(BUILD_CLIENT)) { + hasClientBuilder = true; + } + } +} diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java new file mode 100644 index 0000000000000..76594c741d484 --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java @@ -0,0 +1,267 @@ +// 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.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Stream; + +/** + * Service Client Method Checks. All methods that has a @ServiceMethod annotation in a class annotated with + * @ServiceClient should follow below rules: + * + * (1) Method naming pattern. Refer to Java Spec: + * (2) Methods should not have "Async" added to the method name + * (3) Return type of async and sync clients should be as per guidelines: + * (a) Return type for async collection should be of type? extends Flux + * (b) Return type for async single value should be of type? extends Mono + * (c) Return type for sync collection should be of type? extends Stream + * (d) Return type for sync single value should be of type? extends Response + */ +public class ServiceClientMethodCheck extends AbstractCheck { + private static final String DOT = "."; + private static final String SERVICE_CLIENT = "ServiceClient"; + private static final String SERVICE_METHOD = "ServiceMethod"; + private static final String ASYNC = "Async"; + private static final String RETURNS = "returns"; + private static final String SINGLE_RETURN_TYPE = "ReturnType.SINGLE"; + private static final String COLLECTION_RETURN_TYPE = "ReturnType.COLLECTION"; + + private static final String FLUX = "reactor.core.publisher.Flux"; + private static final String MONO = "reactor.core.publisher.Mono"; + private static final String RESPONSE = "com.azure.core.http.rest.response"; + + private static final String FAILED_TO_LOAD_MESSAGE = "%s class failed to load, ServiceClientChecks will be ignored."; + private static final String SINGLE_VALUE_RETURN_ERROR = "%s should either be a ''Mono'' class or class extends it if returns an ''async'' single value, " + + "or a ''Response'' class or class extends it if returns a ''sync'' single value."; + private static final String COLLECTION_RETURN_ERROR = "%s should either be a ''Flux'' class or class extends it if returns an ''async'' collection, " + + "or a ''Stream'' class or class extends it if returns a ''sync'' collection."; + + private static final Set COMMON_NAMING_PREFIX_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( + "upsert", "set", "create", "update", "replace", "delete", "add", "get", "list" + ))); + + private static final String COMMON_NAMING_SUFFIX = "Exists"; + + private static boolean hasServiceClientAnnotation; + private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); + + Class monoObj; + Class fluxObj; + Class responseObj; + + @Override + public void init() { + try { + fluxObj = Class.forName(FLUX); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, FLUX)); + } + + try { + monoObj = Class.forName(MONO); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, MONO)); + } + + try { + responseObj = Class.forName(RESPONSE); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, RESPONSE)); + } + } + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.IMPORT, + TokenTypes.CLASS_DEF, + TokenTypes.METHOD_DEF + }; + } + + @Override + public void beginTree(DetailAST root) { + hasServiceClientAnnotation = true; + } + + @Override + public void visitToken(DetailAST token) { + if (!hasServiceClientAnnotation) { + return; + } + + switch (token.getType()) { + case TokenTypes.IMPORT: + addImportedClassPath(token); + break; + case TokenTypes.CLASS_DEF: + hasServiceClientAnnotation = hasServiceClientAnnotation(token); + break; + case TokenTypes.METHOD_DEF: + checkMethodNamingPattern(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + /** + * Add all imported classes into a map, key is the name of class and value is the full package path of class. + * + * @param token the IMPORT AST node + */ + private void addImportedClassPath(DetailAST token) { + final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); + final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); + simpleClassNameToQualifiedNameMap.put(className, importClassPath); + } + + /** + * Checks if the class is annotated with @ServiceClient annotation + * + * @param token the CLASS_DEF AST node + * @return true if the class is annotated with @ServiceClient, false otherwise. + */ + private boolean hasServiceClientAnnotation(DetailAST token) { + DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken != null) { + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() == TokenTypes.ANNOTATION) { + DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { + return true; + } + } + } + } + return false; + } + + private void checkMethodNamingPattern(DetailAST methodDefToken) { + DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken == null) { + return; + } + DetailAST serviceMethodAnnotation = hasServiceMethodAnnotation(modifiersToken); + if (serviceMethodAnnotation == null) { + return; + } + + DetailAST methodNameToken = methodDefToken.findFirstToken(TokenTypes.IDENT); + if (methodNameToken == null) { + return; + } + String methodName = methodNameToken.getText(); + if (methodName.contains(ASYNC)) { + log(methodNameToken, String.format("Method name ''%s'' should not contain ''%s'' in the method name", + methodName, ASYNC)); + } + + if (!isCommonNamingPattern(methodName)) { + log(methodDefToken, String.format("Method name ''%s'' should follow a common vocabulary. Refer to Java Spec. ", methodName)); + } + + DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); + if (typeToken == null) { + log(typeToken, "Missing return type"); + return; + } + + String returnTypeAnnotation = getReturnTypeFromAnnotation(serviceMethodAnnotation); + String returnType = methodDefToken.findFirstToken(TokenTypes.TYPE).getText(); + + if (!simpleClassNameToQualifiedNameMap.containsKey(returnType)) { + if (SINGLE_RETURN_TYPE.equals(returnTypeAnnotation)) { + log(typeToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); + } else if (COLLECTION_RETURN_TYPE.equals(returnTypeAnnotation)) { + log(typeToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); + } + } + + String qualifiedReturnName = simpleClassNameToQualifiedNameMap.get(returnType); + Class qualifiedReturnTypeInstance; + try { + qualifiedReturnTypeInstance = Class.forName(qualifiedReturnName); + } catch (ClassNotFoundException ex) { + log(methodDefToken, String.format(FAILED_TO_LOAD_MESSAGE, qualifiedReturnName)); + return; + } + + if (SINGLE_RETURN_TYPE.equals(returnTypeAnnotation)) { + if (!qualifiedReturnTypeInstance.isInstance(monoObj) + && !qualifiedReturnTypeInstance.isInstance(responseObj)) { + log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); + } + } else if (COLLECTION_RETURN_TYPE.equals(returnTypeAnnotation)) { + if (!qualifiedReturnTypeInstance.isInstance(fluxObj) + && !qualifiedReturnTypeInstance.isInstance(Stream.class)) { + log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); + } + } else { + log(serviceMethodAnnotation, String.format("''returns'' value = ''%s'' is neither SINGLE nor COLLECTION return type.", returnTypeAnnotation)); + } + } + + + private String getReturnTypeFromAnnotation(DetailAST annotationMemberValuePairToken) { + + DetailAST identToken = annotationMemberValuePairToken.findFirstToken(TokenTypes.IDENT); + if (identToken == null || !RETURNS.equals(identToken.getText())) { + return null; + } + + String returnType = FullIdent.createFullIdentBelow(annotationMemberValuePairToken.findFirstToken(TokenTypes.EXPR)).getText(); + if (returnType != null && !returnType.isEmpty()) { + return returnType; + } + + return null; + } + + private DetailAST hasServiceMethodAnnotation(DetailAST modifiersToken) { + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + + DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); + if (identToken == null || !SERVICE_METHOD.equals(identToken.getText())) { + continue; + } + return ast; + } + + return null; + } + + private boolean isCommonNamingPattern(String methodName) { + boolean isCommonNamingPattern = COMMON_NAMING_PREFIX_SET.stream().anyMatch(commonName -> + methodName.startsWith(commonName)); + if (!isCommonNamingPattern) { + isCommonNamingPattern = methodName.endsWith(COMMON_NAMING_SUFFIX); + } + return isCommonNamingPattern; + } +} diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java new file mode 100644 index 0000000000000..e042561ec9f4f --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -0,0 +1,100 @@ +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.TokenTypes; + +public class ServiceInterfaceCheck extends AbstractCheck { + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.INTERFACE_DEF + }; + } + + @Override + public void visitToken(DetailAST token) { + switch (token.getType()) { + case TokenTypes.INTERFACE_DEF: + checkServiceInterface(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + private void checkServiceInterface(DetailAST interfaceDefToken) { + DetailAST modifiersToken = interfaceDefToken.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken == null) { + return; + } + + DetailAST serviceInterfaceAnnotationNode = null; + String nameValue = null; + + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + + for (DetailAST annotationChild = ast.getFirstChild(); + annotationChild != null; annotationChild = annotationChild.getNextSibling()) { + + if (annotationChild.getType() == TokenTypes.IDENT) { + if (!"ServiceInterface".equals(annotationChild.getText())) { + break; + } else { + serviceInterfaceAnnotationNode = ast; + } + } + + if (annotationChild.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + DetailAST annotationPairIdent = annotationChild.findFirstToken(TokenTypes.IDENT); + if (annotationPairIdent != null && "name".equals(annotationPairIdent.getText())) { + nameValue = getNamePropertyValue(annotationChild.findFirstToken(TokenTypes.EXPR)); + } + } + } + } + + + if (serviceInterfaceAnnotationNode != null) { + if(nameValue == null) { + log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); + } else { + if (nameValue.contains(" ")) { + log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not contain white space."); + } + if (nameValue.length() > 10) { + log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not have a length > 10."); + } + } + } + } + + private String getNamePropertyValue(DetailAST exprToken) { + if (exprToken == null) { + return null; + } + + DetailAST nameValueToken = exprToken.findFirstToken(TokenTypes.STRING_LITERAL); + if (nameValueToken == null) { + return null; + } + + return nameValueToken.getText(); + } +} 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 0e37ecfac62cf..4b11903d79ecc 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -290,5 +290,20 @@ page at http://checkstyle.sourceforge.net/config.html --> We should only return types, and accept argument types, that are from the com.azure namespace. All other types should have suppression added if required. --> + + + + + + + + From 6e9ca658b05d12fbba1c6e2d4605b25be530b883 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 9 Jul 2019 10:45:18 -0700 Subject: [PATCH 11/35] refactor 1 --- .../checks/ServiceClientBuilderCheck.java | 50 +++++++++---------- .../checks/ServiceInterfaceCheck.java | 31 +++++++----- 2 files changed, 42 insertions(+), 39 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index 61e156af35eaa..a8c6e6cb5ec52 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -6,8 +6,6 @@ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; - private static final String CLIENT_BUILDER = "ClientBuilder"; - private static final String SERVICE_CLIENT_PROPERTY_NAME = "serviceClients"; private static final String BUILD_CLIENT = "buildClient"; private static final String BUILD_ASYNC_CLIENT = "buildAsyncClient"; @@ -60,7 +58,7 @@ public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: - DetailAST annotationToken = getServiceClientAnnotation(token); + final DetailAST annotationToken = getServiceClientAnnotation(token); if (annotationToken != null) { hasServiceClientBuilderAnnotation = true; checkAnnotationMemberValuePair(annotationToken); @@ -78,7 +76,6 @@ public void visitToken(DetailAST token) { } } - /** * Checks if the class is annotated with @ServiceClientBuilder * @@ -86,23 +83,21 @@ public void visitToken(DetailAST token) { * @return true if the class is annotated with @ServiceClientBuilder, false otherwise. */ private DetailAST getServiceClientAnnotation(DetailAST classDefToken) { - DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken != null) { - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.ANNOTATION) { - DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); - if (annotationIdent != null && SERVICE_CLIENT_BUILDER.equals(annotationIdent.getText())) { - return ast; - } - } + final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + if (SERVICE_CLIENT_BUILDER.equals(ast.findFirstToken(TokenTypes.IDENT).getText())) { + return ast; } } return null; } private void checkForClassNamingAndAnnotation(DetailAST classDefToken) { - String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); - if (!className.endsWith(CLIENT_BUILDER)) { + final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); + if (!className.endsWith("ClientBuilder")) { log(classDefToken, "Service client builder class should be named ClientBuilder."); } else { if (!hasServiceClientBuilderAnnotation) { @@ -119,16 +114,19 @@ private void checkForClassNamingAndAnnotation(DetailAST classDefToken) { private void checkAnnotationMemberValuePair(DetailAST annotationToken) { boolean hasServiceClientPropName = false; - for (DetailAST ast = annotationToken.getNextSibling(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - DetailAST identAST = ast.findFirstToken(TokenTypes.IDENT); - if (identAST != null) { - String identName = identAST.getText(); - if (identName.equals(SERVICE_CLIENT_PROPERTY_NAME)) { - hasServiceClientPropName = true; - break; - } - } + for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + continue; + } + + DetailAST identAST = ast.findFirstToken(TokenTypes.IDENT); + if (identAST == null) { + continue; + } + + if ("serviceClients".equals(identAST.getText())) { + hasServiceClientPropName = true; + break; } } @@ -146,7 +144,7 @@ private void checkAnnotationMemberValuePair(DetailAST annotationToken) { * @param methodDefToken METHOD_DEF AST node */ private void checkBuilderMethods(DetailAST methodDefToken) { - String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); + final String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); if (methodName.equals(BUILD_ASYNC_CLIENT)) { hasAsyncClientBuilder = true; } else if (methodName.equals(BUILD_CLIENT)) { diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index e042561ec9f4f..a98fbc66c8058 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -35,23 +35,23 @@ public void visitToken(DetailAST token) { } } + /** + * + * @param interfaceDefToken + */ private void checkServiceInterface(DetailAST interfaceDefToken) { - DetailAST modifiersToken = interfaceDefToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken == null) { - return; - } - DetailAST serviceInterfaceAnnotationNode = null; String nameValue = null; + DetailAST modifiersToken = interfaceDefToken.findFirstToken(TokenTypes.MODIFIERS); for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - + // we care about only the ANNOTATION type if (ast.getType() != TokenTypes.ANNOTATION) { continue; } - - for (DetailAST annotationChild = ast.getFirstChild(); - annotationChild != null; annotationChild = annotationChild.getNextSibling()) { + // find the first + for (DetailAST annotationChild = ast.getFirstChild(); annotationChild != null; + annotationChild = annotationChild.getNextSibling()) { if (annotationChild.getType() == TokenTypes.IDENT) { if (!"ServiceInterface".equals(annotationChild.getText())) { @@ -62,15 +62,15 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } if (annotationChild.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - DetailAST annotationPairIdent = annotationChild.findFirstToken(TokenTypes.IDENT); + final DetailAST annotationPairIdent = annotationChild.findFirstToken(TokenTypes.IDENT); if (annotationPairIdent != null && "name".equals(annotationPairIdent.getText())) { nameValue = getNamePropertyValue(annotationChild.findFirstToken(TokenTypes.EXPR)); } } } } - - + + // if if (serviceInterfaceAnnotationNode != null) { if(nameValue == null) { log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); @@ -85,12 +85,17 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } } + /** + * + * @param exprToken + * @return + */ private String getNamePropertyValue(DetailAST exprToken) { if (exprToken == null) { return null; } - DetailAST nameValueToken = exprToken.findFirstToken(TokenTypes.STRING_LITERAL); + final DetailAST nameValueToken = exprToken.findFirstToken(TokenTypes.STRING_LITERAL); if (nameValueToken == null) { return null; } From 183222a65b2a444d97cec8f37d4c781fe2c0c139 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 9 Jul 2019 14:56:55 -0700 Subject: [PATCH 12/35] refactor ServiceInterface annotation check --- .../checks/ServiceInterfaceCheck.java | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index a98fbc66c8058..3eb3dd92dddec 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -4,6 +4,11 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +/** + * The @ServiceInterface class should have following rules: + * 1) has annotation property 'name' and should be non-empty + * 2) length of value of property 'name' should be less than 10 characters and without space + */ public class ServiceInterfaceCheck extends AbstractCheck { @Override @@ -36,23 +41,26 @@ public void visitToken(DetailAST token) { } /** + * Checks for @ServiceInterface rules that defined at the beginning of class definition. * - * @param interfaceDefToken + * @param interfaceDefToken INTERFACE_DEF AST node */ private void checkServiceInterface(DetailAST interfaceDefToken) { DetailAST serviceInterfaceAnnotationNode = null; String nameValue = null; DetailAST modifiersToken = interfaceDefToken.findFirstToken(TokenTypes.MODIFIERS); + // Find the @ServiceInterface and the property 'name' and the corresponding value for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - // we care about only the ANNOTATION type + // We care about only the ANNOTATION type if (ast.getType() != TokenTypes.ANNOTATION) { continue; } - // find the first + for (DetailAST annotationChild = ast.getFirstChild(); annotationChild != null; annotationChild = annotationChild.getNextSibling()) { + // IDENT if (annotationChild.getType() == TokenTypes.IDENT) { if (!"ServiceInterface".equals(annotationChild.getText())) { break; @@ -61,34 +69,40 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } } + // ANNOTATION_MEMBER_VALUE_PAIR if (annotationChild.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - final DetailAST annotationPairIdent = annotationChild.findFirstToken(TokenTypes.IDENT); - if (annotationPairIdent != null && "name".equals(annotationPairIdent.getText())) { + if ("name".equals(annotationChild.findFirstToken(TokenTypes.IDENT).getText())) { nameValue = getNamePropertyValue(annotationChild.findFirstToken(TokenTypes.EXPR)); } } } } - // if - if (serviceInterfaceAnnotationNode != null) { - if(nameValue == null) { - log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); - } else { - if (nameValue.contains(" ")) { - log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not contain white space."); - } - if (nameValue.length() > 10) { - log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not have a length > 10."); - } + // No @ServiceInterface annotation found + if (serviceInterfaceAnnotationNode == null) { + return; + } + + // Checks the rules: + // Missing 'name' property + if(nameValue == null) { + log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); + } else { + // No Space allowed + if (nameValue.contains(" ")) { + log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not contain white space."); + } + // Length should less than or equal to 10 characters + if (nameValue.length() > 10) { + log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not have a length > 10."); } } } /** - * - * @param exprToken - * @return + * Get the name property value from the EXPR node + * @param exprToken EXPR + * @return null if EXPR node doesn't exist or no STRING_LITERAL. Otherwise, returns the value of the property. */ private String getNamePropertyValue(DetailAST exprToken) { if (exprToken == null) { From 9fa4a164ea780b4b94d226a741b895b33fd07711 Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 9 Jul 2019 15:58:36 -0700 Subject: [PATCH 13/35] refactor Service Client Builder check class --- .../checks/ServiceClientBuilderCheck.java | 117 +++++++++--------- 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index a8c6e6cb5ec52..057a00c61f935 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -1,9 +1,19 @@ +// 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.TokenTypes; +/** + * The @ServiceClientBuilder class should have following rules: + * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. + * 2) A property named 'serviceClients'. +* 3) Has a method 'buildClient()' to build a synchronous client, + * 4) Has a method 'buildAsyncClient()' to build a asynchronous client + */ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; private static final String BUILD_CLIENT = "buildClient"; @@ -22,12 +32,13 @@ public void beginTree(DetailAST root) { @Override public void finishTree(DetailAST root) { + // Checks if the @ServiceClientBuilder class has an asynchronous and synchronous method. if (hasServiceClientBuilderAnnotation) { if (!hasAsyncClientBuilder) { log(root, String.format("ServiceClientBuilder missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); } if (!hasClientBuilder) { - log(root, String.format("ServiceClientBuilder missing an synchronous method, ''%s''", BUILD_CLIENT)); + log(root, String.format("ServiceClientBuilder missing a synchronous method, ''%s''", BUILD_CLIENT)); } } } @@ -45,7 +56,7 @@ public int[] getAcceptableTokens() { @Override public int[] getRequiredTokens() { return new int[] { - TokenTypes.IMPORT, + TokenTypes.CLASS_DEF, TokenTypes.METHOD_DEF }; } @@ -59,16 +70,38 @@ public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: final DetailAST annotationToken = getServiceClientAnnotation(token); - if (annotationToken != null) { - hasServiceClientBuilderAnnotation = true; - checkAnnotationMemberValuePair(annotationToken); + final String className = token.findFirstToken(TokenTypes.IDENT).getText(); + + hasServiceClientBuilderAnnotation = annotationToken != null; + + if (hasServiceClientBuilderAnnotation) { + // Checks if the ANNOTATION has property named 'serviceClients' + if (!hasServiceClientsAnnotationProperty(annotationToken)) { + log(annotationToken, String.format( + "Annotation @%s should have ''serviceClients'' as property of annotation and should list all of the service clients it can build.", + SERVICE_CLIENT_BUILDER)); + } + // HAS @ServiceClientBuilder annotation but NOT named the class ClientBuilder + if (!className.endsWith("ClientBuilder")) { + log(token, "Service client builder class should be named ClientBuilder."); + } } else { - hasServiceClientBuilderAnnotation = false; + // No @ServiceClientBuilder annotation but HAS named the class ClientBuilder + if (className.endsWith("ClientBuilder")) { + log(token, String.format("Class ''%s'' should be annotated with @ServiceClientBuilder.", className)); + } } - checkForClassNamingAndAnnotation(token); break; case TokenTypes.METHOD_DEF: - checkBuilderMethods(token); + final String methodName = token.findFirstToken(TokenTypes.IDENT).getText(); + + if (BUILD_ASYNC_CLIENT.equals(methodName)) { + hasAsyncClientBuilder = true; + } + + if (BUILD_CLIENT.equals(methodName)) { + hasClientBuilder = true; + } break; default: // Checkstyle complains if there's no default block in switch @@ -77,78 +110,42 @@ public void visitToken(DetailAST token) { } /** - * Checks if the class is annotated with @ServiceClientBuilder + * Checks if the class is annotated with @ServiceClientBuilder. * * @param classDefToken the CLASS_DEF AST node - * @return true if the class is annotated with @ServiceClientBuilder, false otherwise. + * @return the annotation node if the class is annotated with @ServiceClientBuilder, null otherwise. */ private DetailAST getServiceClientAnnotation(DetailAST classDefToken) { final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION) { - continue; - } - if (SERVICE_CLIENT_BUILDER.equals(ast.findFirstToken(TokenTypes.IDENT).getText())) { - return ast; - } + + if (!modifiersToken.branchContains(TokenTypes.ANNOTATION)) { + return null; } - return null; - } - private void checkForClassNamingAndAnnotation(DetailAST classDefToken) { - final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); - if (!className.endsWith("ClientBuilder")) { - log(classDefToken, "Service client builder class should be named ClientBuilder."); - } else { - if (!hasServiceClientBuilderAnnotation) { - log(classDefToken, String.format("Class ''%s'' should be annotated with @ServiceClientBuilder.", className)); - } + DetailAST annotationToken = modifiersToken.findFirstToken(TokenTypes.ANNOTATION); + if (!SERVICE_CLIENT_BUILDER.equals(annotationToken.findFirstToken(TokenTypes.IDENT).getText())) { + return null; } + + return annotationToken; } /** * Checks if the {@code ServiceClientBuilder} annotation has a service client prop named 'serviceClients'. * * @param annotationToken the ANNOTATION AST node + * @return true if the @ServiceClientBuilder has property named 'serviceClients', false if none */ - private void checkAnnotationMemberValuePair(DetailAST annotationToken) { - boolean hasServiceClientPropName = false; - + private boolean hasServiceClientsAnnotationProperty(DetailAST annotationToken) { for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { continue; } - - DetailAST identAST = ast.findFirstToken(TokenTypes.IDENT); - if (identAST == null) { - continue; + // if there is ANNOTATION_MEMBER_VALUE_PAIR exist, it always has IDENT node + if ("serviceClients".equals(ast.findFirstToken(TokenTypes.IDENT).getText())) { + return true; } - - if ("serviceClients".equals(identAST.getText())) { - hasServiceClientPropName = true; - break; - } - } - - if (!hasServiceClientPropName) { - log(annotationToken, String.format( - "Annotation @%s should have ''serviceClients'' as property of annotation and should list all of the service clients it can build.", - SERVICE_CLIENT_BUILDER)); - } - } - - /** - * Every service client builder should have a sync client builder and async client builder, - * buildClient() and buildAsyncClient(), respectively. - * - * @param methodDefToken METHOD_DEF AST node - */ - private void checkBuilderMethods(DetailAST methodDefToken) { - final String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); - if (methodName.equals(BUILD_ASYNC_CLIENT)) { - hasAsyncClientBuilder = true; - } else if (methodName.equals(BUILD_CLIENT)) { - hasClientBuilder = true; } + return false; } } From a5aac0603be5be7c1c26dc8bf0be26719ec5238d Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 12:00:18 -0700 Subject: [PATCH 14/35] Refactor ServiceClientBuilder and ServiceInterface checks --- .../checks/ServiceClientBuilderCheck.java | 44 ++++++++++--------- .../checks/ServiceInterfaceCheck.java | 16 ++++--- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index 057a00c61f935..a696abbf94bec 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -25,21 +25,22 @@ public class ServiceClientBuilderCheck extends AbstractCheck { @Override public void beginTree(DetailAST root) { - hasServiceClientBuilderAnnotation = true; + hasServiceClientBuilderAnnotation = false; hasAsyncClientBuilder = false; hasClientBuilder = false; } @Override public void finishTree(DetailAST root) { - // Checks if the @ServiceClientBuilder class has an asynchronous and synchronous method. - if (hasServiceClientBuilderAnnotation) { - if (!hasAsyncClientBuilder) { - log(root, String.format("ServiceClientBuilder missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); - } - if (!hasClientBuilder) { - log(root, String.format("ServiceClientBuilder missing a synchronous method, ''%s''", BUILD_CLIENT)); - } + if (!hasServiceClientBuilderAnnotation) { + return; + } + + if (!hasAsyncClientBuilder) { + log(root, String.format("Every Missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); + } + if (!hasClientBuilder) { + log(root, String.format("Missing a synchronous method, ''%s''", BUILD_CLIENT)); } } @@ -63,27 +64,23 @@ public int[] getRequiredTokens() { @Override public void visitToken(DetailAST token) { - if (!hasServiceClientBuilderAnnotation) { - return; - } - switch (token.getType()) { case TokenTypes.CLASS_DEF: - final DetailAST annotationToken = getServiceClientAnnotation(token); + final DetailAST serviceClientAnnotationToken = getServiceClientAnnotation(token); final String className = token.findFirstToken(TokenTypes.IDENT).getText(); - hasServiceClientBuilderAnnotation = annotationToken != null; + hasServiceClientBuilderAnnotation = serviceClientAnnotationToken != null; if (hasServiceClientBuilderAnnotation) { // Checks if the ANNOTATION has property named 'serviceClients' - if (!hasServiceClientsAnnotationProperty(annotationToken)) { - log(annotationToken, String.format( + if (!hasServiceClientsAnnotationProperty(serviceClientAnnotationToken)) { + log(serviceClientAnnotationToken, String.format( "Annotation @%s should have ''serviceClients'' as property of annotation and should list all of the service clients it can build.", SERVICE_CLIENT_BUILDER)); } // HAS @ServiceClientBuilder annotation but NOT named the class ClientBuilder if (!className.endsWith("ClientBuilder")) { - log(token, "Service client builder class should be named ClientBuilder."); + log(token, String.format("@ServiceClientBuilder class ''%s''should be named ClientBuilder.", className)); } } else { // No @ServiceClientBuilder annotation but HAS named the class ClientBuilder @@ -93,12 +90,19 @@ public void visitToken(DetailAST token) { } break; case TokenTypes.METHOD_DEF: - final String methodName = token.findFirstToken(TokenTypes.IDENT).getText(); + if (!hasServiceClientBuilderAnnotation) { + return; + } + // if buildAsyncClient() and buildClient() methods already exist, skip rest of METHOD_DEF checks + if (hasAsyncClientBuilder && hasClientBuilder) { + return; + } + + final String methodName = token.findFirstToken(TokenTypes.IDENT).getText(); if (BUILD_ASYNC_CLIENT.equals(methodName)) { hasAsyncClientBuilder = true; } - if (BUILD_CLIENT.equals(methodName)) { hasClientBuilder = true; } diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 3eb3dd92dddec..8e9343a1f6a46 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -41,7 +41,9 @@ public void visitToken(DetailAST token) { } /** - * Checks for @ServiceInterface rules that defined at the beginning of class definition. + * The @ServiceInterface class should have following rules: + * 1) has annotation property 'name' and should be non-empty + * 2) length of value of property 'name' should be less than 10 characters and without space * * @param interfaceDefToken INTERFACE_DEF AST node */ @@ -56,12 +58,13 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { if (ast.getType() != TokenTypes.ANNOTATION) { continue; } - + // ANNOTATION for (DetailAST annotationChild = ast.getFirstChild(); annotationChild != null; annotationChild = annotationChild.getNextSibling()) { // IDENT if (annotationChild.getType() == TokenTypes.IDENT) { + // Skip this annotation if it is not @ServiceInterface, if (!"ServiceInterface".equals(annotationChild.getText())) { break; } else { @@ -77,18 +80,18 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } } } - - // No @ServiceInterface annotation found + + // Checks the rules: + // Skip the check if no @ServiceInterface annotation found if (serviceInterfaceAnnotationNode == null) { return; } - // Checks the rules: // Missing 'name' property if(nameValue == null) { log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); } else { - // No Space allowed + // No Space allowed if (nameValue.contains(" ")) { log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not contain white space."); } @@ -101,6 +104,7 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { /** * Get the name property value from the EXPR node + * * @param exprToken EXPR * @return null if EXPR node doesn't exist or no STRING_LITERAL. Otherwise, returns the value of the property. */ From 036787cc9dcc28e98e925110338e3e02680561b8 Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 14:48:23 -0700 Subject: [PATCH 15/35] merge Service Client Method check to Service Client check --- .../checks/ServiceClientMethodCheck.java | 267 ------------------ 1 file changed, 267 deletions(-) delete mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java deleted file mode 100644 index 76594c741d484..0000000000000 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientMethodCheck.java +++ /dev/null @@ -1,267 +0,0 @@ -// 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.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; -import java.util.stream.Stream; - -/** - * Service Client Method Checks. All methods that has a @ServiceMethod annotation in a class annotated with - * @ServiceClient should follow below rules: - * - * (1) Method naming pattern. Refer to Java Spec: - * (2) Methods should not have "Async" added to the method name - * (3) Return type of async and sync clients should be as per guidelines: - * (a) Return type for async collection should be of type? extends Flux - * (b) Return type for async single value should be of type? extends Mono - * (c) Return type for sync collection should be of type? extends Stream - * (d) Return type for sync single value should be of type? extends Response - */ -public class ServiceClientMethodCheck extends AbstractCheck { - private static final String DOT = "."; - private static final String SERVICE_CLIENT = "ServiceClient"; - private static final String SERVICE_METHOD = "ServiceMethod"; - private static final String ASYNC = "Async"; - private static final String RETURNS = "returns"; - private static final String SINGLE_RETURN_TYPE = "ReturnType.SINGLE"; - private static final String COLLECTION_RETURN_TYPE = "ReturnType.COLLECTION"; - - private static final String FLUX = "reactor.core.publisher.Flux"; - private static final String MONO = "reactor.core.publisher.Mono"; - private static final String RESPONSE = "com.azure.core.http.rest.response"; - - private static final String FAILED_TO_LOAD_MESSAGE = "%s class failed to load, ServiceClientChecks will be ignored."; - private static final String SINGLE_VALUE_RETURN_ERROR = "%s should either be a ''Mono'' class or class extends it if returns an ''async'' single value, " + - "or a ''Response'' class or class extends it if returns a ''sync'' single value."; - private static final String COLLECTION_RETURN_ERROR = "%s should either be a ''Flux'' class or class extends it if returns an ''async'' collection, " + - "or a ''Stream'' class or class extends it if returns a ''sync'' collection."; - - private static final Set COMMON_NAMING_PREFIX_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( - "upsert", "set", "create", "update", "replace", "delete", "add", "get", "list" - ))); - - private static final String COMMON_NAMING_SUFFIX = "Exists"; - - private static boolean hasServiceClientAnnotation; - private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); - - Class monoObj; - Class fluxObj; - Class responseObj; - - @Override - public void init() { - try { - fluxObj = Class.forName(FLUX); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, FLUX)); - } - - try { - monoObj = Class.forName(MONO); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, MONO)); - } - - try { - responseObj = Class.forName(RESPONSE); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, RESPONSE)); - } - } - - @Override - public int[] getDefaultTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getAcceptableTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getRequiredTokens() { - return new int[] { - TokenTypes.IMPORT, - TokenTypes.CLASS_DEF, - TokenTypes.METHOD_DEF - }; - } - - @Override - public void beginTree(DetailAST root) { - hasServiceClientAnnotation = true; - } - - @Override - public void visitToken(DetailAST token) { - if (!hasServiceClientAnnotation) { - return; - } - - switch (token.getType()) { - case TokenTypes.IMPORT: - addImportedClassPath(token); - break; - case TokenTypes.CLASS_DEF: - hasServiceClientAnnotation = hasServiceClientAnnotation(token); - break; - case TokenTypes.METHOD_DEF: - checkMethodNamingPattern(token); - break; - default: - // Checkstyle complains if there's no default block in switch - break; - } - } - - /** - * Add all imported classes into a map, key is the name of class and value is the full package path of class. - * - * @param token the IMPORT AST node - */ - private void addImportedClassPath(DetailAST token) { - final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); - final String className = importClassPath.substring(importClassPath.lastIndexOf(DOT) + 1); - simpleClassNameToQualifiedNameMap.put(className, importClassPath); - } - - /** - * Checks if the class is annotated with @ServiceClient annotation - * - * @param token the CLASS_DEF AST node - * @return true if the class is annotated with @ServiceClient, false otherwise. - */ - private boolean hasServiceClientAnnotation(DetailAST token) { - DetailAST modifiersToken = token.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken != null) { - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() == TokenTypes.ANNOTATION) { - DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); - if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { - return true; - } - } - } - } - return false; - } - - private void checkMethodNamingPattern(DetailAST methodDefToken) { - DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken == null) { - return; - } - DetailAST serviceMethodAnnotation = hasServiceMethodAnnotation(modifiersToken); - if (serviceMethodAnnotation == null) { - return; - } - - DetailAST methodNameToken = methodDefToken.findFirstToken(TokenTypes.IDENT); - if (methodNameToken == null) { - return; - } - String methodName = methodNameToken.getText(); - if (methodName.contains(ASYNC)) { - log(methodNameToken, String.format("Method name ''%s'' should not contain ''%s'' in the method name", - methodName, ASYNC)); - } - - if (!isCommonNamingPattern(methodName)) { - log(methodDefToken, String.format("Method name ''%s'' should follow a common vocabulary. Refer to Java Spec. ", methodName)); - } - - DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); - if (typeToken == null) { - log(typeToken, "Missing return type"); - return; - } - - String returnTypeAnnotation = getReturnTypeFromAnnotation(serviceMethodAnnotation); - String returnType = methodDefToken.findFirstToken(TokenTypes.TYPE).getText(); - - if (!simpleClassNameToQualifiedNameMap.containsKey(returnType)) { - if (SINGLE_RETURN_TYPE.equals(returnTypeAnnotation)) { - log(typeToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); - } else if (COLLECTION_RETURN_TYPE.equals(returnTypeAnnotation)) { - log(typeToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); - } - } - - String qualifiedReturnName = simpleClassNameToQualifiedNameMap.get(returnType); - Class qualifiedReturnTypeInstance; - try { - qualifiedReturnTypeInstance = Class.forName(qualifiedReturnName); - } catch (ClassNotFoundException ex) { - log(methodDefToken, String.format(FAILED_TO_LOAD_MESSAGE, qualifiedReturnName)); - return; - } - - if (SINGLE_RETURN_TYPE.equals(returnTypeAnnotation)) { - if (!qualifiedReturnTypeInstance.isInstance(monoObj) - && !qualifiedReturnTypeInstance.isInstance(responseObj)) { - log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); - } - } else if (COLLECTION_RETURN_TYPE.equals(returnTypeAnnotation)) { - if (!qualifiedReturnTypeInstance.isInstance(fluxObj) - && !qualifiedReturnTypeInstance.isInstance(Stream.class)) { - log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); - } - } else { - log(serviceMethodAnnotation, String.format("''returns'' value = ''%s'' is neither SINGLE nor COLLECTION return type.", returnTypeAnnotation)); - } - } - - - private String getReturnTypeFromAnnotation(DetailAST annotationMemberValuePairToken) { - - DetailAST identToken = annotationMemberValuePairToken.findFirstToken(TokenTypes.IDENT); - if (identToken == null || !RETURNS.equals(identToken.getText())) { - return null; - } - - String returnType = FullIdent.createFullIdentBelow(annotationMemberValuePairToken.findFirstToken(TokenTypes.EXPR)).getText(); - if (returnType != null && !returnType.isEmpty()) { - return returnType; - } - - return null; - } - - private DetailAST hasServiceMethodAnnotation(DetailAST modifiersToken) { - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION) { - continue; - } - - DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); - if (identToken == null || !SERVICE_METHOD.equals(identToken.getText())) { - continue; - } - return ast; - } - - return null; - } - - private boolean isCommonNamingPattern(String methodName) { - boolean isCommonNamingPattern = COMMON_NAMING_PREFIX_SET.stream().anyMatch(commonName -> - methodName.startsWith(commonName)); - if (!isCommonNamingPattern) { - isCommonNamingPattern = methodName.endsWith(COMMON_NAMING_SUFFIX); - } - return isCommonNamingPattern; - } -} From 484dd982667e7580393f1f8ba61299dad191e89e Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 15:11:09 -0700 Subject: [PATCH 16/35] suppression implementation and test but not test my test yet --- .../ServiceClientInstantiationCheck.java | 212 ++++++++++++++++-- .../checkstyle/checkstyle-suppressions.xml | 11 + 2 files changed, 210 insertions(+), 13 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java index 38adfaabbade6..bdd0fb12ae61f 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -10,6 +10,14 @@ import com.puppycrawl.tools.checkstyle.checks.naming.AccessModifier; import com.puppycrawl.tools.checkstyle.utils.CheckUtil; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.stream.Stream; + /** * Verify the classes with annotation @ServiceClient should have following rules: *
      @@ -17,17 +25,47 @@ *
    1. No public static method named 'builder'
      2. *
    2. Since these classes are supposed to be immutable, all fields in the service client classes should be final.
      3. *
    + * + * All methods that has a @ServiceMethod annotation in a class annotated with @ServiceClient should follow below rules: + *
      + *
    1. Method naming pattern. Refer to Java Spec:
      2. + *
    2. Methods should not have "Async" added to the method name
      3. + *
        Return type of async and sync clients should be as per guidelines: + *
      1. Return type for async collection should be of type? extends Flux
        2. + *
      2. Return type for async single value should be of type? extends Mono
        3. + *
      3. Return type for sync collection should be of type? extends Stream
        4. + *
      4. Return type for sync single value should be of type? extends Response
        5. + *
      + *
    */ public class ServiceClientInstantiationCheck extends AbstractCheck { - private static final String SERVICE_CLIENT = "ServiceClient"; - private static final String BUILDER = "builder"; + private static final String ASYNC = "Async"; private static final String ASYNC_CLIENT ="AsyncClient"; + private static final String BUILDER = "builder"; private static final String CLIENT = "Client"; private static final String IS_ASYNC = "isAsync"; + private static final String SERVICE_CLIENT = "ServiceClient"; + + private static final String COLLECTION_RETURN_TYPE = "ReturnType.COLLECTION"; + private static final String SINGLE_RETURN_TYPE = "ReturnType.SINGLE"; + + private static final String FLUX = "reactor.core.publisher.Flux"; + private static final String MONO = "reactor.core.publisher.Mono"; + private static final String RESPONSE = "com.azure.core.http.rest.response"; + + private static final String COLLECTION_RETURN_ERROR = "%s should either be a ''Flux'' class or class extends it if returns an ''async'' collection, " + + "or a ''Stream'' class or class extends it if returns a ''sync'' collection."; + private static final String FAILED_TO_LOAD_MESSAGE = "%s class failed to load, ServiceClientChecks will be ignored."; + private static final String SINGLE_VALUE_RETURN_ERROR = "%s should either be a ''Mono'' class or class extends it if returns an ''async'' single value, " + + "or a ''Response'' class or class extends it if returns a ''sync'' single value."; + + private static final Set COMMON_NAMING_PREFIX_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( + "upsert", "set", "create", "update", "replace", "delete", "add", "get", "list" + ))); - private static boolean hasServiceClientAnnotation; private static boolean isAsync; - private static boolean isImplPackage; + private static boolean hasServiceClientAnnotation; + private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); @Override public int[] getDefaultTokens() { @@ -42,7 +80,7 @@ public int[] getAcceptableTokens() { @Override public int[] getRequiredTokens() { return new int[] { - TokenTypes.PACKAGE_DEF, + TokenTypes.IMPORT, TokenTypes.CLASS_DEF, TokenTypes.CTOR_DEF, TokenTypes.METHOD_DEF, @@ -50,23 +88,42 @@ public int[] getRequiredTokens() { }; } + Class monoObj; + Class fluxObj; + Class responseObj; + + @Override + public void init() { + try { + fluxObj = Class.forName(FLUX); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, FLUX)); + } + + try { + monoObj = Class.forName(MONO); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, MONO)); + } + + try { + responseObj = Class.forName(RESPONSE); + } catch (ClassNotFoundException ex) { + log(0, String.format(FAILED_TO_LOAD_MESSAGE, RESPONSE)); + } + } + @Override public void beginTree(DetailAST root) { hasServiceClientAnnotation = false; isAsync = false; - isImplPackage = false; } @Override public void visitToken(DetailAST token) { - if (isImplPackage) { - return; - } - switch (token.getType()) { - case TokenTypes.PACKAGE_DEF: - String packageName = FullIdent.createFullIdent(token.findFirstToken(TokenTypes.DOT)).getText(); - isImplPackage = packageName.contains(".implementation"); + case TokenTypes.IMPORT: + addImportedClassPath(token); break; case TokenTypes.CLASS_DEF: hasServiceClientAnnotation = hasServiceClientAnnotation(token); @@ -82,6 +139,7 @@ public void visitToken(DetailAST token) { case TokenTypes.METHOD_DEF: if (hasServiceClientAnnotation) { checkMethodName(token); + checkMethodNamingPattern(token); } break; case TokenTypes.OBJBLOCK: @@ -228,4 +286,132 @@ private boolean isAsyncServiceClient(DetailAST annotationToken) { // By default, if the IS_ASYNC doesn't exist, the service client is a synchronous client. return false; } + + + private void checkMethodNamingPattern(DetailAST methodDefToken) { + DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + DetailAST serviceMethodAnnotation = hasServiceMethodAnnotation(modifiersToken); + // NOT a @ServiceMethod method + if (serviceMethodAnnotation == null) { + return; + } + + String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); + if (methodName.contains(ASYNC)) { + log(methodDefToken, String.format("Method name ''%s'' should not contain ''%s'' in the method name", + methodName, ASYNC)); + } + + if (!isCommonNamingPattern(methodName)) { + log(methodDefToken, String.format("Method name ''%s'' should follow a common vocabulary. Refer to Java Spec. ", methodName)); + } + + // Find the annotation member 'returns' value + String returnsAnnotationMemberValue = getAnnotationMemberReturnsValue(serviceMethodAnnotation); + + String returnType = methodDefToken.findFirstToken(TokenTypes.TYPE).getText(); + if (!simpleClassNameToQualifiedNameMap.containsKey(returnType)) { + if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); + } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); + } + } + + String qualifiedReturnName = simpleClassNameToQualifiedNameMap.get(returnType); + Class qualifiedReturnTypeInstance; + try { + qualifiedReturnTypeInstance = Class.forName(qualifiedReturnName); + } catch (ClassNotFoundException ex) { + log(methodDefToken, String.format(FAILED_TO_LOAD_MESSAGE, qualifiedReturnName)); + return; + } + + if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + if (!qualifiedReturnTypeInstance.isInstance(monoObj) + && !qualifiedReturnTypeInstance.isInstance(responseObj)) { + log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); + } + } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + if (!qualifiedReturnTypeInstance.isInstance(fluxObj) + && !qualifiedReturnTypeInstance.isInstance(Stream.class)) { + log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); + } + } else { + log(serviceMethodAnnotation, String.format("''returns'' value = ''%s'' is neither SINGLE nor COLLECTION return type.", returnsAnnotationMemberValue)); + } + } + + /** + * Add all imported classes into a map, key is the name of class and value is the full package path of class. + * + * @param token the IMPORT AST node + */ + private void addImportedClassPath(DetailAST token) { + final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); + final String className = importClassPath.substring(importClassPath.lastIndexOf(".") + 1); + simpleClassNameToQualifiedNameMap.put(className, importClassPath); + } + + /** + * + * @param modifiersToken + * @return + */ + private DetailAST hasServiceMethodAnnotation(DetailAST modifiersToken) { + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + + DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); + if (identToken == null || !"ServiceMethod".equals(identToken.getText())) { + continue; + } + return ast; + } + + return null; + } + + /** + * + * @param methodName + * @return + */ + private boolean isCommonNamingPattern(String methodName) { + boolean isCommonNamingPattern = COMMON_NAMING_PREFIX_SET.stream().anyMatch( + commonName -> methodName.startsWith(commonName)); + if (!isCommonNamingPattern) { + isCommonNamingPattern = methodName.endsWith("Exists"); + } + return isCommonNamingPattern; + } + + /** + * Find the annotation member 'returns' value + * + * @param serviceMethodAnnotation ANNOTATION_MEMBER_VALUE_PAIR AST node + * @return annotation member 'returns' value if found, null otherwise. + */ + private String getAnnotationMemberReturnsValue(DetailAST serviceMethodAnnotation) { + for (DetailAST annotationChild = serviceMethodAnnotation.getFirstChild(); annotationChild != null; + annotationChild = annotationChild.getNextSibling()) { + // Skip if not ANNOTATION_MEMBER_VALUE_PAIR + if (annotationChild.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + continue; + } + // Skip if the annotation member is not 'returns' + String annotationParamName = annotationChild.findFirstToken(TokenTypes.IDENT).getText(); + if (!"returns".equals(annotationParamName)) { + continue; + } + // value of Annotation member 'returns' + String returnsValue = FullIdent.createFullIdentBelow(annotationChild.findFirstToken(TokenTypes.EXPR)).getText(); + if (returnsValue != null && !returnsValue.isEmpty()) { + return returnsValue; + } + } + return null; + } } 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 09da6af09b39e..850f4b7a58c2d 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 @@ -85,4 +85,15 @@ + + + + + + + + + + + From 228a3ad7be1ded5cf8bf2e7cdd7d03c9ff3524ec Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 15:46:00 -0700 Subject: [PATCH 17/35] add license title --- .../azure/tools/checkstyle/checks/ServiceInterfaceCheck.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 8e9343a1f6a46..318678362fbfa 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -1,3 +1,6 @@ +// 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; From a4720d02514e0d49fe2eed260cff9884606be07b Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 16:32:58 -0700 Subject: [PATCH 18/35] ServiceInterface check tested. Looks good --- .../checks/ServiceInterfaceCheck.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 318678362fbfa..166104049670c 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -90,19 +90,22 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { return; } - // Missing 'name' property - if(nameValue == null) { - log(serviceInterfaceAnnotationNode, "@ServiceInterface annotation missing ''name'' property."); - } else { - // No Space allowed - if (nameValue.contains(" ")) { - log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not contain white space."); - } - // Length should less than or equal to 10 characters - if (nameValue.length() > 10) { - log(serviceInterfaceAnnotationNode, "The ''name'' property of @ServiceInterface should not have a length > 10."); - } + // 'name' is required at @ServiceInterface + // 'name' should not be empty + if (nameValue.isEmpty()) { + log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface, ''%s'' should not be empty.", nameValue)); + } + + // No Space allowed + if (nameValue.contains(" ")) { + log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface, ''%s'' should not contain white space.", nameValue)); } + // Length should less than or equal to 10 characters + if (nameValue.length() > 10) { + log(serviceInterfaceAnnotationNode, "[DEBUG] length = " + nameValue.length() + ", name = " + nameValue); + log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface ''%s'' should not have a length > 10.", nameValue)); + } + } /** @@ -121,6 +124,9 @@ private String getNamePropertyValue(DetailAST exprToken) { return null; } - return nameValueToken.getText(); + String nameValue = nameValueToken.getText(); + + // remove the beginning and ending double quote + return nameValue.replaceAll("^\"|\"$", ""); } } From df0685818afeb4de09e0bab39fc539e15beece2f Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 17:56:36 -0700 Subject: [PATCH 19/35] tested ServiceClientBuilder check --- .../checks/ServiceClientBuilderCheck.java | 38 ++++--------------- .../main/resources/checkstyle/checkstyle.xml | 17 +++++++-- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index a696abbf94bec..88ccdd3015285 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -9,10 +9,9 @@ /** * The @ServiceClientBuilder class should have following rules: - * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. - * 2) A property named 'serviceClients'. -* 3) Has a method 'buildClient()' to build a synchronous client, - * 4) Has a method 'buildAsyncClient()' to build a asynchronous client + * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. + * 2) Has a method 'buildClient()' to build a synchronous client, + * 3) Has a method 'buildAsyncClient()' to build a asynchronous client */ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; @@ -37,7 +36,7 @@ public void finishTree(DetailAST root) { } if (!hasAsyncClientBuilder) { - log(root, String.format("Every Missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); + log(root, String.format("Missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); } if (!hasClientBuilder) { log(root, String.format("Missing a synchronous method, ''%s''", BUILD_CLIENT)); @@ -72,15 +71,11 @@ public void visitToken(DetailAST token) { hasServiceClientBuilderAnnotation = serviceClientAnnotationToken != null; if (hasServiceClientBuilderAnnotation) { - // Checks if the ANNOTATION has property named 'serviceClients' - if (!hasServiceClientsAnnotationProperty(serviceClientAnnotationToken)) { - log(serviceClientAnnotationToken, String.format( - "Annotation @%s should have ''serviceClients'' as property of annotation and should list all of the service clients it can build.", - SERVICE_CLIENT_BUILDER)); - } + // Don't need to check if the 'serviceClients' exist. It is required when using @ServiceClientBuilder + // HAS @ServiceClientBuilder annotation but NOT named the class ClientBuilder if (!className.endsWith("ClientBuilder")) { - log(token, String.format("@ServiceClientBuilder class ''%s''should be named ClientBuilder.", className)); + log(token, String.format("@ServiceClientBuilder class ''%s'' should be named ClientBuilder.", className)); } } else { // No @ServiceClientBuilder annotation but HAS named the class ClientBuilder @@ -133,23 +128,4 @@ private DetailAST getServiceClientAnnotation(DetailAST classDefToken) { return annotationToken; } - - /** - * Checks if the {@code ServiceClientBuilder} annotation has a service client prop named 'serviceClients'. - * - * @param annotationToken the ANNOTATION AST node - * @return true if the @ServiceClientBuilder has property named 'serviceClients', false if none - */ - private boolean hasServiceClientsAnnotationProperty(DetailAST annotationToken) { - for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - continue; - } - // if there is ANNOTATION_MEMBER_VALUE_PAIR exist, it always has IDENT node - if ("serviceClients".equals(ast.findFirstToken(TokenTypes.IDENT).getText())) { - return true; - } - } - return false; - } } 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 4b11903d79ecc..6220ef04cbfad 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -282,7 +282,17 @@ page at http://checkstyle.sourceforge.net/config.html --> + 3) Since these classes are supposed to be immutable, all fields in the service client classes should be final + + Also, verify all methods that has a @ServiceMethod annotation in a class annotated with @ServiceClient should + follow below rules: + 1) Follows method naming pattern. Refer to Java Spec. + 2) Methods should not have "Async" added to the method name + 3) Return type of async and sync clients should be as per guidelines: + 3.1) Return type for async collection should be of type? extends Flux + 3.2) Return type for async single value should be of type? extends Mono + 3.3) Return type for sync collection should be of type? extends Stream + 3.4) Return type for sync single value should be of type? extends Response --> @@ -294,9 +304,8 @@ page at http://checkstyle.sourceforge.net/config.html --> + 2) Has a method 'buildClient()' to build a synchronous client, + 3) Has a method 'buildAsyncClient()' to build a asynchronous client --> From 8610ddf594078798adc726039e8aae2b839817a2 Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 18:26:56 -0700 Subject: [PATCH 20/35] revive wording --- .../checks/ServiceClientBuilderCheck.java | 4 ++-- .../checks/ServiceInterfaceCheck.java | 12 +++++----- .../main/resources/checkstyle/checkstyle.xml | 23 +++++++++---------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index 88ccdd3015285..a2dbdac2697fa 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -8,10 +8,10 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes; /** - * The @ServiceClientBuilder class should have following rules: + * The @ServiceClientBuilder class should have the following rules: * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. * 2) Has a method 'buildClient()' to build a synchronous client, - * 3) Has a method 'buildAsyncClient()' to build a asynchronous client + * 3) Has a method 'buildAsyncClient()' to build an asynchronous client */ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 166104049670c..8b7829d7896b1 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -8,9 +8,9 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes; /** - * The @ServiceInterface class should have following rules: - * 1) has annotation property 'name' and should be non-empty - * 2) length of value of property 'name' should be less than 10 characters and without space + * The @ServiceInterface class should have the following rules: + * 1) The annotation property 'name' should be non-empty + * 2) The length of value of property 'name' should be less than 10 characters and without space */ public class ServiceInterfaceCheck extends AbstractCheck { @@ -44,9 +44,9 @@ public void visitToken(DetailAST token) { } /** - * The @ServiceInterface class should have following rules: - * 1) has annotation property 'name' and should be non-empty - * 2) length of value of property 'name' should be less than 10 characters and without space + * The @ServiceInterface class should have the following rules: + * 1) The annotation property 'name' should be non-empty + * 2) The length of value of property 'name' should be less than 10 characters and without space * * @param interfaceDefToken INTERFACE_DEF AST node */ 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 6220ef04cbfad..60a765a6f56f7 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -284,15 +284,15 @@ page at http://checkstyle.sourceforge.net/config.html --> 2) No public static method named 'builder' 3) Since these classes are supposed to be immutable, all fields in the service client classes should be final - Also, verify all methods that has a @ServiceMethod annotation in a class annotated with @ServiceClient should + Also, verify all methods that have a @ServiceMethod annotation in a class annotated with @ServiceClient should follow below rules: 1) Follows method naming pattern. Refer to Java Spec. 2) Methods should not have "Async" added to the method name - 3) Return type of async and sync clients should be as per guidelines: - 3.1) Return type for async collection should be of type? extends Flux - 3.2) Return type for async single value should be of type? extends Mono - 3.3) Return type for sync collection should be of type? extends Stream - 3.4) Return type for sync single value should be of type? extends Response --> + 3) The return type of async and sync clients should be as per guidelines: + 3.1) The return type for async collection should be of type? extends Flux + 3.2) The return type for async single value should be of type? extends Mono + 3.3) The return type for sync collection should be of type? extends Stream + 3.4) The return type for sync single value should be of type? extends Response --> @@ -302,17 +302,16 @@ page at http://checkstyle.sourceforge.net/config.html --> - + 3) Has a method 'buildAsyncClient()' to build an asynchronous client --> - + From 634c1f69ef0769ec0db716c22e783d139562be88 Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 23:05:52 -0700 Subject: [PATCH 21/35] revive wording --- .../tools/checkstyle/checks/ServiceClientBuilderCheck.java | 6 +++--- .../tools/checkstyle/checks/ServiceInterfaceCheck.java | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index a2dbdac2697fa..0f9360b24a2fe 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -36,10 +36,10 @@ public void finishTree(DetailAST root) { } if (!hasAsyncClientBuilder) { - log(root, String.format("Missing an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); + log(root, String.format("The class annotated with @ServiceClientBuilder requires an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); } if (!hasClientBuilder) { - log(root, String.format("Missing a synchronous method, ''%s''", BUILD_CLIENT)); + log(root, String.format("The class annotated with @ServiceClientBuilder requires a synchronous method, ''%s''", BUILD_CLIENT)); } } @@ -75,7 +75,7 @@ public void visitToken(DetailAST token) { // HAS @ServiceClientBuilder annotation but NOT named the class ClientBuilder if (!className.endsWith("ClientBuilder")) { - log(token, String.format("@ServiceClientBuilder class ''%s'' should be named ClientBuilder.", className)); + log(token, String.format("Class annotated with @ServiceClientBuilder ''%s'' should be named ClientBuilder.", className)); } } else { // No @ServiceClientBuilder annotation but HAS named the class ClientBuilder diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 8b7829d7896b1..0f6ef77dc54b7 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -14,6 +14,11 @@ */ public class ServiceInterfaceCheck extends AbstractCheck { + @Override + public void init() { + log(0, "99999999999999999999999999999999999999"); + } + @Override public int[] getDefaultTokens() { return getRequiredTokens(); From f27f579aeab2a64dd36c31bea0b5a840ebb9157c Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 23:06:49 -0700 Subject: [PATCH 22/35] rm rule #5 since it is not ready for review --- .../ServiceClientInstantiationCheck.java | 417 ------------------ 1 file changed, 417 deletions(-) delete mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java deleted file mode 100644 index bdd0fb12ae61f..0000000000000 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ /dev/null @@ -1,417 +0,0 @@ -// 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 com.puppycrawl.tools.checkstyle.checks.naming.AccessModifier; -import com.puppycrawl.tools.checkstyle.utils.CheckUtil; - -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; -import java.util.stream.Stream; - -/** - * Verify the classes with annotation @ServiceClient should have following rules: - *
      - *
    1. No public or protected constructors
      2. - *
    2. No public static method named 'builder'
      3. - *
    3. Since these classes are supposed to be immutable, all fields in the service client classes should be final.
      4. - *
    - * - * All methods that has a @ServiceMethod annotation in a class annotated with @ServiceClient should follow below rules: - *
      - *
    1. Method naming pattern. Refer to Java Spec:
      2. - *
    2. Methods should not have "Async" added to the method name
      3. - *
        Return type of async and sync clients should be as per guidelines: - *
      1. Return type for async collection should be of type? extends Flux
        2. - *
      2. Return type for async single value should be of type? extends Mono
        3. - *
      3. Return type for sync collection should be of type? extends Stream
        4. - *
      4. Return type for sync single value should be of type? extends Response
        5. - *
      - *
    - */ -public class ServiceClientInstantiationCheck extends AbstractCheck { - private static final String ASYNC = "Async"; - private static final String ASYNC_CLIENT ="AsyncClient"; - private static final String BUILDER = "builder"; - private static final String CLIENT = "Client"; - private static final String IS_ASYNC = "isAsync"; - private static final String SERVICE_CLIENT = "ServiceClient"; - - private static final String COLLECTION_RETURN_TYPE = "ReturnType.COLLECTION"; - private static final String SINGLE_RETURN_TYPE = "ReturnType.SINGLE"; - - private static final String FLUX = "reactor.core.publisher.Flux"; - private static final String MONO = "reactor.core.publisher.Mono"; - private static final String RESPONSE = "com.azure.core.http.rest.response"; - - private static final String COLLECTION_RETURN_ERROR = "%s should either be a ''Flux'' class or class extends it if returns an ''async'' collection, " + - "or a ''Stream'' class or class extends it if returns a ''sync'' collection."; - private static final String FAILED_TO_LOAD_MESSAGE = "%s class failed to load, ServiceClientChecks will be ignored."; - private static final String SINGLE_VALUE_RETURN_ERROR = "%s should either be a ''Mono'' class or class extends it if returns an ''async'' single value, " + - "or a ''Response'' class or class extends it if returns a ''sync'' single value."; - - private static final Set COMMON_NAMING_PREFIX_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( - "upsert", "set", "create", "update", "replace", "delete", "add", "get", "list" - ))); - - private static boolean isAsync; - private static boolean hasServiceClientAnnotation; - private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); - - @Override - public int[] getDefaultTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getAcceptableTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getRequiredTokens() { - return new int[] { - TokenTypes.IMPORT, - TokenTypes.CLASS_DEF, - TokenTypes.CTOR_DEF, - TokenTypes.METHOD_DEF, - TokenTypes.OBJBLOCK - }; - } - - Class monoObj; - Class fluxObj; - Class responseObj; - - @Override - public void init() { - try { - fluxObj = Class.forName(FLUX); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, FLUX)); - } - - try { - monoObj = Class.forName(MONO); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, MONO)); - } - - try { - responseObj = Class.forName(RESPONSE); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, RESPONSE)); - } - } - - @Override - public void beginTree(DetailAST root) { - hasServiceClientAnnotation = false; - isAsync = false; - } - - @Override - public void visitToken(DetailAST token) { - switch (token.getType()) { - case TokenTypes.IMPORT: - addImportedClassPath(token); - break; - case TokenTypes.CLASS_DEF: - hasServiceClientAnnotation = hasServiceClientAnnotation(token); - if (hasServiceClientAnnotation) { - checkServiceClientNaming(token); - } - break; - case TokenTypes.CTOR_DEF: - if (hasServiceClientAnnotation) { - checkConstructor(token); - } - break; - case TokenTypes.METHOD_DEF: - if (hasServiceClientAnnotation) { - checkMethodName(token); - checkMethodNamingPattern(token); - } - break; - case TokenTypes.OBJBLOCK: - if (hasServiceClientAnnotation) { - checkClassField(token); - } - break; - default: - // Checkstyle complains if there's no default block in switch - break; - } - } - - /** - * Checks if the class is annotated with annotation @ServiceClient. A class could have multiple annotations. - * - * @param classDefToken the CLASS_DEF AST node - * @return true if the class is annotated with @ServiceClient, false otherwise. - */ - private boolean hasServiceClientAnnotation(DetailAST classDefToken) { - // Always has MODIFIERS node - final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); - - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION) { - continue; - } - // One class could have multiple annotations, return true if found one. - final DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); - if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { - isAsync = isAsyncServiceClient(ast); - return true; - } - } - // If no @ServiceClient annotated with this class, return false - return false; - } - - /** - * Checks for public or protected constructor for the service client class. - * Log error if the service client has public or protected constructor. - * - * @param ctorToken the CTOR_DEF AST node - */ - private void checkConstructor(DetailAST ctorToken) { - final DetailAST modifiersToken = ctorToken.findFirstToken(TokenTypes.MODIFIERS); - // find constructor's modifier accessibility, no public or protected constructor - final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); - if (accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED)) { - log(modifiersToken, "@ServiceClient class should not have any public or protected constructor."); - } - } - - /** - * Checks for public static method named 'builder'. Should avoid to use method name, 'builder'. - * - * @param methodDefToken the METHOD_DEF AST node - */ - private void checkMethodName(DetailAST methodDefToken) { - final DetailAST methodNameToken = methodDefToken.findFirstToken(TokenTypes.IDENT); - if (!BUILDER.equals(methodNameToken.getText())) { - return; - } - - final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); - // find method's modifier accessibility, should not have a public static method called 'builder' - final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); - if (accessModifier.equals(AccessModifier.PUBLIC) && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { - log(modifiersToken, "@ServiceClient class should not have a public static method named ''builder''."); - } - } - - /** - * Checks that the field variables in the @ServiceClient are final. ServiceClients should be immutable. - * - * @param objBlockToken the OBJBLOCK AST node - */ - private void checkClassField(DetailAST objBlockToken) { - for (DetailAST ast = objBlockToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (TokenTypes.VARIABLE_DEF != ast.getType()) { - continue; - } - final DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); - // VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under - // MODIFIERS token. Also the previous sibling of OBJBLOCK will always be class name IDENT node. - if (!modifiersToken.branchContains(TokenTypes.FINAL)) { - log(modifiersToken, String.format("The variable field ''%s'' of class ''%s'' should be final. Classes annotated with @ServiceClient are supposed to be immutable.", - ast.findFirstToken(TokenTypes.IDENT).getText(), objBlockToken.getPreviousSibling().getText())); - } - } - } - - /** - * Checks for the class name of Service Client. It should be named AsyncClient or Client. - * - * @param classDefToken the CLASS_DEF AST node - */ - private void checkServiceClientNaming(DetailAST classDefToken) { - final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); - // Async service client - if (isAsync && !className.endsWith(ASYNC_CLIENT)) { - log(classDefToken, String.format("Async class ''%s'' must be named AsyncClient ", className)); - } - // Sync service client - if (!isAsync && !className.endsWith(CLIENT)) { - log(classDefToken, String.format("Sync class %s must be named Client.", className)); - } - } - - /** - * A function checks if the annotation node has a member key is {@code IS_ASYNC} with value equals to 'true'. - * If the value equals 'true', which indicates the @ServiceClient is an asynchronous client. - * If the member pair is missing. By default, it is a synchronous service client. - * - * @param annotationToken the ANNOTATION AST node - * @return true if the annotation has {@code IS_ASYNC} value 'true', otherwise, false. - */ - private boolean isAsyncServiceClient(DetailAST annotationToken) { - for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - continue; - } - - // skip this annotation member value pair if no IDENT found, since we are looking for member, 'isAsync'. - final DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); - if (identToken == null) { - continue; - } - - // skip this annotation member value pair if the member is not 'isAsync'. - if (!IS_ASYNC.equals(identToken.getText())) { - continue; - } - - // skip this annotation member value pair if the member has no EXPR value - final DetailAST exprToken = ast.findFirstToken(TokenTypes.EXPR); - if (exprToken == null) { - continue; - } - - // true if isAsync = true, false otherwise. - return exprToken.branchContains(TokenTypes.LITERAL_TRUE); - } - // By default, if the IS_ASYNC doesn't exist, the service client is a synchronous client. - return false; - } - - - private void checkMethodNamingPattern(DetailAST methodDefToken) { - DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); - DetailAST serviceMethodAnnotation = hasServiceMethodAnnotation(modifiersToken); - // NOT a @ServiceMethod method - if (serviceMethodAnnotation == null) { - return; - } - - String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); - if (methodName.contains(ASYNC)) { - log(methodDefToken, String.format("Method name ''%s'' should not contain ''%s'' in the method name", - methodName, ASYNC)); - } - - if (!isCommonNamingPattern(methodName)) { - log(methodDefToken, String.format("Method name ''%s'' should follow a common vocabulary. Refer to Java Spec. ", methodName)); - } - - // Find the annotation member 'returns' value - String returnsAnnotationMemberValue = getAnnotationMemberReturnsValue(serviceMethodAnnotation); - - String returnType = methodDefToken.findFirstToken(TokenTypes.TYPE).getText(); - if (!simpleClassNameToQualifiedNameMap.containsKey(returnType)) { - if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { - log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); - } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { - log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); - } - } - - String qualifiedReturnName = simpleClassNameToQualifiedNameMap.get(returnType); - Class qualifiedReturnTypeInstance; - try { - qualifiedReturnTypeInstance = Class.forName(qualifiedReturnName); - } catch (ClassNotFoundException ex) { - log(methodDefToken, String.format(FAILED_TO_LOAD_MESSAGE, qualifiedReturnName)); - return; - } - - if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { - if (!qualifiedReturnTypeInstance.isInstance(monoObj) - && !qualifiedReturnTypeInstance.isInstance(responseObj)) { - log(methodDefToken, String.format(SINGLE_VALUE_RETURN_ERROR, SINGLE_RETURN_TYPE)); - } - } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { - if (!qualifiedReturnTypeInstance.isInstance(fluxObj) - && !qualifiedReturnTypeInstance.isInstance(Stream.class)) { - log(methodDefToken, String.format(COLLECTION_RETURN_ERROR, COLLECTION_RETURN_TYPE)); - } - } else { - log(serviceMethodAnnotation, String.format("''returns'' value = ''%s'' is neither SINGLE nor COLLECTION return type.", returnsAnnotationMemberValue)); - } - } - - /** - * Add all imported classes into a map, key is the name of class and value is the full package path of class. - * - * @param token the IMPORT AST node - */ - private void addImportedClassPath(DetailAST token) { - final String importClassPath = FullIdent.createFullIdentBelow(token).getText(); - final String className = importClassPath.substring(importClassPath.lastIndexOf(".") + 1); - simpleClassNameToQualifiedNameMap.put(className, importClassPath); - } - - /** - * - * @param modifiersToken - * @return - */ - private DetailAST hasServiceMethodAnnotation(DetailAST modifiersToken) { - for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { - if (ast.getType() != TokenTypes.ANNOTATION) { - continue; - } - - DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); - if (identToken == null || !"ServiceMethod".equals(identToken.getText())) { - continue; - } - return ast; - } - - return null; - } - - /** - * - * @param methodName - * @return - */ - private boolean isCommonNamingPattern(String methodName) { - boolean isCommonNamingPattern = COMMON_NAMING_PREFIX_SET.stream().anyMatch( - commonName -> methodName.startsWith(commonName)); - if (!isCommonNamingPattern) { - isCommonNamingPattern = methodName.endsWith("Exists"); - } - return isCommonNamingPattern; - } - - /** - * Find the annotation member 'returns' value - * - * @param serviceMethodAnnotation ANNOTATION_MEMBER_VALUE_PAIR AST node - * @return annotation member 'returns' value if found, null otherwise. - */ - private String getAnnotationMemberReturnsValue(DetailAST serviceMethodAnnotation) { - for (DetailAST annotationChild = serviceMethodAnnotation.getFirstChild(); annotationChild != null; - annotationChild = annotationChild.getNextSibling()) { - // Skip if not ANNOTATION_MEMBER_VALUE_PAIR - if (annotationChild.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - continue; - } - // Skip if the annotation member is not 'returns' - String annotationParamName = annotationChild.findFirstToken(TokenTypes.IDENT).getText(); - if (!"returns".equals(annotationParamName)) { - continue; - } - // value of Annotation member 'returns' - String returnsValue = FullIdent.createFullIdentBelow(annotationChild.findFirstToken(TokenTypes.EXPR)).getText(); - if (returnsValue != null && !returnsValue.isEmpty()) { - return returnsValue; - } - } - return null; - } -} From ea745cf628579419fef654f9a7d350c5780f5fc0 Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 23:21:24 -0700 Subject: [PATCH 23/35] without ServiceMethod but keep ServiceClient annotation --- .../ServiceClientInstantiationCheck.java | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java new file mode 100644 index 0000000000000..38adfaabbade6 --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java @@ -0,0 +1,231 @@ +// 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 com.puppycrawl.tools.checkstyle.checks.naming.AccessModifier; +import com.puppycrawl.tools.checkstyle.utils.CheckUtil; + +/** + * Verify the classes with annotation @ServiceClient should have following rules: + *
      + *
    1. No public or protected constructors
      2. + *
    2. No public static method named 'builder'
      3. + *
    3. Since these classes are supposed to be immutable, all fields in the service client classes should be final.
      4. + *
    + */ +public class ServiceClientInstantiationCheck extends AbstractCheck { + private static final String SERVICE_CLIENT = "ServiceClient"; + private static final String BUILDER = "builder"; + private static final String ASYNC_CLIENT ="AsyncClient"; + private static final String CLIENT = "Client"; + private static final String IS_ASYNC = "isAsync"; + + private static boolean hasServiceClientAnnotation; + private static boolean isAsync; + private static boolean isImplPackage; + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.PACKAGE_DEF, + TokenTypes.CLASS_DEF, + TokenTypes.CTOR_DEF, + TokenTypes.METHOD_DEF, + TokenTypes.OBJBLOCK + }; + } + + @Override + public void beginTree(DetailAST root) { + hasServiceClientAnnotation = false; + isAsync = false; + isImplPackage = false; + } + + @Override + public void visitToken(DetailAST token) { + if (isImplPackage) { + return; + } + + switch (token.getType()) { + case TokenTypes.PACKAGE_DEF: + String packageName = FullIdent.createFullIdent(token.findFirstToken(TokenTypes.DOT)).getText(); + isImplPackage = packageName.contains(".implementation"); + break; + case TokenTypes.CLASS_DEF: + hasServiceClientAnnotation = hasServiceClientAnnotation(token); + if (hasServiceClientAnnotation) { + checkServiceClientNaming(token); + } + break; + case TokenTypes.CTOR_DEF: + if (hasServiceClientAnnotation) { + checkConstructor(token); + } + break; + case TokenTypes.METHOD_DEF: + if (hasServiceClientAnnotation) { + checkMethodName(token); + } + break; + case TokenTypes.OBJBLOCK: + if (hasServiceClientAnnotation) { + checkClassField(token); + } + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + /** + * Checks if the class is annotated with annotation @ServiceClient. A class could have multiple annotations. + * + * @param classDefToken the CLASS_DEF AST node + * @return true if the class is annotated with @ServiceClient, false otherwise. + */ + private boolean hasServiceClientAnnotation(DetailAST classDefToken) { + // Always has MODIFIERS node + final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); + + for (DetailAST ast = modifiersToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION) { + continue; + } + // One class could have multiple annotations, return true if found one. + final DetailAST annotationIdent = ast.findFirstToken(TokenTypes.IDENT); + if (annotationIdent != null && SERVICE_CLIENT.equals(annotationIdent.getText())) { + isAsync = isAsyncServiceClient(ast); + return true; + } + } + // If no @ServiceClient annotated with this class, return false + return false; + } + + /** + * Checks for public or protected constructor for the service client class. + * Log error if the service client has public or protected constructor. + * + * @param ctorToken the CTOR_DEF AST node + */ + private void checkConstructor(DetailAST ctorToken) { + final DetailAST modifiersToken = ctorToken.findFirstToken(TokenTypes.MODIFIERS); + // find constructor's modifier accessibility, no public or protected constructor + final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (accessModifier.equals(AccessModifier.PUBLIC) || accessModifier.equals(AccessModifier.PROTECTED)) { + log(modifiersToken, "@ServiceClient class should not have any public or protected constructor."); + } + } + + /** + * Checks for public static method named 'builder'. Should avoid to use method name, 'builder'. + * + * @param methodDefToken the METHOD_DEF AST node + */ + private void checkMethodName(DetailAST methodDefToken) { + final DetailAST methodNameToken = methodDefToken.findFirstToken(TokenTypes.IDENT); + if (!BUILDER.equals(methodNameToken.getText())) { + return; + } + + final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + // find method's modifier accessibility, should not have a public static method called 'builder' + final AccessModifier accessModifier = CheckUtil.getAccessModifierFromModifiersToken(modifiersToken); + if (accessModifier.equals(AccessModifier.PUBLIC) && modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { + log(modifiersToken, "@ServiceClient class should not have a public static method named ''builder''."); + } + } + + /** + * Checks that the field variables in the @ServiceClient are final. ServiceClients should be immutable. + * + * @param objBlockToken the OBJBLOCK AST node + */ + private void checkClassField(DetailAST objBlockToken) { + for (DetailAST ast = objBlockToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (TokenTypes.VARIABLE_DEF != ast.getType()) { + continue; + } + final DetailAST modifiersToken = ast.findFirstToken(TokenTypes.MODIFIERS); + // VARIABLE_DEF token will always MODIFIERS token. If there is no modifier at the variable, no child under + // MODIFIERS token. Also the previous sibling of OBJBLOCK will always be class name IDENT node. + if (!modifiersToken.branchContains(TokenTypes.FINAL)) { + log(modifiersToken, String.format("The variable field ''%s'' of class ''%s'' should be final. Classes annotated with @ServiceClient are supposed to be immutable.", + ast.findFirstToken(TokenTypes.IDENT).getText(), objBlockToken.getPreviousSibling().getText())); + } + } + } + + /** + * Checks for the class name of Service Client. It should be named AsyncClient or Client. + * + * @param classDefToken the CLASS_DEF AST node + */ + private void checkServiceClientNaming(DetailAST classDefToken) { + final String className = classDefToken.findFirstToken(TokenTypes.IDENT).getText(); + // Async service client + if (isAsync && !className.endsWith(ASYNC_CLIENT)) { + log(classDefToken, String.format("Async class ''%s'' must be named AsyncClient ", className)); + } + // Sync service client + if (!isAsync && !className.endsWith(CLIENT)) { + log(classDefToken, String.format("Sync class %s must be named Client.", className)); + } + } + + /** + * A function checks if the annotation node has a member key is {@code IS_ASYNC} with value equals to 'true'. + * If the value equals 'true', which indicates the @ServiceClient is an asynchronous client. + * If the member pair is missing. By default, it is a synchronous service client. + * + * @param annotationToken the ANNOTATION AST node + * @return true if the annotation has {@code IS_ASYNC} value 'true', otherwise, false. + */ + private boolean isAsyncServiceClient(DetailAST annotationToken) { + for (DetailAST ast = annotationToken.getFirstChild(); ast != null; ast = ast.getNextSibling()) { + if (ast.getType() != TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { + continue; + } + + // skip this annotation member value pair if no IDENT found, since we are looking for member, 'isAsync'. + final DetailAST identToken = ast.findFirstToken(TokenTypes.IDENT); + if (identToken == null) { + continue; + } + + // skip this annotation member value pair if the member is not 'isAsync'. + if (!IS_ASYNC.equals(identToken.getText())) { + continue; + } + + // skip this annotation member value pair if the member has no EXPR value + final DetailAST exprToken = ast.findFirstToken(TokenTypes.EXPR); + if (exprToken == null) { + continue; + } + + // true if isAsync = true, false otherwise. + return exprToken.branchContains(TokenTypes.LITERAL_TRUE); + } + // By default, if the IS_ASYNC doesn't exist, the service client is a synchronous client. + return false; + } +} From fe12876877bd6818a1ee89561a0506d05e4f078c Mon Sep 17 00:00:00 2001 From: shafang Date: Wed, 10 Jul 2019 23:25:08 -0700 Subject: [PATCH 24/35] remove debugging and comment for rule 5 --- .../checkstyle/checks/ServiceInterfaceCheck.java | 6 ------ .../src/main/resources/checkstyle/checkstyle.xml | 14 ++------------ 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 0f6ef77dc54b7..8aec6405aedcd 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -13,12 +13,6 @@ * 2) The length of value of property 'name' should be less than 10 characters and without space */ public class ServiceInterfaceCheck extends AbstractCheck { - - @Override - public void init() { - log(0, "99999999999999999999999999999999999999"); - } - @Override public int[] getDefaultTokens() { return getRequiredTokens(); 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 60a765a6f56f7..c2582a23b5ab4 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -282,17 +282,7 @@ page at http://checkstyle.sourceforge.net/config.html --> + 3) Since these classes are supposed to be immutable, all fields in the service client classes should be final --> @@ -311,7 +301,7 @@ page at http://checkstyle.sourceforge.net/config.html --> + 2) The length of value of property 'name' should be less than 10 characters and without space --> From 8f0a8c9a350bfe30fd9367d4c706cb432836000b Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 00:19:23 -0700 Subject: [PATCH 25/35] rule 8 added --- .../src/main/resources/checkstyle/checkstyle-suppressions.xml | 2 ++ .../src/main/resources/checkstyle/checkstyle.xml | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) 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 850f4b7a58c2d..b8e4ddaaab519 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,4 +96,6 @@ + + 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 c2582a23b5ab4..8c5e1e895f4b2 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -49,7 +49,7 @@ page at http://checkstyle.sourceforge.net/config.html --> - + From 2563084e10c32458f993d9ecfa1d8be3b2b0b78c Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 14:02:48 -0700 Subject: [PATCH 26/35] refacor based on srnagar's feedback --- .../checks/ServiceClientBuilderCheck.java | 69 +++++++++++-------- .../checks/ServiceInterfaceCheck.java | 56 ++++++--------- 2 files changed, 63 insertions(+), 62 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index 0f9360b24a2fe..6a1bb101f7845 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -7,6 +7,8 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import java.util.Stack; + /** * The @ServiceClientBuilder class should have the following rules: * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. @@ -18,30 +20,10 @@ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String BUILD_CLIENT = "buildClient"; private static final String BUILD_ASYNC_CLIENT = "buildAsyncClient"; - private static boolean hasServiceClientBuilderAnnotation; - private static boolean hasAsyncClientBuilder; - private static boolean hasClientBuilder; - - @Override - public void beginTree(DetailAST root) { - hasServiceClientBuilderAnnotation = false; - hasAsyncClientBuilder = false; - hasClientBuilder = false; - } - - @Override - public void finishTree(DetailAST root) { - if (!hasServiceClientBuilderAnnotation) { - return; - } - - if (!hasAsyncClientBuilder) { - log(root, String.format("The class annotated with @ServiceClientBuilder requires an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); - } - if (!hasClientBuilder) { - log(root, String.format("The class annotated with @ServiceClientBuilder requires a synchronous method, ''%s''", BUILD_CLIENT)); - } - } + private Stack hasServiceClientBuilderAnnotationStack = new Stack(); + private boolean hasServiceClientBuilderAnnotation; + private boolean hasAsyncClientBuilder; + private boolean hasClientBuilder; @Override public int[] getDefaultTokens() { @@ -61,15 +43,46 @@ public int[] getRequiredTokens() { }; } + @Override + public void leaveToken(DetailAST token) { + switch (token.getType()) { + case TokenTypes.CLASS_DEF: + final boolean previousState = hasServiceClientBuilderAnnotationStack.pop(); + + if (!hasServiceClientBuilderAnnotation) { + return; + } + + if (!hasAsyncClientBuilder) { + log(token, String.format("The class annotated with @ServiceClientBuilder requires an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); + } + if (!hasClientBuilder) { + log(token, String.format("The class annotated with @ServiceClientBuilder requires a synchronous method, ''%s''", BUILD_CLIENT)); + } + // end of CLASS_DEF node, reset the value back to previous state + hasServiceClientBuilderAnnotation = previousState; + 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: - final DetailAST serviceClientAnnotationToken = getServiceClientAnnotation(token); - final String className = token.findFirstToken(TokenTypes.IDENT).getText(); + // for the starting root of every class, reset to false + hasAsyncClientBuilder = false; + hasClientBuilder = false; + + // Save the state of variable 'hasServiceClientBuilderAnnotation' to limit the scope of accessibility + hasServiceClientBuilderAnnotationStack.push(hasServiceClientBuilderAnnotation); - hasServiceClientBuilderAnnotation = serviceClientAnnotationToken != null; + final DetailAST serviceClientAnnotationBuilderToken = getServiceClientBuilderAnnotation(token); + final String className = token.findFirstToken(TokenTypes.IDENT).getText(); + hasServiceClientBuilderAnnotation = serviceClientAnnotationBuilderToken != null; if (hasServiceClientBuilderAnnotation) { // Don't need to check if the 'serviceClients' exist. It is required when using @ServiceClientBuilder @@ -114,7 +127,7 @@ public void visitToken(DetailAST token) { * @param classDefToken the CLASS_DEF AST node * @return the annotation node if the class is annotated with @ServiceClientBuilder, null otherwise. */ - private DetailAST getServiceClientAnnotation(DetailAST classDefToken) { + private DetailAST getServiceClientBuilderAnnotation(DetailAST classDefToken) { final DetailAST modifiersToken = classDefToken.findFirstToken(TokenTypes.MODIFIERS); if (!modifiersToken.branchContains(TokenTypes.ANNOTATION)) { diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 8aec6405aedcd..f7e6e1416577d 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -7,6 +7,8 @@ import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; +import java.util.regex.Pattern; + /** * The @ServiceInterface class should have the following rules: * 1) The annotation property 'name' should be non-empty @@ -60,26 +62,20 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { if (ast.getType() != TokenTypes.ANNOTATION) { continue; } - // ANNOTATION - for (DetailAST annotationChild = ast.getFirstChild(); annotationChild != null; - annotationChild = annotationChild.getNextSibling()) { - - // IDENT - if (annotationChild.getType() == TokenTypes.IDENT) { - // Skip this annotation if it is not @ServiceInterface, - if (!"ServiceInterface".equals(annotationChild.getText())) { - break; - } else { - serviceInterfaceAnnotationNode = ast; - } - } - - // ANNOTATION_MEMBER_VALUE_PAIR - if (annotationChild.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR) { - if ("name".equals(annotationChild.findFirstToken(TokenTypes.IDENT).getText())) { - nameValue = getNamePropertyValue(annotationChild.findFirstToken(TokenTypes.EXPR)); - } - } + // Skip if not @ServiceInterface annotation + if (!"ServiceInterface".equals(ast.findFirstToken(TokenTypes.IDENT).getText())) { + continue; + } + + // Get the @ServiceInterface annotation node + serviceInterfaceAnnotationNode = ast; + + // Get the 'name' property value of @ServiceInterface + // @ServiceInterface requires 'name' property + DetailAST annotationMemberValuePairToken = ast.findFirstToken(TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR); + if ("name".equals(annotationMemberValuePairToken.findFirstToken(TokenTypes.IDENT).getText())) { + nameValue = getNamePropertyValue(annotationMemberValuePairToken.findFirstToken(TokenTypes.EXPR)); + break; } } @@ -90,21 +86,13 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } // 'name' is required at @ServiceInterface - // 'name' should not be empty - if (nameValue.isEmpty()) { - log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface, ''%s'' should not be empty.", nameValue)); - } - - // No Space allowed - if (nameValue.contains(" ")) { - log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface, ''%s'' should not contain white space.", nameValue)); + // 'name' should not be empty, no Space allowed and the length should less than or equal to 10 characters + Pattern serviceNamePattern = Pattern.compile("^[a-zA-Z0-9]{1,10}$"); + if (!serviceNamePattern.matcher(nameValue).find()) { + log(serviceInterfaceAnnotationNode, String.format( + "The ''name'' property of @ServiceInterface, ''%s'' should be non-empty, alphanumeric and not more than 10 characters", + nameValue)); } - // Length should less than or equal to 10 characters - if (nameValue.length() > 10) { - log(serviceInterfaceAnnotationNode, "[DEBUG] length = " + nameValue.length() + ", name = " + nameValue); - log(serviceInterfaceAnnotationNode, String.format("The ''name'' property of @ServiceInterface ''%s'' should not have a length > 10.", nameValue)); - } - } /** From 128690bac791e6ee17d6f0326b134085bc662e1e Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 15:01:14 -0700 Subject: [PATCH 27/35] add rule 9 --- .../checks/ThrownClientLoggerCheck.java | 46 +++++++++++++++++++ .../checkstyle/checkstyle-suppressions.xml | 3 ++ .../main/resources/checkstyle/checkstyle.xml | 4 ++ 3 files changed, 53 insertions(+) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java new file mode 100644 index 0000000000000..8770d38657ae2 --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java @@ -0,0 +1,46 @@ +// 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.TokenTypes; + +/** + * To throws an exception, must do it through a 'clientLogger.logAndThrow', + * rather than by directly calling 'throw exception' + */ +public class ThrownClientLoggerCheck extends AbstractCheck { + + @Override + public int[] getDefaultTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return getRequiredTokens(); + } + + @Override + public int[] getRequiredTokens() { + return new int[] { + TokenTypes.LITERAL_THROWS + }; + } + + @Override + public void visitToken(DetailAST token) { + + switch (token.getType()) { + case TokenTypes.LITERAL_THROWS: + log(token, "To throws an exception, must do it through a ''clientLogger.logAndThrow'', rather than " + + "by directly calling ''throw exception''."); + 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 b8e4ddaaab519..5e60d2ec1fb0c 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 @@ -98,4 +98,7 @@ + + + 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 8c5e1e895f4b2..a627fe42d3f5e 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -303,5 +303,9 @@ page at http://checkstyle.sourceforge.net/config.html --> 1) The annotation property 'name' should be non-empty 2) The length of value of property 'name' should be less than 10 characters and without space --> + + + + From b6e23478cc96a8ce3852a1e471eec3d7e48939a3 Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 15:08:15 -0700 Subject: [PATCH 28/35] only track 2 and correct path --- .../src/main/resources/checkstyle/checkstyle-suppressions.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 5e60d2ec1fb0c..9bff361f5ba82 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 @@ -89,6 +89,7 @@ + @@ -100,5 +101,5 @@ - + From aba32d251201e9fdb4004e62f2da1daaa86f8ffc Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 17:30:29 -0700 Subject: [PATCH 29/35] updates ThrowClientLoggerCheck --- .../checks/ThrownClientLoggerCheck.java | 29 +++++++++++++++---- .../checkstyle/checkstyle-suppressions.xml | 3 ++ .../main/resources/checkstyle/checkstyle.xml | 2 +- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java index 8770d38657ae2..698ce33b22a4b 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java @@ -8,8 +8,13 @@ import com.puppycrawl.tools.checkstyle.api.TokenTypes; /** - * To throws an exception, must do it through a 'clientLogger.logAndThrow', - * rather than by directly calling 'throw exception' + * To throws an exception, must do it through a 'logger.logAndThrow', + * rather than by directly calling 'throw exception'. + * + * Skip checks if + * + *
  • throw exception from static method
    • + *     */ public class ThrownClientLoggerCheck extends AbstractCheck { @@ -26,7 +31,7 @@ public int[] getAcceptableTokens() { @Override public int[] getRequiredTokens() { return new int[] { - TokenTypes.LITERAL_THROWS + TokenTypes.LITERAL_THROW }; } @@ -34,8 +39,22 @@ public int[] getRequiredTokens() { public void visitToken(DetailAST token) { switch (token.getType()) { - case TokenTypes.LITERAL_THROWS: - log(token, "To throws an exception, must do it through a ''clientLogger.logAndThrow'', rather than " + case TokenTypes.LITERAL_THROW: + DetailAST parentToken = token.getParent(); + while (parentToken!= null) { + // Skip throw exception Throw from static method + if (parentToken.getType() == TokenTypes.METHOD_DEF) { + DetailAST modifiersToken = parentToken.findFirstToken(TokenTypes.MODIFIERS); + if (modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { + return; + } + break; + } + parentToken = parentToken.getParent(); + } + + // non static method and class + log(token, "To throws an exception, must do it through a ''logger.logAndThrow'', rather than " + "by directly calling ''throw exception''."); break; default: 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 9bff361f5ba82..a0430a349e4a8 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 @@ -102,4 +102,7 @@ + + + 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 a627fe42d3f5e..7c321e15abb10 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -305,7 +305,7 @@ page at http://checkstyle.sourceforge.net/config.html --> - + From f77acdb9e100970a1478002a056e34827431fd29 Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 23:28:50 -0700 Subject: [PATCH 30/35] move out ThrownClientLoggerCheck --- .../checks/ThrownClientLoggerCheck.java | 65 ------------------- 1 file changed, 65 deletions(-) delete mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java deleted file mode 100644 index 698ce33b22a4b..0000000000000 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ThrownClientLoggerCheck.java +++ /dev/null @@ -1,65 +0,0 @@ -// 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.TokenTypes; - -/** - * To throws an exception, must do it through a 'logger.logAndThrow', - * rather than by directly calling 'throw exception'. - * - * Skip checks if - * - *
  • throw exception from static method
    • - *     - */ -public class ThrownClientLoggerCheck extends AbstractCheck { - - @Override - public int[] getDefaultTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getAcceptableTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getRequiredTokens() { - return new int[] { - TokenTypes.LITERAL_THROW - }; - } - - @Override - public void visitToken(DetailAST token) { - - switch (token.getType()) { - case TokenTypes.LITERAL_THROW: - DetailAST parentToken = token.getParent(); - while (parentToken!= null) { - // Skip throw exception Throw from static method - if (parentToken.getType() == TokenTypes.METHOD_DEF) { - DetailAST modifiersToken = parentToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifiersToken.branchContains(TokenTypes.LITERAL_STATIC)) { - return; - } - break; - } - parentToken = parentToken.getParent(); - } - - // non static method and class - log(token, "To throws an exception, must do it through a ''logger.logAndThrow'', rather than " - + "by directly calling ''throw exception''."); - break; - default: - // Checkstyle complains if there's no default block in switch - break; - } - } -} From d18e3a1c2f8749e3a02090ef884aa14bdd14d733 Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 11 Jul 2019 23:32:01 -0700 Subject: [PATCH 31/35] comment out ThrownClientLoggerCheck --- .../src/main/resources/checkstyle/checkstyle-suppressions.xml | 2 +- .../src/main/resources/checkstyle/checkstyle.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 a0430a349e4a8..8d16874c0bd00 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 @@ -101,7 +101,7 @@ - + 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 7c321e15abb10..5698807228eea 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -306,6 +306,6 @@ page at http://checkstyle.sourceforge.net/config.html --> - +     From bdb6cdefa7cbbcb970850f9b8e1d4ac2d4ca6548 Mon Sep 17 00:00:00 2001 From: shafang Date: Fri, 12 Jul 2019 10:42:10 -0700 Subject: [PATCH 32/35] fixes for KV and no other method has prefix build other than buildClient or buildAsyncClient --- .../checks/ServiceClientBuilderCheck.java | 39 ++++--------------- .../checkstyle/checkstyle-suppressions.xml | 3 ++ .../eventhubs/EventHubClientBuilder.java | 2 + .../keyvault/secrets/SecretService.java | 2 +- 4 files changed, 14 insertions(+), 32 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index 6a1bb101f7845..f21ca3568af1b 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -12,8 +12,7 @@ /** * The @ServiceClientBuilder class should have the following rules: * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. - * 2) Has a method 'buildClient()' to build a synchronous client, - * 3) Has a method 'buildAsyncClient()' to build an asynchronous client + * 2) No other method have prefix 'build' other than 'buildClient' or 'buildAsyncClient'. */ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; @@ -22,8 +21,6 @@ public class ServiceClientBuilderCheck extends AbstractCheck { private Stack hasServiceClientBuilderAnnotationStack = new Stack(); private boolean hasServiceClientBuilderAnnotation; - private boolean hasAsyncClientBuilder; - private boolean hasClientBuilder; @Override public int[] getDefaultTokens() { @@ -47,20 +44,8 @@ public int[] getRequiredTokens() { public void leaveToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: - final boolean previousState = hasServiceClientBuilderAnnotationStack.pop(); - - if (!hasServiceClientBuilderAnnotation) { - return; - } - - if (!hasAsyncClientBuilder) { - log(token, String.format("The class annotated with @ServiceClientBuilder requires an asynchronous method, ''%s''", BUILD_ASYNC_CLIENT)); - } - if (!hasClientBuilder) { - log(token, String.format("The class annotated with @ServiceClientBuilder requires a synchronous method, ''%s''", BUILD_CLIENT)); - } // end of CLASS_DEF node, reset the value back to previous state - hasServiceClientBuilderAnnotation = previousState; + hasServiceClientBuilderAnnotation = hasServiceClientBuilderAnnotationStack.pop(); break; default: // Checkstyle complains if there's no default block in switch @@ -72,10 +57,6 @@ public void leaveToken(DetailAST token) { public void visitToken(DetailAST token) { switch (token.getType()) { case TokenTypes.CLASS_DEF: - // for the starting root of every class, reset to false - hasAsyncClientBuilder = false; - hasClientBuilder = false; - // Save the state of variable 'hasServiceClientBuilderAnnotation' to limit the scope of accessibility hasServiceClientBuilderAnnotationStack.push(hasServiceClientBuilderAnnotation); @@ -102,18 +83,14 @@ public void visitToken(DetailAST token) { return; } - // if buildAsyncClient() and buildClient() methods already exist, skip rest of METHOD_DEF checks - if (hasAsyncClientBuilder && hasClientBuilder) { - return; - } - final String methodName = token.findFirstToken(TokenTypes.IDENT).getText(); - if (BUILD_ASYNC_CLIENT.equals(methodName)) { - hasAsyncClientBuilder = true; - } - if (BUILD_CLIENT.equals(methodName)) { - hasClientBuilder = true; + // method name has prefix 'build' but not 'buildClient' or 'buildAsyncClient' + if (methodName.startsWith("build") && !BUILD_ASYNC_CLIENT.equals(methodName) && !BUILD_CLIENT.equals(methodName)) { + log(token, String.format( + "@ServiceClientBuilder class should not have a method name, '''' starting with ''build'' " + + "other than ''buildClient'' or ''buildAsyncClient''." , methodName)); } + break; default: // Checkstyle complains if there's no default block in switch 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 8d16874c0bd00..5acafa820b752 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 @@ -105,4 +105,7 @@ + + + diff --git a/sdk/eventhubs/azure-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubClientBuilder.java b/sdk/eventhubs/azure-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubClientBuilder.java index a336bd7774d43..9e1065c1f3e61 100644 --- a/sdk/eventhubs/azure-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubClientBuilder.java +++ b/sdk/eventhubs/azure-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventHubClientBuilder.java @@ -7,6 +7,7 @@ import com.azure.core.amqp.TransportType; import com.azure.core.credentials.TokenCredential; import com.azure.core.exception.AzureException; +import com.azure.core.implementation.annotation.ServiceClientBuilder; import com.azure.core.implementation.util.ImplUtils; import com.azure.core.util.configuration.BaseConfigurations; import com.azure.core.util.configuration.Configuration; @@ -53,6 +54,7 @@ * * @see EventHubClient */ +@ServiceClientBuilder(serviceClients = EventHubClientBuilder.class) public class EventHubClientBuilder { private static final String AZURE_EVENT_HUBS_CONNECTION_STRING = "AZURE_EVENT_HUBS_CONNECTION_STRING"; diff --git a/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java b/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java index e49c88b9b2146..1d6aa3d2cf773 100644 --- a/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java +++ b/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java @@ -38,7 +38,7 @@ * This is package-private so that these REST calls are transparent to the user. */ @Host("{url}") -@ServiceInterface(name = "KeyVaultSecrets") +@ServiceInterface(name = "KVSecrets") interface SecretService { @Put("secrets/{secret-name}") From 408b77c144684abca0eefc3f89919e4314c2cb5f Mon Sep 17 00:00:00 2001 From: shafang Date: Thu, 18 Jul 2019 13:25:33 -0700 Subject: [PATCH 33/35] starting with build and ending with Client --- .../checks/ServiceClientBuilderCheck.java | 35 ++++++++++--------- .../checks/ServiceInterfaceCheck.java | 9 ++--- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java index f21ca3568af1b..8623828a9c63e 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientBuilderCheck.java @@ -12,15 +12,15 @@ /** * The @ServiceClientBuilder class should have the following rules: * 1) All service client builder should be named ClientBuilder and annotated with @ServiceClientBuilder. - * 2) No other method have prefix 'build' other than 'buildClient' or 'buildAsyncClient'. + * 2) No other method have prefix 'build' other than 'build*Client' or 'build*AsyncClient'. */ public class ServiceClientBuilderCheck extends AbstractCheck { private static final String SERVICE_CLIENT_BUILDER = "ServiceClientBuilder"; - private static final String BUILD_CLIENT = "buildClient"; - private static final String BUILD_ASYNC_CLIENT = "buildAsyncClient"; private Stack hasServiceClientBuilderAnnotationStack = new Stack(); + private Stack hasBuildMethodStack = new Stack<>(); private boolean hasServiceClientBuilderAnnotation; + private boolean hasBuildMethod; @Override public int[] getDefaultTokens() { @@ -42,14 +42,12 @@ public int[] getRequiredTokens() { @Override public void leaveToken(DetailAST token) { - switch (token.getType()) { - case TokenTypes.CLASS_DEF: - // end of CLASS_DEF node, reset the value back to previous state - hasServiceClientBuilderAnnotation = hasServiceClientBuilderAnnotationStack.pop(); - break; - default: - // Checkstyle complains if there's no default block in switch - break; + if (token.getType() == TokenTypes.CLASS_DEF) { + hasServiceClientBuilderAnnotation = hasServiceClientBuilderAnnotationStack.pop(); + hasBuildMethod = hasBuildMethodStack.pop(); + if (hasServiceClientBuilderAnnotation && !hasBuildMethod) { + log(token, "Class with @ServiceClientBuilder annotation must have a method starting with ''build'' and ending with ''Client''."); + } } } @@ -59,7 +57,7 @@ public void visitToken(DetailAST token) { case TokenTypes.CLASS_DEF: // Save the state of variable 'hasServiceClientBuilderAnnotation' to limit the scope of accessibility hasServiceClientBuilderAnnotationStack.push(hasServiceClientBuilderAnnotation); - + hasBuildMethodStack.push(hasBuildMethod); final DetailAST serviceClientAnnotationBuilderToken = getServiceClientBuilderAnnotation(token); final String className = token.findFirstToken(TokenTypes.IDENT).getText(); @@ -84,13 +82,16 @@ public void visitToken(DetailAST token) { } final String methodName = token.findFirstToken(TokenTypes.IDENT).getText(); - // method name has prefix 'build' but not 'buildClient' or 'buildAsyncClient' - if (methodName.startsWith("build") && !BUILD_ASYNC_CLIENT.equals(methodName) && !BUILD_CLIENT.equals(methodName)) { - log(token, String.format( - "@ServiceClientBuilder class should not have a method name, '''' starting with ''build'' " + - "other than ''buildClient'' or ''buildAsyncClient''." , methodName)); + if (!methodName.startsWith("build")) { + break; } + hasBuildMethod = true; + // method name has prefix 'build' but not 'build*Client' or 'build*AsyncClient' + if (!methodName.endsWith("Client")) { + log(token, String.format( + "@ServiceClientBuilder class should not have a method name, ''%s'' starting with ''build'' but not ending with ''Client''." , methodName)); + } break; default: // Checkstyle complains if there's no default block in switch diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index f7e6e1416577d..ee07430fd2aae 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -34,13 +34,8 @@ public int[] getRequiredTokens() { @Override public void visitToken(DetailAST token) { - switch (token.getType()) { - case TokenTypes.INTERFACE_DEF: - checkServiceInterface(token); - break; - default: - // Checkstyle complains if there's no default block in switch - break; + if (token.getType() == TokenTypes.INTERFACE_DEF) { + checkServiceInterface(token); } } From e3842ad3a839348aae7515dc5cbe28a7083b5f7a Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 23 Jul 2019 09:40:35 -0700 Subject: [PATCH 34/35] revert kv changes and update limit character to 20 --- .../azure/tools/checkstyle/checks/ServiceInterfaceCheck.java | 4 ++-- .../com/azure/security/keyvault/secrets/SecretService.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index ee07430fd2aae..3c4967ada6191 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -81,8 +81,8 @@ private void checkServiceInterface(DetailAST interfaceDefToken) { } // 'name' is required at @ServiceInterface - // 'name' should not be empty, no Space allowed and the length should less than or equal to 10 characters - Pattern serviceNamePattern = Pattern.compile("^[a-zA-Z0-9]{1,10}$"); + // 'name' should not be empty, no Space allowed and the length should less than or equal to 20 characters + Pattern serviceNamePattern = Pattern.compile("^[a-zA-Z0-9]{1,20}$"); if (!serviceNamePattern.matcher(nameValue).find()) { log(serviceInterfaceAnnotationNode, String.format( "The ''name'' property of @ServiceInterface, ''%s'' should be non-empty, alphanumeric and not more than 10 characters", diff --git a/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java b/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java index e9ec9aa6a957a..a686836effc66 100644 --- a/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java +++ b/sdk/keyvault/azure-keyvault-secrets/src/main/java/com/azure/security/keyvault/secrets/SecretService.java @@ -39,7 +39,7 @@ * This is package-private so that these REST calls are transparent to the user. */ @Host("{url}") -@ServiceInterface(name = "KVSecrets") +@ServiceInterface(name = "KeyVaultSecrets") interface SecretService { @Put("secrets/{secret-name}") From 4b98e06c7d3ec4d9487a48b022aa08bdb2d9974b Mon Sep 17 00:00:00 2001 From: shafang Date: Tue, 23 Jul 2019 09:42:23 -0700 Subject: [PATCH 35/35] updates comments --- .../azure/tools/checkstyle/checks/ServiceInterfaceCheck.java | 4 ++-- .../src/main/resources/checkstyle/checkstyle.xml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java index 3c4967ada6191..3ac00ff0cd08a 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceInterfaceCheck.java @@ -12,7 +12,7 @@ /** * The @ServiceInterface class should have the following rules: * 1) The annotation property 'name' should be non-empty - * 2) The length of value of property 'name' should be less than 10 characters and without space + * 2) The length of value of property 'name' should be less than 20 characters and without space */ public class ServiceInterfaceCheck extends AbstractCheck { @Override @@ -42,7 +42,7 @@ public void visitToken(DetailAST token) { /** * The @ServiceInterface class should have the following rules: * 1) The annotation property 'name' should be non-empty - * 2) The length of value of property 'name' should be less than 10 characters and without space + * 2) The length of value of property 'name' should be less than 20 characters and without space * * @param interfaceDefToken INTERFACE_DEF AST node */ 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 5698807228eea..41e4303d4378c 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -301,7 +301,7 @@ page at http://checkstyle.sourceforge.net/config.html --> + 2) The length of value of property 'name' should be less than 20 characters and without space -->