Suggestions

PicnicSupermarket · Apr 11, 2022 · f405976 · f405976
1 parent 5d00e71
commit f405976
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 47 deletions.
diff --git a/...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java b/...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java
@@ -3,76 +3,43 @@
 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;
 import com.google.common.collect.ImmutableMap;
 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.
- *
- * <p>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<MethodTree> HAS_REQUEST_MAPPING_ANNOTATION =
+  private static final Matcher<VariableTree> 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<MethodTree> 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/...rib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheckTest.java b/...rib/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<String> param);",
             "  @PostMapping A properRequestParam(@RequestBody String body, @RequestParam Set<String> param);",
             "  @PutMapping A properRequestParam(@RequestBody String body, @RequestParam Map<String, String> param);",
             "",
             "  // BUG: Diagnostic contains:",
-            "  @PostMapping A post(@RequestParam ImmutableList<String> param);",
+            "  @GetMapping A get(@RequestParam ImmutableBiMap<String, String> param);",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  @PostMapping A post(@Nullable @RequestParam ImmutableList<String> param);",
             "",
             "  // BUG: Diagnostic contains:",
             "  @PutMapping A put(@RequestBody String body, @RequestParam ImmutableSet<String> param);",