From 621b84d49edeea38858a68cd040940c07a26ec77 Mon Sep 17 00:00:00 2001 From: Chamil Prabodha Date: Wed, 28 Dec 2022 10:45:14 +0100 Subject: [PATCH 1/3] Introduce additional arguments to `RequestParamType` --- .../bugpatterns/RequestParamType.java | 59 ++++++++-- .../bugpatterns/RequestParamTypeTest.java | 104 ++++++++++++++++++ 2 files changed, 156 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java index eb076e3867..eada0abe49 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java @@ -1,5 +1,6 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; @@ -9,17 +10,21 @@ import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.isType; +import static com.google.errorprone.matchers.Matchers.not; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableCollection; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; /** A {@link BugChecker} that flags {@code @RequestParam} parameters with an unsupported type. */ @@ -32,18 +37,58 @@ tags = LIKELY_ERROR) public final class RequestParamType extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher HAS_UNSUPPORTED_REQUEST_PARAM = - allOf( - annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")), - anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class))); + private static final String FLAG_PREFIX = "RequestParamType:"; + private static final String INCLUDED_CLASS_FLAG = FLAG_PREFIX + "Includes"; - /** Instantiates a new {@link RequestParamType} instance. */ - public RequestParamType() {} + private final Matcher hasUnsupportedRequestParams; + + /** Instantiates a default {@link RequestParamType} instance. */ + public RequestParamType() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link RequestParamType} instance. + * + * @param flags Any provided command line flags. + */ + public RequestParamType(ErrorProneFlags flags) { + hasUnsupportedRequestParams = createVariableTreeMatcher(flags); + } @Override public Description matchVariable(VariableTree tree, VisitorState state) { - return HAS_UNSUPPORTED_REQUEST_PARAM.matches(tree, state) + return hasUnsupportedRequestParams.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } + + private static Matcher createVariableTreeMatcher(ErrorProneFlags flags) { + return allOf( + annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")), + anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class)), + not(anyOf(getSupportedClasses(includedClassNames(flags))))); + } + + private static ImmutableList includedClassNames(ErrorProneFlags flags) { + return flags.getList(INCLUDED_CLASS_FLAG).map(ImmutableList::copyOf).orElse(ImmutableList.of()); + } + + private static ImmutableList> getSupportedClasses( + ImmutableList inclusions) { + return inclusions.stream() + .filter(inclusion -> !inclusion.isEmpty()) + .map(String::trim) + .map(inclusion -> isSubtypeOf(createClass(inclusion))) + .collect(toImmutableList()); + } + + private static Class createClass(String className) { + try { + return Class.forName(className); + } catch (ClassNotFoundException e) { + throw new IllegalArgumentException( + String.format("Invalid class name '%s' in `%s`", className, INCLUDED_CLASS_FLAG), e); + } + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java index 45d58534c6..cb26871498 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java @@ -1,11 +1,22 @@ package tech.picnic.errorprone.bugpatterns; +import com.google.common.collect.ImmutableList; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; final class RequestParamTypeTest { private final CompilationTestHelper compilationTestHelper = CompilationTestHelper.newInstance(RequestParamType.class, getClass()); + private final CompilationTestHelper restrictedCompilationTestHelper = + CompilationTestHelper.newInstance(RequestParamType.class, getClass()) + .setArgs( + ImmutableList.of( + "-XepOpt:RequestParamType:Includes=com.google.common.collect.ImmutableCollection")); + private final CompilationTestHelper restrictedWithSubTypeCompilationTestHelper = + CompilationTestHelper.newInstance(RequestParamType.class, getClass()) + .setArgs( + ImmutableList.of( + "-XepOpt:RequestParamType:Includes=com.google.common.collect.ImmutableSet")); @Test void identification() { @@ -63,4 +74,97 @@ void identification() { "}") .doTest(); } + + @Test + void identificationOfIncludedClass() { + restrictedCompilationTestHelper + .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 org.jspecify.annotations.Nullable;", + "import org.springframework.web.bind.annotation.DeleteMapping;", + "import org.springframework.web.bind.annotation.GetMapping;", + "import org.springframework.web.bind.annotation.PostMapping;", + "import org.springframework.web.bind.annotation.PutMapping;", + "import org.springframework.web.bind.annotation.RequestBody;", + "import org.springframework.web.bind.annotation.RequestParam;", + "", + "interface A {", + " @PostMapping", + " A properRequestParam(@RequestBody String body);", + "", + " @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);", + "", + " @GetMapping", + " // BUG: Diagnostic contains:", + " A get(@RequestParam ImmutableBiMap param);", + "", + " @PostMapping", + " A post(@Nullable @RequestParam ImmutableList param);", + "", + " @PutMapping", + " A put(@RequestBody String body, @RequestParam ImmutableSet param);", + "", + " @DeleteMapping", + " // BUG: Diagnostic contains:", + " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", + "", + " void negative(ImmutableSet set, ImmutableMap map);", + "}") + .doTest(); + } + + @Test + void identificationOfIncludedSubClass() { + restrictedWithSubTypeCompilationTestHelper + .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 org.jspecify.annotations.Nullable;", + "import org.springframework.web.bind.annotation.DeleteMapping;", + "import org.springframework.web.bind.annotation.GetMapping;", + "import org.springframework.web.bind.annotation.PostMapping;", + "import org.springframework.web.bind.annotation.PutMapping;", + "import org.springframework.web.bind.annotation.RequestBody;", + "import org.springframework.web.bind.annotation.RequestParam;", + "", + "interface A {", + " @GetMapping", + " // BUG: Diagnostic contains:", + " A get(@RequestParam ImmutableBiMap param);", + "", + " @PostMapping", + " // BUG: Diagnostic contains:", + " A post(@Nullable @RequestParam ImmutableList param);", + "", + " @PutMapping", + " A put(@RequestBody String body, @RequestParam ImmutableSet param);", + "", + " @DeleteMapping", + " // BUG: Diagnostic contains:", + " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", + "", + " void negative(ImmutableSet set, ImmutableMap map);", + "}") + .doTest(); + } } From c2e476773404d106cdf91009831b98174f699374 Mon Sep 17 00:00:00 2001 From: Chamil Prabodha Date: Wed, 28 Dec 2022 16:23:08 +0100 Subject: [PATCH 2/3] Add a test for invalid flags --- .../bugpatterns/RequestParamType.java | 2 +- .../bugpatterns/RequestParamTypeTest.java | 60 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java index eada0abe49..c74ca1f329 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java @@ -77,8 +77,8 @@ private static ImmutableList includedClassNames(ErrorProneFlags flags) { private static ImmutableList> getSupportedClasses( ImmutableList inclusions) { return inclusions.stream() - .filter(inclusion -> !inclusion.isEmpty()) .map(String::trim) + .filter(inclusion -> !inclusion.isEmpty()) .map(inclusion -> isSubtypeOf(createClass(inclusion))) .collect(toImmutableList()); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java index cb26871498..e024be4dc0 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java @@ -17,6 +17,9 @@ final class RequestParamTypeTest { .setArgs( ImmutableList.of( "-XepOpt:RequestParamType:Includes=com.google.common.collect.ImmutableSet")); + private final CompilationTestHelper restrictedWithInvalidFlagCompilationTestHelper = + CompilationTestHelper.newInstance(RequestParamType.class, getClass()) + .setArgs(ImmutableList.of("-XepOpt:RequestParamType:Includes=")); @Test void identification() { @@ -167,4 +170,61 @@ void identificationOfIncludedSubClass() { "}") .doTest(); } + + @Test + void identificationOfInvalidFlag() { + restrictedWithInvalidFlagCompilationTestHelper + .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 org.jspecify.annotations.Nullable;", + "import org.springframework.web.bind.annotation.DeleteMapping;", + "import org.springframework.web.bind.annotation.GetMapping;", + "import org.springframework.web.bind.annotation.PostMapping;", + "import org.springframework.web.bind.annotation.PutMapping;", + "import org.springframework.web.bind.annotation.RequestBody;", + "import org.springframework.web.bind.annotation.RequestParam;", + "", + "interface A {", + " @PostMapping", + " A properRequestParam(@RequestBody String body);", + "", + " @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);", + "", + " @GetMapping", + " // BUG: Diagnostic contains:", + " A get(@RequestParam ImmutableBiMap param);", + "", + " @PostMapping", + " // BUG: Diagnostic contains:", + " A post(@Nullable @RequestParam ImmutableList param);", + "", + " @PutMapping", + " // BUG: Diagnostic contains:", + " A put(@RequestBody String body, @RequestParam ImmutableSet param);", + "", + " @DeleteMapping", + " // BUG: Diagnostic contains:", + " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", + "", + " void negative(ImmutableSet set, ImmutableMap map);", + "}") + .doTest(); + } } From 63a13863bd33fbbbe1e5e63fdcb109cf56b7f784 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 1 Jan 2023 14:42:24 +0100 Subject: [PATCH 3/3] Suggestions --- ...cographicalAnnotationAttributeListing.java | 3 +- .../RedundantStringConversion.java | 13 +- .../bugpatterns/RequestParamType.java | 43 ++---- .../errorprone/bugpatterns/util/Flags.java | 25 +++ .../bugpatterns/RequestParamTypeTest.java | 145 ++---------------- .../bugpatterns/util/FlagsTest.java | 31 ++++ 6 files changed, 96 insertions(+), 164 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/Flags.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/FlagsTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java index 28e5f51b40..2bf8cbd739 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java @@ -42,6 +42,7 @@ import java.util.stream.Stream; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.AnnotationAttributeMatcher; +import tech.picnic.errorprone.bugpatterns.util.Flags; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -220,7 +221,7 @@ private static AnnotationAttributeMatcher createAnnotationAttributeMatcher( private static ImmutableList excludedAnnotations(ErrorProneFlags flags) { Set exclusions = new HashSet<>(); - flags.getList(EXCLUDED_ANNOTATIONS_FLAG).ifPresent(exclusions::addAll); + exclusions.addAll(Flags.getList(flags, EXCLUDED_ANNOTATIONS_FLAG)); exclusions.addAll(BLACKLISTED_ANNOTATIONS); return ImmutableList.copyOf(exclusions); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java index fae14e3d36..3fae404499 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java @@ -49,6 +49,7 @@ import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; +import tech.picnic.errorprone.bugpatterns.util.Flags; import tech.picnic.errorprone.bugpatterns.util.MethodMatcherFactory; import tech.picnic.errorprone.bugpatterns.util.SourceCode; @@ -63,9 +64,8 @@ public final class RedundantStringConversion extends BugChecker implements BinaryTreeMatcher, CompoundAssignmentTreeMatcher, MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final String FLAG_PREFIX = "RedundantStringConversion:"; private static final String EXTRA_STRING_CONVERSION_METHODS_FLAG = - FLAG_PREFIX + "ExtraConversionMethods"; + "RedundantStringConversion:ExtraConversionMethods"; @SuppressWarnings("UnnecessaryLambda") private static final Matcher ANY_EXPR = (t, s) -> true; @@ -374,10 +374,9 @@ private static Matcher createConversionMethodMatcher( ErrorProneFlags flags) { // XXX: ErrorProneFlags#getList splits by comma, but method signatures may also contain commas. // For this class methods accepting more than one argument are not valid, but still: not nice. - return flags - .getList(EXTRA_STRING_CONVERSION_METHODS_FLAG) - .map(new MethodMatcherFactory()::create) - .map(m -> anyOf(WELL_KNOWN_STRING_CONVERSION_METHODS, m)) - .orElse(WELL_KNOWN_STRING_CONVERSION_METHODS); + return anyOf( + WELL_KNOWN_STRING_CONVERSION_METHODS, + new MethodMatcherFactory() + .create(Flags.getList(flags, EXTRA_STRING_CONVERSION_METHODS_FLAG))); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java index c74ca1f329..28b9e8d439 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java @@ -24,23 +24,25 @@ import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Suppliers; import com.sun.source.tree.Tree; import com.sun.source.tree.VariableTree; +import tech.picnic.errorprone.bugpatterns.util.Flags; /** A {@link BugChecker} that flags {@code @RequestParam} parameters with an unsupported type. */ @AutoService(BugChecker.class) @BugPattern( - summary = "`@RequestParam` does not support `ImmutableCollection` and `ImmutableMap` subtypes", + summary = + "By default, `@RequestParam` does not support `ImmutableCollection` and `ImmutableMap` subtypes", link = BUG_PATTERNS_BASE_URL + "RequestParamType", linkType = CUSTOM, severity = ERROR, tags = LIKELY_ERROR) public final class RequestParamType extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; - private static final String FLAG_PREFIX = "RequestParamType:"; - private static final String INCLUDED_CLASS_FLAG = FLAG_PREFIX + "Includes"; + private static final String SUPPORTED_CUSTOM_TYPES_FLAG = "RequestParamType:SupportedCustomTypes"; - private final Matcher hasUnsupportedRequestParams; + private final Matcher hasUnsupportedRequestParamType; /** Instantiates a default {@link RequestParamType} instance. */ public RequestParamType() { @@ -53,42 +55,27 @@ public RequestParamType() { * @param flags Any provided command line flags. */ public RequestParamType(ErrorProneFlags flags) { - hasUnsupportedRequestParams = createVariableTreeMatcher(flags); + hasUnsupportedRequestParamType = hasUnsupportedRequestParamType(flags); } @Override public Description matchVariable(VariableTree tree, VisitorState state) { - return hasUnsupportedRequestParams.matches(tree, state) + return hasUnsupportedRequestParamType.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } - private static Matcher createVariableTreeMatcher(ErrorProneFlags flags) { + private static Matcher hasUnsupportedRequestParamType(ErrorProneFlags flags) { return allOf( annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")), anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class)), - not(anyOf(getSupportedClasses(includedClassNames(flags))))); + not(isSubtypeOfAny(Flags.getList(flags, SUPPORTED_CUSTOM_TYPES_FLAG)))); } - private static ImmutableList includedClassNames(ErrorProneFlags flags) { - return flags.getList(INCLUDED_CLASS_FLAG).map(ImmutableList::copyOf).orElse(ImmutableList.of()); - } - - private static ImmutableList> getSupportedClasses( - ImmutableList inclusions) { - return inclusions.stream() - .map(String::trim) - .filter(inclusion -> !inclusion.isEmpty()) - .map(inclusion -> isSubtypeOf(createClass(inclusion))) - .collect(toImmutableList()); - } - - private static Class createClass(String className) { - try { - return Class.forName(className); - } catch (ClassNotFoundException e) { - throw new IllegalArgumentException( - String.format("Invalid class name '%s' in `%s`", className, INCLUDED_CLASS_FLAG), e); - } + private static Matcher isSubtypeOfAny(ImmutableList inclusions) { + return anyOf( + inclusions.stream() + .map(inclusion -> isSubtypeOf(Suppliers.typeFromString(inclusion))) + .collect(toImmutableList())); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/Flags.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/Flags.java new file mode 100644 index 0000000000..ec5f4e92b3 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/Flags.java @@ -0,0 +1,25 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.ErrorProneFlags; + +/** Helper methods for working with {@link ErrorProneFlags}. */ +public final class Flags { + private Flags() {} + + /** + * Returns the list of (comma-separated) arguments passed using the given Error Prone flag. + * + * @param errorProneFlags The full set of flags provided. + * @param name The name of the flag of interest. + * @return A non-{@code null} list of provided arguments; this list is empty if the flag was not + * provided, or if the flag's value is the empty string. + */ + public static ImmutableList getList(ErrorProneFlags errorProneFlags, String name) { + return errorProneFlags + .getList(name) + .map(ImmutableList::copyOf) + .filter(flags -> !flags.equals(ImmutableList.of(""))) + .orElseGet(ImmutableList::of); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java index e024be4dc0..97b4c3cc4b 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java @@ -1,6 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import com.google.common.collect.ImmutableList; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; @@ -10,16 +9,7 @@ final class RequestParamTypeTest { private final CompilationTestHelper restrictedCompilationTestHelper = CompilationTestHelper.newInstance(RequestParamType.class, getClass()) .setArgs( - ImmutableList.of( - "-XepOpt:RequestParamType:Includes=com.google.common.collect.ImmutableCollection")); - private final CompilationTestHelper restrictedWithSubTypeCompilationTestHelper = - CompilationTestHelper.newInstance(RequestParamType.class, getClass()) - .setArgs( - ImmutableList.of( - "-XepOpt:RequestParamType:Includes=com.google.common.collect.ImmutableSet")); - private final CompilationTestHelper restrictedWithInvalidFlagCompilationTestHelper = - CompilationTestHelper.newInstance(RequestParamType.class, getClass()) - .setArgs(ImmutableList.of("-XepOpt:RequestParamType:Includes=")); + "-XepOpt:RequestParamType:SupportedCustomTypes=com.google.common.collect.ImmutableSet,com.google.common.collect.ImmutableSortedMultiset"); @Test void identification() { @@ -79,151 +69,50 @@ void identification() { } @Test - void identificationOfIncludedClass() { + void identificationRestricted() { restrictedCompilationTestHelper .addSourceLines( "A.java", "import com.google.common.collect.ImmutableBiMap;", + "import com.google.common.collect.ImmutableCollection;", "import com.google.common.collect.ImmutableList;", "import com.google.common.collect.ImmutableMap;", + "import com.google.common.collect.ImmutableMultiset;", "import com.google.common.collect.ImmutableSet;", - "import java.util.List;", - "import java.util.Map;", - "import java.util.Set;", - "import org.jspecify.annotations.Nullable;", - "import org.springframework.web.bind.annotation.DeleteMapping;", + "import com.google.common.collect.ImmutableSortedMultiset;", + "import com.google.common.collect.ImmutableSortedSet;", "import org.springframework.web.bind.annotation.GetMapping;", - "import org.springframework.web.bind.annotation.PostMapping;", - "import org.springframework.web.bind.annotation.PutMapping;", - "import org.springframework.web.bind.annotation.RequestBody;", "import org.springframework.web.bind.annotation.RequestParam;", "", "interface A {", - " @PostMapping", - " A properRequestParam(@RequestBody String body);", - "", - " @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);", - "", " @GetMapping", " // BUG: Diagnostic contains:", - " A get(@RequestParam ImmutableBiMap param);", - "", - " @PostMapping", - " A post(@Nullable @RequestParam ImmutableList param);", + " A immutableCollection(@RequestParam ImmutableCollection param);", "", - " @PutMapping", - " A put(@RequestBody String body, @RequestParam ImmutableSet param);", - "", - " @DeleteMapping", - " // BUG: Diagnostic contains:", - " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", - "", - " void negative(ImmutableSet set, ImmutableMap map);", - "}") - .doTest(); - } - - @Test - void identificationOfIncludedSubClass() { - restrictedWithSubTypeCompilationTestHelper - .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 org.jspecify.annotations.Nullable;", - "import org.springframework.web.bind.annotation.DeleteMapping;", - "import org.springframework.web.bind.annotation.GetMapping;", - "import org.springframework.web.bind.annotation.PostMapping;", - "import org.springframework.web.bind.annotation.PutMapping;", - "import org.springframework.web.bind.annotation.RequestBody;", - "import org.springframework.web.bind.annotation.RequestParam;", - "", - "interface A {", " @GetMapping", " // BUG: Diagnostic contains:", - " A get(@RequestParam ImmutableBiMap param);", - "", - " @PostMapping", - " // BUG: Diagnostic contains:", - " A post(@Nullable @RequestParam ImmutableList param);", - "", - " @PutMapping", - " A put(@RequestBody String body, @RequestParam ImmutableSet param);", - "", - " @DeleteMapping", - " // BUG: Diagnostic contains:", - " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", - "", - " void negative(ImmutableSet set, ImmutableMap map);", - "}") - .doTest(); - } - - @Test - void identificationOfInvalidFlag() { - restrictedWithInvalidFlagCompilationTestHelper - .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 org.jspecify.annotations.Nullable;", - "import org.springframework.web.bind.annotation.DeleteMapping;", - "import org.springframework.web.bind.annotation.GetMapping;", - "import org.springframework.web.bind.annotation.PostMapping;", - "import org.springframework.web.bind.annotation.PutMapping;", - "import org.springframework.web.bind.annotation.RequestBody;", - "import org.springframework.web.bind.annotation.RequestParam;", - "", - "interface A {", - " @PostMapping", - " A properRequestParam(@RequestBody String body);", + " A immutableList(@RequestParam ImmutableList param);", "", " @GetMapping", - " A properRequestParam(@RequestParam int param);", + " A immutableSet(@RequestParam ImmutableSet 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);", + " A immutableSortedSet(@RequestParam ImmutableSortedSet param);", "", " @GetMapping", " // BUG: Diagnostic contains:", - " A get(@RequestParam ImmutableBiMap param);", + " A immutableMultiset(@RequestParam ImmutableMultiset param);", "", - " @PostMapping", - " // BUG: Diagnostic contains:", - " A post(@Nullable @RequestParam ImmutableList param);", + " @GetMapping", + " A immutableSortedMultiset(@RequestParam ImmutableSortedMultiset param);", "", - " @PutMapping", + " @GetMapping", " // BUG: Diagnostic contains:", - " A put(@RequestBody String body, @RequestParam ImmutableSet param);", + " A immutableMap(@RequestParam ImmutableMap param);", "", - " @DeleteMapping", + " @GetMapping", " // BUG: Diagnostic contains:", - " A delete(@RequestBody String body, @RequestParam ImmutableMap param);", - "", - " void negative(ImmutableSet set, ImmutableMap map);", + " A immutableBiMap(@RequestParam ImmutableBiMap param);", "}") .doTest(); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/FlagsTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/FlagsTest.java new file mode 100644 index 0000000000..3ba51f0511 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/FlagsTest.java @@ -0,0 +1,31 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.ErrorProneOptions; +import java.util.stream.Stream; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +final class FlagsTest { + private static Stream getListTestCases() { + /* { args, flag, expected } */ + return Stream.of( + arguments(ImmutableList.of(), "Foo", ImmutableList.of()), + arguments(ImmutableList.of("-XepOpt:Foo=bar,baz"), "Qux", ImmutableList.of()), + arguments(ImmutableList.of("-XepOpt:Foo="), "Foo", ImmutableList.of()), + arguments(ImmutableList.of("-XepOpt:Foo=bar"), "Foo", ImmutableList.of("bar")), + arguments(ImmutableList.of("-XepOpt:Foo=bar,baz"), "Foo", ImmutableList.of("bar", "baz")), + arguments(ImmutableList.of("-XepOpt:Foo=,"), "Foo", ImmutableList.of("", ""))); + } + + @MethodSource("getListTestCases") + @ParameterizedTest + void getList(ImmutableList args, String flag, ImmutableList expected) { + assertThat(Flags.getList(ErrorProneOptions.processArgs(args).getFlags(), flag)) + .containsExactlyElementsOf(expected); + } +}