From 66fd3cd33f20f031abb2296df568a7abd97308f0 Mon Sep 17 00:00:00 2001 From: Shawn Fang <45607042+mssfang@users.noreply.github.com> Date: Wed, 28 Aug 2019 16:02:35 -0700 Subject: [PATCH] Custom CheckStyle: Batch 2, Rule #5 and #10; Batch 3, Rule #1 and #6 (#4712) Batch 2 Rule # 5 Service client methods: All methods that are in a class annotated with @ServiceClient, where the method has a @ServiceMethod annotation, should follow these rules: Methods names should follow a common vocabulary. Refer to Java spec for this naming pattern https://azure.github.io/azure-sdk/java_design.html. Method names imply certain rules around expected return type - these should all be validated. Methods should not have 'Async' added to their method name. Return types of async and sync clients should be as per guidelines: [this check will be ignored for now since I am still struggling on how to get the Reflection working in the code-quality-check] Return type for async collection should be of type ? extends PagedFlux Return type for async single value should be of type ? extends Mono Return type for sync collection should be of type ? extends PagedIterable Return type for sync single value should be of type ? extends Response Rule # 10 'withResponse' naming pattern: All methods annotated with @ServiceMethod that return a Response (or Mono) must have their method name end with 'withResponse'. If the service method does not return a Response or Mono, it must not end with 'withResponse'. Batch 3 Rule # 1 Context in all the right places: Context should be passed in as an argument to all public methods annotated with @ServiceMethod that return Response in sync clients. Only in the sync case: E.g. we want this: Public Response getFooWithResponse(int x, int y, Context c) In the async case, we should check to ensure we have no service methods that take Context! Rule # 6 Async client should have async = true property set in @ServiceClient annotation To validate this, if the class has @ServiceClient annotation and if the classname AsyncClient, verify that the async property of @ServiceClient annotation is set to true. Similarly, if the class has @ServiceClient annotation and if the classname is Client, verify that the async property of @ServiceClient annotation is set to false Change class Context to be final --- .../checks/FluentMethodNameCheck.java | 8 +- .../checkstyle/checks/ServiceClientCheck.java | 499 ++++++++++++++++++ .../checks/ServiceClientChecks.java | 179 ------- .../ServiceClientInstantiationCheck.java | 231 -------- .../checkstyle/checkstyle-suppressions.xml | 14 +- .../main/resources/checkstyle/checkstyle.xml | 27 +- 6 files changed, 529 insertions(+), 429 deletions(-) create mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientCheck.java delete mode 100644 eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientChecks.java 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/FluentMethodNameCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/FluentMethodNameCheck.java index bae5bef8404dc..5c2cca7ca2ab6 100644 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/FluentMethodNameCheck.java +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/FluentMethodNameCheck.java @@ -129,10 +129,10 @@ private boolean isFluentMethod(DetailAST methodDefToken) { final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); // If no @Fluent annotated with this class, return false return TokenUtil.findFirstTokenByPredicate(modifiersToken, - annotationToken -> annotationToken.getType() == TokenTypes.ANNOTATION - && TokenUtil.findFirstTokenByPredicate(annotationToken, - identToken -> identToken.getType() == TokenTypes.IDENT && - "Fluent".equals(identToken.getText())).isPresent()) + annotationToken -> annotationToken.getType() == TokenTypes.ANNOTATION + && TokenUtil.findFirstTokenByPredicate(annotationToken, + identToken -> identToken.getType() == TokenTypes.IDENT + && "Fluent".equals(identToken.getText())).isPresent()) .isPresent(); } } diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientCheck.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientCheck.java new file mode 100644 index 0000000000000..c7c6fc297529c --- /dev/null +++ b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientCheck.java @@ -0,0 +1,499 @@ +// 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 com.puppycrawl.tools.checkstyle.utils.TokenUtil; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +/** + * 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. Follows method naming pattern. Refer to Java Spec:
    2. + *
  2. Methods should not have "Async" added to the method name
    3. + *
  3. Return type of async and sync clients should be as per guidelines: + *
      + *
    1. The return type for async collection should be of type that extends PagedFlux
      2. + *
    2. The return type for async single value should be of type that extends Mono
      3. + *
    3. The return type for sync collection should be of type that extends PagedIterable
      4. + *
    4. The return type for sync single value should be of type that extends Response
      5. + *
    + *
    4. + *
+ */ +public class ServiceClientCheck extends AbstractCheck { + private static final String ASYNC = "Async"; + 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 final String CONTEXT = "Context"; + + private static final String RESPONSE_BRACKET = "Response<"; + private static final String MONO_BRACKET = "Mono<"; + private static final String MONO_RESPONSE_BRACKET = "Mono COMMON_NAMING_PREFIX_SET = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( + "upsert", "set", "create", "update", "replace", "delete", "add", "get", "list" + ))); + + // Add all imported classes into a map, key is the name of class and value is the full package path of class. + private final Map simpleClassNameToQualifiedNameMap = new HashMap<>(); + + private boolean isAsync; + private boolean isServiceClientAnnotation; + + @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 + }; + } + + @Override + public void beginTree(DetailAST root) { + isServiceClientAnnotation = false; + isAsync = false; + } + + @Override + public void visitToken(DetailAST token) { + switch (token.getType()) { + case TokenTypes.IMPORT: + addImportedClassPath(token); + break; + case TokenTypes.CLASS_DEF: + isServiceClientAnnotation = hasServiceClientAnnotation(token); + if (!isServiceClientAnnotation) { + return; + } + checkServiceClientNaming(token); + break; + case TokenTypes.CTOR_DEF: + if (!isServiceClientAnnotation) { + return; + } + checkConstructor(token); + break; + case TokenTypes.METHOD_DEF: + if (!isServiceClientAnnotation) { + return; + } + checkMethodNameBuilder(token); + checkMethodNamingPattern(token); + break; + case TokenTypes.OBJBLOCK: + if (!isServiceClientAnnotation) { + return; + } + checkClassField(token); + break; + default: + // Checkstyle complains if there's no default block in switch + break; + } + } + + /** + * 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 checkMethodNameBuilder(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) { + final Optional varDefTokenOption = TokenUtil.findFirstTokenByPredicate(objBlockToken, node -> + node.getType() == TokenTypes.VARIABLE_DEF && !node.findFirstToken(TokenTypes.MODIFIERS).branchContains(TokenTypes.FINAL)); + if (varDefTokenOption.isPresent()) { + final DetailAST varDefToken = varDefTokenOption.get(); + final String varName = varDefToken.findFirstToken(TokenTypes.IDENT).getText(); + log(varDefToken, String.format("The variable field ''%s'' of class ''%s'' should be final. Classes " + + "annotated with @ServiceClient are supposed to be immutable.", varName, 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 client must be named AsyncClient, and Sync client must be named Client + if (isAsync && !className.endsWith(ASYNC_CLIENT)) { + log(classDefToken, String.format("Asynchronous class ''%s'' must be named AsyncClient, which " + + "concatenates by service name and a fixed word 'AsyncClient'.", className)); + } else if (!isAsync && !className.endsWith(CLIENT)) { + log(classDefToken, String.format("Synchronous class %s must be named Client," + + " which concatenates by service name and a fixed word 'Client'.", className)); + } + + // Class named AsyncClient, the property 'isAsync' must set to true + // Class named Client, the property 'isAsync' must to be false or use the default value + if (className.endsWith(ASYNC_CLIENT) && !isAsync) { + log(classDefToken, String.format("class ''%s'' is an asynchronous client, must set property ''%s'' to true.", className, IS_ASYNC)); + } else if (className.endsWith(CLIENT) && !className.endsWith(ASYNC_CLIENT) && isAsync) { + log(classDefToken, String.format("class ''%s'' is a synchronous client, must set property ''%s'' to false or without the property.", className, IS_ASYNC)); + } + } + + /** + * 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) 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 PagedFlux. + * 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 PagedIterable. + * 3.4) The return type for sync single value should be of type? extends Response. + * 4) Naming pattern for 'WithResponse'. + * 5) Synchronous method with annotation @ServiceMethod has to have {@code Context} as a parameter. + * Asynchronous method with annotation @ServiceMethod must not has {@code Context} as a parameter. + * + * @param methodDefToken METHOD_DEF AST node + */ + private void checkMethodNamingPattern(DetailAST methodDefToken) { + final DetailAST modifiersToken = methodDefToken.findFirstToken(TokenTypes.MODIFIERS); + final Optional serviceMethodAnnotationOption = TokenUtil.findFirstTokenByPredicate(modifiersToken, + node -> { + if (node.getType() != TokenTypes.ANNOTATION) { + return false; + } + final DetailAST annotationIdentToken = node.findFirstToken(TokenTypes.IDENT); + return annotationIdentToken != null && "ServiceMethod".equals(annotationIdentToken.getText()); + }); + // NOT a @ServiceMethod method + if (!serviceMethodAnnotationOption.isPresent()) { + return; + } + + final DetailAST serviceMethodAnnotation = serviceMethodAnnotationOption.get(); + final String methodName = methodDefToken.findFirstToken(TokenTypes.IDENT).getText(); + + // 1) Follows method naming pattern. Refer to Java Spec. + // prefix of method name that contains all lower letters + final String prefix = methodName.split("[A-Z]", 2)[0]; + if (!methodName.endsWith("Exists") && !COMMON_NAMING_PREFIX_SET.contains(prefix)) { + log(methodDefToken, String.format("Method name ''%s'' should follow a common vocabulary. Refer to Java Spec: %s.", + methodName, JAVA_SPEC_LINK)); + } + + // 2) Methods should not have "Async" added to the method name + if (methodName.contains(ASYNC)) { + log(methodDefToken, String.format("Method name ''%s'' should not contain ''%s'' in the method name.", + methodName, ASYNC)); + } + + // 3) The return type of async and sync clients should be as per guidelines + checkServiceClientMethodReturnType(methodDefToken, serviceMethodAnnotation, methodName); + + // 4) Check 'withResponse' naming pattern + checkReturnTypeNamingPattern(methodDefToken, methodName); + + // 5) Synchronous method with annotation @ServiceMethod has to have {@code Context} as a parameter. + // Asynchronous method with annotation @ServiceMethod must not has {@code Context} as a parameter. + checkContextInRightPlace(methodDefToken); + } + + /** + * Checks for the return type of async and sync clients should be as per guidelines: + * 1) The return type for async collection should be of type? extends PagedFlux + * 2) The return type for async single value should be of type? extends Mono + * 3) The return type for sync collection should be of type? extends PagedIterable + * 4) The return type for sync single value should be of type? extends Response + * + * @param methodDefToken METHOD_DEF AST node + * @param serviceMethodAnnotation ANNOTATION AST node which used to find the if the annotation has 'return' key, + * @param methodName method name + * if found. return the value of member'return'. + */ + private void checkServiceClientMethodReturnType(DetailAST methodDefToken, DetailAST serviceMethodAnnotation, String methodName) { + // Find the annotation member 'returns' value + String returnsAnnotationMemberValue = null; + final Optional returnValueOption = TokenUtil.findFirstTokenByPredicate(serviceMethodAnnotation, node -> + node.getType() == TokenTypes.ANNOTATION_MEMBER_VALUE_PAIR && node.findFirstToken(TokenTypes.IDENT).getText().equals("returns") + && !FullIdent.createFullIdentBelow(node.findFirstToken(TokenTypes.EXPR)).getText().isEmpty()); + + if (returnValueOption.isPresent()) { + returnsAnnotationMemberValue = FullIdent.createFullIdentBelow(returnValueOption.get().findFirstToken(TokenTypes.EXPR)).getText(); + } + + final String returnType = getReturnType(methodDefToken.findFirstToken(TokenTypes.TYPE), new StringBuilder()).toString(); + + if (isAsync) { + if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + // If value of 'returns' is SINGLE, and then log error if the return type of the method is not start with {@code Mono} + if (!returnType.startsWith(MONO_BRACKET)) { + log(methodDefToken, String.format(RETURN_TYPE_ERROR, "Asynchronous", SINGLE_RETURN_TYPE, MONO)); + } + } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + // If value of 'returns' is COLLECTION, and then log error if the return type of the method is not start with {@code PagedFlux} + if (!returnType.startsWith(PAGED_FLUX_BRACKET)) { + log(methodDefToken, String.format(RETURN_TYPE_ERROR, "Asynchronous", COLLECTION_RETURN_TYPE, PAGED_FLUX)); + } + } + } else { + if (SINGLE_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + // If value of 'returns' is SINGLE, and then log error if the return type of the method is not start + // with {@code Response} when the method name ends with 'WithResponse'. + if ((returnType.startsWith(RESPONSE_BRACKET) && !methodName.endsWith(WITH_RESPONSE)) + || (!returnType.startsWith(RESPONSE_BRACKET) && methodName.endsWith(WITH_RESPONSE))) { + log(methodDefToken, String.format(RESPONSE_METHOD_NAME_ERROR, "Synchronous", SINGLE_RETURN_TYPE, + RESPONSE, WITH_RESPONSE, WITH_RESPONSE, RESPONSE)); + } + } else if (COLLECTION_RETURN_TYPE.equals(returnsAnnotationMemberValue)) { + // If value of 'returns' is COLLECTION, and then log error if the return type of the method is not start with {@code PagedIterable} + if (!returnType.startsWith(PAGED_ITERABLE_BRACKET)) { + log(methodDefToken, String.format(RETURN_TYPE_ERROR, "Synchronous", COLLECTION_RETURN_TYPE, PAGED_ITERABLE)); + } + } + } + } + + /** + * Given the method is already annotated @ServiceMethod. Checks if the return type is {@code Response} or + * {@code Mono>}, + * Sync: + * If the return type is Response, the method name must end with WithResponse. + * If the return type is T, the method name must NOT end with WithResponse. + * Async: + * If the return type is Mono>, the method name must end with WithResponse. + * If the return type is Mono, the method name must NOT end with WithResponse. + * + * @param methodDefToken METHOD_DEF AST node + */ + private void checkReturnTypeNamingPattern(DetailAST methodDefToken, String methodName) { + final DetailAST typeToken = methodDefToken.findFirstToken(TokenTypes.TYPE); + // Use recursion to get the return type + final String returnType = getReturnType(typeToken, new StringBuilder()).toString(); + + if (methodName.endsWith(WITH_RESPONSE)) { + if (!returnType.startsWith(RESPONSE_BRACKET) && !returnType.startsWith(MONO_RESPONSE_BRACKET)) { + log(methodDefToken, String.format(RETURN_TYPE_WITH_RESPONSE_ERROR, returnType, "must not", WITH_RESPONSE)); + } + } else { + if (returnType.startsWith(RESPONSE_BRACKET) || returnType.startsWith(MONO_RESPONSE_BRACKET)) { + log(methodDefToken, String.format(RETURN_TYPE_WITH_RESPONSE_ERROR, returnType, "must", WITH_RESPONSE)); + } + } + } + + /** + * Checks the type Context should be in the right place. Context should be passed in as an argument to all public + * methods annotated with @ServiceMethod that return {@code Response} in synchronous clients. + * Synchronous method with annotation @ServiceMethod has to have {@code Context} as a parameter. + * Asynchronous method with annotation @ServiceMethod must not has {@code Context} as a parameter. + * + * @param methodDefToken METHOD_DEF AST token + */ + private void checkContextInRightPlace(DetailAST methodDefToken) { + final DetailAST parametersToken = methodDefToken.findFirstToken(TokenTypes.PARAMETERS); + final String returnType = getReturnType(methodDefToken.findFirstToken(TokenTypes.TYPE), new StringBuilder()).toString(); + + final boolean containsContextParameter = TokenUtil.findFirstTokenByPredicate(parametersToken, + parameterToken -> { + if (parameterToken.getType() != TokenTypes.PARAMETER_DEF) { + return false; + } + final DetailAST paramTypeIdentToken = parameterToken.findFirstToken(TokenTypes.TYPE).findFirstToken(TokenTypes.IDENT); + return paramTypeIdentToken != null && CONTEXT.equals(paramTypeIdentToken.getText()); + }) + .isPresent(); + + if (containsContextParameter) { + // MONO and PagedFlux return type implies Asynchronous method + if (returnType.startsWith(MONO_BRACKET) || returnType.startsWith(PAGED_FLUX_BRACKET)) { + log(methodDefToken, String.format(ASYNC_CONTEXT_ERROR, CONTEXT)); + } + } else { + // Context should be passed in as an argument to all public methods annotated with @ServiceMethod that + // return Response in sync clients. + if (returnType.startsWith(RESPONSE_BRACKET)) { + log(methodDefToken, String.format(SYNC_CONTEXT_ERROR, CONTEXT)); + } + } + } + + /** + * 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); + final Optional serviceClientAnnotationOption = TokenUtil.findFirstTokenByPredicate(modifiersToken, + node -> { + if (node.getType() != TokenTypes.ANNOTATION) { + return false; + } + final DetailAST annotationIdentToken = node.findFirstToken(TokenTypes.IDENT); + return annotationIdentToken != null && SERVICE_CLIENT.equals(annotationIdentToken.getText()); + } + ); + if (serviceClientAnnotationOption.isPresent()) { + isAsync = isAsyncServiceClient(serviceClientAnnotationOption.get()); + return true; + } + // If no @ServiceClient annotated with this class, return false + return false; + } + + /** + * 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); + } + + /** + * 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; + } + + /** + * Get full name of return type. Such as Response, Mono. + * + * @param token a token could be a TYPE, TYPE_ARGUMENT, TYPE_ARGUMENTS token + * @param sb a StringBuilder that used to collect method return type. + */ + private StringBuilder getReturnType(DetailAST token, StringBuilder sb) { + for (DetailAST currentToken = token.getFirstChild(); currentToken != null; currentToken = currentToken.getNextSibling()) { + switch (currentToken.getType()) { + case TokenTypes.TYPE_ARGUMENT: + case TokenTypes.TYPE_ARGUMENTS: + // Recursive call + getReturnType(currentToken, sb); + break; + default: + sb.append(currentToken.getText()); + } + } + return sb; + } +} diff --git a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientChecks.java b/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientChecks.java deleted file mode 100644 index d5e15ce5f1f16..0000000000000 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientChecks.java +++ /dev/null @@ -1,179 +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.utils.TokenUtil; - -/** - * Verifies that subclasses of ServiceClient meet a set of guidelines. - *
    - *
  1. No public or protected constructors
    2. - *
  2. Implements a public static method named builder
    3. - *
- */ -public class ServiceClientChecks extends AbstractCheck { - private static final String BUILDER_METHOD_NAME = "builder"; - private static final String SERVICE_CLIENT_CLASS_NAME = "com.azure.common.ServiceClient"; - - private static final String FAILED_TO_LOAD_MESSAGE = "%s class failed to load, ServiceClientChecks will be ignored."; - private static final String CONSTRUCTOR_ERROR_MESSAGE = "Descendants of ServiceClient cannot have public or protected constructors."; - private static final String BUILDER_ERROR_MESSAGE = "Descendants of ServiceClient must have a static method named builder."; - - private static final int[] TOKENS = new int[] { - TokenTypes.PACKAGE_DEF, - TokenTypes.CTOR_DEF, - TokenTypes.METHOD_DEF - }; - - private Class serviceClientClass; - private boolean extendsServiceClient; - private boolean hasStaticBuilder; - - @Override - public int[] getDefaultTokens() { - return getRequiredTokens(); - } - - @Override - public int[] getAcceptableTokens() { - return getRequiredTokens(); - } - - /** - * Array of tokens that trigger visitToken when the TreeWalker is traversing the AST. - * @return The list of tokens that trigger visitToken. - */ - @Override - public int[] getRequiredTokens() { - return TOKENS; - } - - @Override - public void init() { - try { - this.serviceClientClass = Class.forName(SERVICE_CLIENT_CLASS_NAME); - } catch (ClassNotFoundException ex) { - log(0, String.format(FAILED_TO_LOAD_MESSAGE, "ServiceClient")); - } - } - - /** - * Start of the TreeWalker traversal. - * @param rootAST Root of the AST. - */ - @Override - public void beginTree(DetailAST rootAST) { - this.extendsServiceClient = false; - this.hasStaticBuilder = false; - } - /** - * Processes a token from the required tokens list when the TreeWalker visits it. - * @param token Node in the AST. - */ - @Override - public void visitToken(DetailAST token) { - // Failed to load ServiceClient's class, don't validate anything. - if (this.serviceClientClass == null) { - return; - } - - switch (token.getType()) { - case TokenTypes.PACKAGE_DEF: - this.extendsServiceClient = extendsServiceClient(token); - break; - case TokenTypes.CTOR_DEF: - if (this.extendsServiceClient && visibilityIsPublicOrProtected(token)) { - log(token, CONSTRUCTOR_ERROR_MESSAGE); - } - break; - case TokenTypes.METHOD_DEF: - if (this.extendsServiceClient && !this.hasStaticBuilder && methodIsStaticBuilder(token)) { - this.hasStaticBuilder = true; - } - break; - default: - // Checkstyle complains if there's no default block in switch - break; - } - } - - /** - * End of the TreeWalker traversal. - * @param rootAST Root of the AST. - */ - @Override - public void finishTree(DetailAST rootAST) { - if (this.extendsServiceClient && !this.hasStaticBuilder) { - log(0, BUILDER_ERROR_MESSAGE); - } - } - - /** - * Determines if the class extends ServiceClient. - * @param packageDefinitionToken Package definition token. - * @return True if the package is not in "com.microsoft", the file is a class definition, and the class extends ServiceClient. - */ - private boolean extendsServiceClient(DetailAST packageDefinitionToken) { - String packageName = FullIdent.createFullIdent(packageDefinitionToken.findFirstToken(TokenTypes.DOT)).getText(); - if (packageName.startsWith("com.microsoft")) { - return false; - } - - DetailAST classDefinitionToken = packageDefinitionToken.findFirstToken(TokenTypes.CLASS_DEF); - if (classDefinitionToken == null) { - return false; - } - - String className = classDefinitionToken.findFirstToken(TokenTypes.IDENT).getText(); - try { - Class clazz = Class.forName(packageName + "." + className); - - return this.serviceClientClass.isAssignableFrom(clazz); - } catch (ClassNotFoundException ex) { - log(classDefinitionToken, String.format(FAILED_TO_LOAD_MESSAGE, className)); - return false; - } - } - - /** - * Checks if the constructor is using the public or protected scope. - * @param constructorToken Construction token. - * @return True if the constructor has a public or protected modifier token. - */ - private boolean visibilityIsPublicOrProtected(DetailAST constructorToken) { - DetailAST modifierToken = constructorToken.findFirstToken(TokenTypes.MODIFIERS); - - // No modifiers means package private. - if (modifierToken == null) { - return false; - } - - return TokenUtil.findFirstTokenByPredicate(modifierToken, - node -> node.getType() == TokenTypes.LITERAL_PUBLIC || node.getType() == TokenTypes.LITERAL_PROTECTED) - .isPresent(); - } - - /** - * Checks if the method node is public static and named builder. - * @param methodToken Method node - * @return True if the method is public static and is named builder - */ - private boolean methodIsStaticBuilder(DetailAST methodToken) { - DetailAST modifierToken = methodToken.findFirstToken(TokenTypes.MODIFIERS); - if (modifierToken == null) { - return false; - } - - if (modifierToken.findFirstToken(TokenTypes.LITERAL_STATIC) == null - || modifierToken.findFirstToken(TokenTypes.LITERAL_PUBLIC) == null) { - return false; - } - - return methodToken.findFirstToken(TokenTypes.IDENT).getText().equals(BUILDER_METHOD_NAME); - } -} 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 02669178d0ca1..0000000000000 --- a/eng/code-quality-reports/src/main/java/com/azure/tools/checkstyle/checks/ServiceClientInstantiationCheck.java +++ /dev/null @@ -1,231 +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; - -/** - * 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; - } -} 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 e97bae6a888b9..5f006328f2810 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 @@ -32,7 +32,7 @@ - + @@ -98,11 +98,11 @@ - - @@ -145,4 +145,12 @@ + + + + + + + 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 8f90edbdd7e24..38b93695b88b4 100755 --- a/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml +++ b/eng/code-quality-reports/src/main/resources/checkstyle/checkstyle.xml @@ -276,14 +276,7 @@ page at http://checkstyle.sourceforge.net/config.html --> - - - - - @@ -292,8 +285,18 @@ 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 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) 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 PagedFlux + 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 PagedIterable + 3.4) The return type for sync single value should be of type? extends Response --> + parameter name or return --> - + - +