Skip to content

Commit

Permalink
Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
Stephan202 committed Jan 1, 2023
1 parent 16553b1 commit fb04db6
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 164 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -220,7 +221,7 @@ private static AnnotationAttributeMatcher createAnnotationAttributeMatcher(

private static ImmutableList<String> excludedAnnotations(ErrorProneFlags flags) {
Set<String> 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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<ExpressionTree> ANY_EXPR = (t, s) -> true;
Expand Down Expand Up @@ -374,10 +374,9 @@ private static Matcher<MethodInvocationTree> 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)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<VariableTree> hasUnsupportedRequestParams;
private final Matcher<VariableTree> hasUnsupportedRequestParamType;

/** Instantiates a default {@link RequestParamType} instance. */
public RequestParamType() {
Expand All @@ -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<VariableTree> createVariableTreeMatcher(ErrorProneFlags flags) {
private static Matcher<VariableTree> 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<String> includedClassNames(ErrorProneFlags flags) {
return flags.getList(INCLUDED_CLASS_FLAG).map(ImmutableList::copyOf).orElse(ImmutableList.of());
}

private static ImmutableList<Matcher<Tree>> getSupportedClasses(
ImmutableList<String> 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<Tree> isSubtypeOfAny(ImmutableList<String> inclusions) {
return anyOf(
inclusions.stream()
.map(inclusion -> isSubtypeOf(Suppliers.typeFromString(inclusion)))
.collect(toImmutableList()));
}
}
Original file line number Diff line number Diff line change
@@ -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<String> getList(ErrorProneFlags errorProneFlags, String name) {
return errorProneFlags
.getList(name)
.map(ImmutableList::copyOf)
.filter(flags -> !flags.equals(ImmutableList.of("")))
.orElseGet(ImmutableList::of);
}
}
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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() {
Expand Down Expand Up @@ -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<String> param);",
"",
" @PostMapping",
" A properRequestParam(@RequestBody String body, @RequestParam Set<String> param);",
"",
" @PutMapping",
" A properRequestParam(@RequestBody String body, @RequestParam Map<String, String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A get(@RequestParam ImmutableBiMap<String, String> param);",
"",
" @PostMapping",
" A post(@Nullable @RequestParam ImmutableList<String> param);",
" A immutableCollection(@RequestParam ImmutableCollection<String> param);",
"",
" @PutMapping",
" A put(@RequestBody String body, @RequestParam ImmutableSet<String> param);",
"",
" @DeleteMapping",
" // BUG: Diagnostic contains:",
" A delete(@RequestBody String body, @RequestParam ImmutableMap<String, String> param);",
"",
" void negative(ImmutableSet<Integer> set, ImmutableMap<String, String> 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<String, String> param);",
"",
" @PostMapping",
" // BUG: Diagnostic contains:",
" A post(@Nullable @RequestParam ImmutableList<String> param);",
"",
" @PutMapping",
" A put(@RequestBody String body, @RequestParam ImmutableSet<String> param);",
"",
" @DeleteMapping",
" // BUG: Diagnostic contains:",
" A delete(@RequestBody String body, @RequestParam ImmutableMap<String, String> param);",
"",
" void negative(ImmutableSet<Integer> set, ImmutableMap<String, String> 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<String> param);",
"",
" @GetMapping",
" A properRequestParam(@RequestParam int param);",
" A immutableSet(@RequestParam ImmutableSet<String> 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);",
" A immutableSortedSet(@RequestParam ImmutableSortedSet<String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A get(@RequestParam ImmutableBiMap<String, String> param);",
" A immutableMultiset(@RequestParam ImmutableMultiset<String> param);",
"",
" @PostMapping",
" // BUG: Diagnostic contains:",
" A post(@Nullable @RequestParam ImmutableList<String> param);",
" @GetMapping",
" A immutableSortedMultiset(@RequestParam ImmutableSortedMultiset<String> param);",
"",
" @PutMapping",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A put(@RequestBody String body, @RequestParam ImmutableSet<String> param);",
" A immutableMap(@RequestParam ImmutableMap<String, String> param);",
"",
" @DeleteMapping",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A delete(@RequestBody String body, @RequestParam ImmutableMap<String, String> param);",
"",
" void negative(ImmutableSet<Integer> set, ImmutableMap<String, String> map);",
" A immutableBiMap(@RequestParam ImmutableBiMap<String, String> param);",
"}")
.doTest();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Arguments> 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<String> args, String flag, ImmutableList<String> expected) {
assertThat(Flags.getList(ErrorProneOptions.processArgs(args).getFlags(), flag))
.containsExactlyElementsOf(expected);
}
}

0 comments on commit fb04db6

Please sign in to comment.