diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java index 43a6c10223..543fc4239f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java @@ -3,15 +3,12 @@ import static com.google.errorprone.BugPattern.LinkType.NONE; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; -import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL; import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.annotations; import static com.google.errorprone.matchers.Matchers.anyOf; -import static com.google.errorprone.matchers.Matchers.isSameType; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.isType; -import static com.google.errorprone.matchers.Matchers.methodHasParameters; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableCollection; @@ -19,60 +16,30 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.MethodTree; +import com.sun.source.tree.VariableTree; -/** - * A {@link BugChecker} which flags {@code RequestParam} parameters that have an invalid type. - * - *

Types considered invalid are {@link ImmutableMap} and subtypes of {@link ImmutableCollection}. - */ +/** A {@link BugChecker} which flags {@code @RequestParam} parameters with an unsupported type. */ @AutoService(BugChecker.class) @BugPattern( - name = "RequestParamAnnotationCheck", - summary = "Make sure all `@RequestParam` method parameters are valid", + name = "RequestParamAnnotation", + summary = "`@RequestParam` does not support `ImmutableCollection` and `ImmutableMap` subtypes", linkType = NONE, severity = ERROR, tags = LIKELY_ERROR) -public final class RequestParamAnnotationCheck extends BugChecker implements MethodTreeMatcher { +public final class RequestParamAnnotationCheck extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; - private static final String ANN_PACKAGE_PREFIX = "org.springframework.web.bind.annotation."; - - private static final Matcher HAS_REQUEST_MAPPING_ANNOTATION = + private static final Matcher HAS_UNSUPPORTED_REQUEST_PARAM = allOf( - annotations( - ALL, - anyOf( - isType(ANN_PACKAGE_PREFIX + "DeleteMapping"), - isType(ANN_PACKAGE_PREFIX + "GetMapping"), - isType(ANN_PACKAGE_PREFIX + "PatchMapping"), - isType(ANN_PACKAGE_PREFIX + "PostMapping"), - isType(ANN_PACKAGE_PREFIX + "PutMapping"), - isType(ANN_PACKAGE_PREFIX + "RequestMapping"))), - methodHasParameters( - AT_LEAST_ONE, - annotations(AT_LEAST_ONE, isType(ANN_PACKAGE_PREFIX + "RequestParam")))); - - private static final Matcher HAS_INVALID_REQUEST_PARAM_TYPE = - methodHasParameters( - AT_LEAST_ONE, - allOf( - annotations(ALL, isType(ANN_PACKAGE_PREFIX + "RequestParam")), - anyOf( - isSubtypeOf("com.google.common.collect.ImmutableCollection"), - isSameType("com.google.common.collect.ImmutableMap")))); + annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")), + anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class))); @Override - public Description matchMethod(MethodTree tree, VisitorState state) { - return HAS_REQUEST_MAPPING_ANNOTATION.matches(tree, state) - && HAS_INVALID_REQUEST_PARAM_TYPE.matches(tree, state) - ? buildDescription(tree) - .setMessage( - "At least one defined Request Parameter has an invalid type. " - + "`ImmutableMap`, `ImmutableCollection` and subtypes are not allowed.") - .build() + public Description matchVariable(VariableTree tree, VisitorState state) { + return HAS_UNSUPPORTED_REQUEST_PARAM.matches(tree, state) + ? describeMatch(tree) : Description.NO_MATCH; } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheckTest.java index 9d85f62327..0ddc057205 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheckTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheckTest.java @@ -12,12 +12,14 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", + "import com.google.common.collect.ImmutableBiMap;", "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableMap;", "import com.google.common.collect.ImmutableSet;", "import java.util.List;", "import java.util.Map;", "import java.util.Set;", + "import javax.annotation.Nullable;", "import org.springframework.web.bind.annotation.DeleteMapping;", "import org.springframework.web.bind.annotation.GetMapping;", "import org.springframework.web.bind.annotation.PostMapping;", @@ -27,13 +29,16 @@ void identification() { "", "interface A {", " @PostMapping A properRequestParam(@RequestBody String body);", - " @GetMapping A properRequestParam(@RequestParam Integer param);", + " @GetMapping A properRequestParam(@RequestParam int param);", " @GetMapping A properRequestParam(@RequestParam List param);", " @PostMapping A properRequestParam(@RequestBody String body, @RequestParam Set param);", " @PutMapping A properRequestParam(@RequestBody String body, @RequestParam Map param);", "", " // BUG: Diagnostic contains:", - " @PostMapping A post(@RequestParam ImmutableList param);", + " @GetMapping A get(@RequestParam ImmutableBiMap param);", + "", + " // BUG: Diagnostic contains:", + " @PostMapping A post(@Nullable @RequestParam ImmutableList param);", "", " // BUG: Diagnostic contains:", " @PutMapping A put(@RequestBody String body, @RequestParam ImmutableSet param);",