Introduce `RequestParamAnnotationCheck` for invalid types #33

hpnog · 2022-01-19T14:17:31Z

[ANS-219]

As a follow-up to PicnicSupermarket/picnic-platform#8503:

Introduce a new check: RequestParamAnnotationCheck;
- This new check should block any instance of ImmutableMap/subtypes of ImmutableCollection from being the type of ane @RequestParam.

rickie

Looks nice! 🚀 .

Pushed some suggestions, let me know what you think 😄

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

rickie · 2022-01-24T15:32:49Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+              AT_LEAST_ONE,
+              annotations(AT_LEAST_ONE, anyOf(isType(ANN_PACKAGE_PREFIX + "RequestParam")))));
+
+  private static final Matcher<MethodTree> HAS_INVALID_REQUEST_PARAM_TYPE =


Based on RequestMappingAnnotationCheck it is understandable that these Matchers are separate. However, in this case we can also combine them, as they are almost identical. By adding the second condition of the second matcher to the first matcher, we only have to use one Matcher without loosing clarity (IMO). Pushed a suggestion, WDYT 😄 ?

I was wrong here, leaving this as is. Still think we can combine it into one Matcher 🤔 .

Added an extra test case.

Ah, I was trying to merge the Matchers but I think I just found why you added that testcase 😅

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

rickie · 2022-01-24T16:07:20Z

(There is a bug in my suggestion, so not pushing yet)

rickie

Thanks for the check @hpnog 🚀 !

werli

Conceptionally it makes sense to split these cases into two checks.

Nice one @hpnog, thanks!

rickie · 2022-03-03T14:41:31Z

Suggested commit message:

Introduce `RequestParamAnnotationCheck`

hpnog · 2022-03-23T10:05:47Z

Rebase ☝️

Stephan202

Rebased and added two commits. Suggested commit message:

Introduce `RequestParamTypeCheck` (#33)

Please carefully re-review, as I've made quite some changes.

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

Stephan202 · 2022-04-11T07:11:55Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+      methodHasParameters(
+          AT_LEAST_ONE,
+          allOf(
+              annotations(ALL, isType(ANN_PACKAGE_PREFIX + "RequestParam")),


Suggested change

annotations(ALL, isType(ANN_PACKAGE_PREFIX + "RequestParam")),

annotations(AT_LEAST_ONE, isType(ANN_PACKAGE_PREFIX + "RequestParam")),

Otherwise the check ignores e.g. @Nullable @RequestParam parameters.

Stephan202 · 2022-04-11T07:32:35Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+              annotations(ALL, isType(ANN_PACKAGE_PREFIX + "RequestParam")),
+              anyOf(
+                  isSubtypeOf("com.google.common.collect.ImmutableCollection"),
+                  isSameType("com.google.common.collect.ImmutableMap"))));


Suggested change

isSameType("com.google.common.collect.ImmutableMap"))));

isSubtypeOf("com.google.common.collect.ImmutableMap"))));

This way e.g. ImmutableBiMap is also covered.

Stephan202 · 2022-04-11T07:34:33Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+  private static final Matcher<MethodTree> HAS_REQUEST_MAPPING_ANNOTATION =
+      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"))));


The @RequestParam check is done twice (here and below). IMHO we can drop this whole HAS_REQUEST_MAPPING_ANNOTATION constant; the other Matcher should suffice.

Stephan202 · 2022-04-11T07:48:14Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+            .setMessage(
+                "At least one defined Request Parameter has an invalid type. "
+                    + "`ImmutableMap`, `ImmutableCollection` and subtypes are not allowed.")


A shorter text could be

@RequestParam does not support ImmutableCollection and ImmutableMap subtypes

We can define this in the @BugPattern summary and then simply use describeMatch(tree) here.

Stephan202 · 2022-04-11T07:53:12Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+      methodHasParameters(
+          AT_LEAST_ONE,


Since in the end we're interested in method parameters and since @RequestParam cannot be applied to anything other than a parameter, we can simplify the code by using a Matcher<VariableTree>. This will also make the Error Prone error message point to the exact parameter impacted.

Stephan202 · 2022-04-11T07:56:38Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+                  isSubtypeOf("com.google.common.collect.ImmutableCollection"),
+                  isSameType("com.google.common.collect.ImmutableMap"))));


We should use string references for types that may not be on the annotation processor classpath; this holds for e.g. org.springframework.web.bind.annotation.RequestParam.

On the other hand, Error Prone itself depends on Guava, so for ImmutableCollection and ImmutableMap we can use .class references.

Stephan202 · 2022-04-11T08:05:05Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamAnnotationCheck.java

+/**
+ * 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}.
+ */


Suggested change

/**

* 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. */

(Over time we may add additional unsupported types.)

rickie · 2022-04-12T07:45:33Z

WDYT of the changes @hpnog ? :)

hpnog · 2022-04-12T08:13:51Z

WDYT of the changes @hpnog ? :)

Took some time to circle back to this one. Looks good 🚀
And cleaner 🧹

If I could, I'd approve ✔️

hpnog requested review from rickie and Stephan202 January 19, 2022 14:17

rickie reviewed Jan 24, 2022

View reviewed changes

rickie changed the title ~~ANS-219 Introduce RequestParamAnnotationCheck for invalid types~~ ANS-219 Introduce RequestParamAnnotationCheck for invalid types Jan 26, 2022

hpnog requested a review from rickie January 26, 2022 17:11

rickie approved these changes Jan 27, 2022

View reviewed changes

werli approved these changes Jan 27, 2022

View reviewed changes

rickie changed the title ~~ANS-219 Introduce RequestParamAnnotationCheck for invalid types~~ Introduce RequestParamAnnotationCheck for invalid types Feb 9, 2022

hpnog force-pushed the hpnog/ANS-219 branch from 70e0930 to bdf7e4d Compare March 23, 2022 10:05

hpnog and others added 4 commits April 11, 2022 10:07

ANS-219 Introduce RequestParamAnnotationCheck for invalid types

4d71b01

Suggestions

5d00e71

Suggestions

f405976

Rename

6f529ea

Stephan202 force-pushed the hpnog/ANS-219 branch from bdf7e4d to 6f529ea Compare April 11, 2022 08:07

Stephan202 approved these changes Apr 11, 2022

View reviewed changes

Stephan202 merged commit cc7074a into master Apr 12, 2022

Stephan202 deleted the hpnog/ANS-219 branch April 12, 2022 08:45

Stephan202 added this to the 0.1.0 milestone Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `RequestParamAnnotationCheck` for invalid types #33

Introduce `RequestParamAnnotationCheck` for invalid types #33

hpnog commented Jan 19, 2022 •

edited

Loading

rickie left a comment

rickie Jan 24, 2022

rickie Jan 24, 2022

rickie Jan 24, 2022

hpnog Jan 26, 2022

rickie commented Jan 24, 2022

rickie left a comment

werli left a comment

rickie commented Mar 3, 2022

hpnog commented Mar 23, 2022

Stephan202 left a comment

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

Stephan202 Apr 11, 2022

rickie commented Apr 12, 2022

hpnog commented Apr 12, 2022

	annotations(ALL, isType(ANN_PACKAGE_PREFIX + "RequestParam")),
	annotations(AT_LEAST_ONE, isType(ANN_PACKAGE_PREFIX + "RequestParam")),

	isSameType("com.google.common.collect.ImmutableMap"))));
	isSubtypeOf("com.google.common.collect.ImmutableMap"))));

Introduce RequestParamAnnotationCheck for invalid types #33

Introduce RequestParamAnnotationCheck for invalid types #33

Conversation

hpnog commented Jan 19, 2022 • edited Loading

rickie 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

rickie commented Jan 24, 2022

rickie left a comment

Choose a reason for hiding this comment

werli left a comment

Choose a reason for hiding this comment

rickie commented Mar 3, 2022

hpnog commented Mar 23, 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

rickie commented Apr 12, 2022

hpnog commented Apr 12, 2022

Introduce `RequestParamAnnotationCheck` for invalid types #33

Introduce `RequestParamAnnotationCheck` for invalid types #33

hpnog commented Jan 19, 2022 •

edited

Loading