Skip to content

Commit

Permalink
Have RequestParamType ignore parameter types with custom deserializ…
Browse files Browse the repository at this point in the history
…ation support (#426)

While there, introduce and use a new `Flags` utility class; various checks'
list flags now better support empty lists.
  • Loading branch information
chamil-prabodha authored Jan 4, 2023
1 parent 9992ff4 commit 9d08e8f
Show file tree
Hide file tree
Showing 6 changed files with 157 additions and 16 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
@@ -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;
Expand All @@ -9,41 +10,72 @@
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.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 Matcher<VariableTree> 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 SUPPORTED_CUSTOM_TYPES_FLAG = "RequestParamType:SupportedCustomTypes";

/** Instantiates a new {@link RequestParamType} instance. */
public RequestParamType() {}
private final Matcher<VariableTree> hasUnsupportedRequestParamType;

/** 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) {
hasUnsupportedRequestParamType = hasUnsupportedRequestParamType(flags);
}

@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
return HAS_UNSUPPORTED_REQUEST_PARAM.matches(tree, state)
return hasUnsupportedRequestParamType.matches(tree, state)
? describeMatch(tree)
: Description.NO_MATCH;
}

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(isSubtypeOfAny(Flags.getList(flags, SUPPORTED_CUSTOM_TYPES_FLAG))));
}

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
Expand Up @@ -6,6 +6,10 @@
final class RequestParamTypeTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(RequestParamType.class, getClass());
private final CompilationTestHelper restrictedCompilationTestHelper =
CompilationTestHelper.newInstance(RequestParamType.class, getClass())
.setArgs(
"-XepOpt:RequestParamType:SupportedCustomTypes=com.google.common.collect.ImmutableSet,com.google.common.collect.ImmutableSortedMultiset");

@Test
void identification() {
Expand Down Expand Up @@ -63,4 +67,53 @@ void identification() {
"}")
.doTest();
}

@Test
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 com.google.common.collect.ImmutableSortedMultiset;",
"import com.google.common.collect.ImmutableSortedSet;",
"import org.springframework.web.bind.annotation.GetMapping;",
"import org.springframework.web.bind.annotation.RequestParam;",
"",
"interface A {",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A immutableCollection(@RequestParam ImmutableCollection<String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A immutableList(@RequestParam ImmutableList<String> param);",
"",
" @GetMapping",
" A immutableSet(@RequestParam ImmutableSet<String> param);",
"",
" @GetMapping",
" A immutableSortedSet(@RequestParam ImmutableSortedSet<String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A immutableMultiset(@RequestParam ImmutableMultiset<String> param);",
"",
" @GetMapping",
" A immutableSortedMultiset(@RequestParam ImmutableSortedMultiset<String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A immutableMap(@RequestParam ImmutableMap<String, String> param);",
"",
" @GetMapping",
" // BUG: Diagnostic contains:",
" A immutableBiMap(@RequestParam ImmutableBiMap<String, String> param);",
"}")
.doTest();
}
}
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 9d08e8f

Please sign in to comment.