Have `RequestParamType` ignore parameter types with custom deserialization support #426

chamil-prabodha · 2022-12-28T13:56:45Z

One would want to use classes such as ImmutableList with @RequestParam given that the custom support for the underlying conversion is provided. For such occasions we can specify the classes we wish to be included on top of what is already provided (by Spring) by using the flag -XepOpt:RequestParamType:Includes=<class-name>

github-actions · 2022-12-28T13:59:26Z

Surviving mutants in this change: 1
Killed mutants in this change: 10

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.RequestParamType`	1	10

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-28T14:00:42Z

Surviving mutants in this change: 1
Killed mutants in this change: 10

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.RequestParamType`	1	10

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-28T15:26:31Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.RequestParamType`	0	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-29T09:12:53Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.RequestParamType`	0	11

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Rebased and added a commit. Thanks for this PR!

Suggested commit message:

Have `RequestParamType` ignore parameter types with custom deserialization support (#426)

While there, introduce and use a new `Flags` utility class; various checks'
list flags now better support empty lists.

Stephan202 · 2023-01-01T13:09:53Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

+        .map(String::trim)
+        .filter(inclusion -> !inclusion.isEmpty())


Other code where we call ErrorProneFlags#getList doesn't trim or filter. Perhaps we should introduce support for that, but for now let's keep the burden with the caller.

Edit: added a helper method :)

Stephan202 · 2023-01-01T13:11:13Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

        ? describeMatch(tree)
        : Description.NO_MATCH;
  }
+
+  private static Matcher<VariableTree> createVariableTreeMatcher(ErrorProneFlags flags) {


This method name doesn't explain what the returned matcher does.

Stephan202 · 2023-01-01T13:12:12Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java


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


Suggested change

private final Matcher<VariableTree> hasUnsupportedRequestParams;

private final Matcher<VariableTree> hasUnsupportedRequestParam;

Even clearer: hasUnsupportedRequestParamType.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

Stephan202 · 2023-01-01T13:13:40Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

-      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:";


This can be inlined.

Stephan202 · 2023-01-01T13:14:20Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

-          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";


This flag name should somehow convey that it enumerates types for which custom support in provided.

The constant should have s/CLASS/TYPES/.

Stephan202 · 2023-01-01T13:18:23Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

+  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);
+    }
+  }


We should not require the listed types to be on the classpath, as that prevents users from centrally configuring the types in a multi-module (Maven) project in which some modules don't have the listed type.

(Also, IIUC this would attempt to load the class from the annotation processor classpath rather than the compilation classpath, which is incorrect.)

Stephan202 · 2023-01-01T13:23:13Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java

+  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="));


Given the logic involved, IIUC one case would suffice.

Stephan202 · 2023-01-01T13:26:31Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java

+  private final CompilationTestHelper restrictedCompilationTestHelper =
+      CompilationTestHelper.newInstance(RequestParamType.class, getClass())
+          .setArgs(
+              ImmutableList.of(


We can omit ImmutableList.of here. (Yep, can also be cleaned up in other tests; I have a TODO list entry for a BugChecker that flags such cases.)

Stephan202 · 2023-01-01T13:39:47Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java

+    return flags.getList(INCLUDED_CLASS_FLAG).map(ImmutableList::copyOf).orElse(ImmutableList.of());
+  }
+
+  private static ImmutableList<Matcher<Tree>> getSupportedClasses(


This method returns matchers, not classes.

github-actions · 2023-01-01T14:43:33Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.RequestParamType`	0	7
🎉`tech.picnic.errorprone.bugpatterns.util.Flags`	0	3
🎉`tech.picnic.errorprone.bugpatterns.RedundantStringConversion`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-03T10:44:58Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.RequestParamType`	0	7
🎉`tech.picnic.errorprone.bugpatterns.util.Flags`	0	3
🎉`tech.picnic.errorprone.bugpatterns.RedundantStringConversion`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

One option we can discuss. Anyway approving the code as it looks good already!

Nice PR @chamil-prabodha 😄 !

@Stephan202 nice Flags improvements 🚀 !

rickie · 2023-01-03T11:00:11Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestParamTypeTest.java

@@ -6,6 +6,10 @@
 final class RequestParamTypeTest {
  private final CompilationTestHelper compilationTestHelper =
      CompilationTestHelper.newInstance(RequestParamType.class, getClass());
+  private final CompilationTestHelper restrictedCompilationTestHelper =


@nathankooij In Error Prone there are cases for both situations; but it can be nice to inline the restrictedCompilationTestHelper IMO. Usually there is only one test with the restricted variant and would make it extra clear in the test method itself that we are using extra flags. Besides that, the flag would be closer to the test, which makes the intend clearer as well.

WDYT?

Yes, I've pondered this a few times as well. I'm fine with that, though it sounds like we should do that in a separate PR.

I'll file a PR for that then!

Filed #442.

github-actions · 2023-01-04T09:58:33Z

Looks good. All 11 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.RequestParamType`	0	7
🎉`tech.picnic.errorprone.bugpatterns.util.Flags`	0	3
🎉`tech.picnic.errorprone.bugpatterns.RedundantStringConversion`	0	1

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

chamil-prabodha force-pushed the chamil-prabodha/PSM-1820 branch from 6e66597 to 5f70f62 Compare December 28, 2022 13:58

chamil-prabodha marked this pull request as ready for review December 28, 2022 15:33

chamil-prabodha requested review from rickie and Stephan202 December 28, 2022 15:33

chamil-prabodha changed the title ~~Introduce support to use additional classes with @RequestParam~~ Introduce support for using additional classes with @RequestParam Dec 28, 2022

chamil-prabodha force-pushed the chamil-prabodha/PSM-1820 branch from 2cf48d9 to 5a48292 Compare December 29, 2022 09:10

Stephan202 force-pushed the chamil-prabodha/PSM-1820 branch from 5a48292 to fb04db6 Compare January 1, 2023 14:41

Stephan202 approved these changes Jan 1, 2023

View reviewed changes

rickie force-pushed the chamil-prabodha/PSM-1820 branch from fb04db6 to eace6ce Compare January 3, 2023 10:42

rickie approved these changes Jan 3, 2023

View reviewed changes

chamil-prabodha and others added 3 commits January 4, 2023 10:55

Introduce additional arguments to RequestParamType

621b84d

Add a test for invalid flags

c2e4767

Suggestions

63a1386

rickie force-pushed the chamil-prabodha/PSM-1820 branch from eace6ce to 63a1386 Compare January 4, 2023 09:56

rickie added the improvement label Jan 4, 2023

rickie added this to the 0.7.0 milestone Jan 4, 2023

rickie changed the title ~~Introduce support for using additional classes with @RequestParam~~ Have RequestParamType ignore parameter types with custom deserialization support Jan 4, 2023

rickie merged commit 9d08e8f into master Jan 4, 2023

rickie deleted the chamil-prabodha/PSM-1820 branch January 4, 2023 10:13

rickie mentioned this pull request Jan 4, 2023

Inline most {BugCheckerRefactoring,Compilation}TestHelper fields #442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have `RequestParamType` ignore parameter types with custom deserialization support #426

Have `RequestParamType` ignore parameter types with custom deserialization support #426

chamil-prabodha commented Dec 28, 2022 •

edited

Loading

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 29, 2022

Stephan202 left a comment

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

Stephan202 Jan 1, 2023

github-actions bot commented Jan 1, 2023

github-actions bot commented Jan 3, 2023

rickie left a comment

rickie Jan 3, 2023 •

edited

Loading

Stephan202 Jan 3, 2023

rickie Jan 4, 2023

rickie Jan 4, 2023

github-actions bot commented Jan 4, 2023

		.map(String::trim)
		.filter(inclusion -> !inclusion.isEmpty())

	private final Matcher<VariableTree> hasUnsupportedRequestParams;
	private final Matcher<VariableTree> hasUnsupportedRequestParam;

Have RequestParamType ignore parameter types with custom deserialization support #426

Have RequestParamType ignore parameter types with custom deserialization support #426

Conversation

chamil-prabodha commented Dec 28, 2022 • edited Loading

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

github-actions bot commented Dec 29, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2023

github-actions bot commented Jan 3, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2023

Have `RequestParamType` ignore parameter types with custom deserialization support #426

Have `RequestParamType` ignore parameter types with custom deserialization support #426

chamil-prabodha commented Dec 28, 2022 •

edited

Loading

rickie Jan 3, 2023 •

edited

Loading