Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exempt @RequestAttribute from RequestMappingAnnotation check #189

Merged
merged 3 commits into from
Aug 12, 2022
Merged

Exempt @RequestAttribute from RequestMappingAnnotation check #189

merged 3 commits into from
Aug 12, 2022

Conversation

ddeya
Copy link
Contributor

@ddeya ddeya commented Aug 11, 2022 •
edited by rickie
Loading

Since it is a valid use case, we should allow this case and exempt @RequestAttribute from the RequestMappingAnnotation check.

@rickie rickie changed the title PSM-1555 Exempt @RequestAttribute from RequestMappingAnnotation check Exempt @RequestAttribute from RequestMappingAnnotation check Aug 11, 2022
Stephan202
Stephan202 reviewed Aug 11, 2022
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tnx @ddeya! And congrats on your first Error Prone Support PR 😄

I don't have the code checked out right now, but don't mind applying these changes myself later :)

...prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java Outdated
Comment on lines 68 to 69
isType(ANN_PACKAGE_PREFIX + "RequestParam"),
isType(ANN_PACKAGE_PREFIX + "RequestAttribute"))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 Let's keep the list sorted :)

Suggested change
isType(ANN_PACKAGE_PREFIX + "RequestParam"),
isType(ANN_PACKAGE_PREFIX + "RequestAttribute"))),
isType(ANN_PACKAGE_PREFIX + "RequestAttribute"),
isType(ANN_PACKAGE_PREFIX + "RequestParam"))),

...e-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotationTest.java Outdated
Comment on lines 57 to 60
" A properRequestParam(@RequestParam String param);",
"",
" @RequestMapping",
" A properRequestAttribute(@RequestAttribute String param);",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then similarly here (also tweaked the parameter name):

Suggested change
" A properRequestParam(@RequestParam String param);",
"",
" @RequestMapping",
" A properRequestAttribute(@RequestAttribute String param);",
" A properRequestAttribute(@RequestAttribute String attribute);",
"",
" @RequestMapping",
" A properRequestParam(@RequestParam String param);",

@Stephan202 Stephan202 added this to the 0.1.1 milestone Aug 11, 2022
rickie
rickie approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@rickie rickie left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the suggestions.

Aha now I see @Stephan202 reviewed it as well 😅.

Anyway, thanks for opening a PR in error-prone-support 🚀🚀🚀 !

...prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotation.java Outdated
@@ -65,7 +65,8 @@ public final class RequestMappingAnnotation extends BugChecker implements Method
isType(ANN_PACKAGE_PREFIX + "PathVariable"),
isType(ANN_PACKAGE_PREFIX + "RequestBody"),
isType(ANN_PACKAGE_PREFIX + "RequestHeader"),
isType(ANN_PACKAGE_PREFIX + "RequestParam"))),
isType(ANN_PACKAGE_PREFIX + "RequestParam"),
isType(ANN_PACKAGE_PREFIX + "RequestAttribute"))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Moving this up to retain sorting :).

...e-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RequestMappingAnnotationTest.java Outdated
@@ -56,6 +57,9 @@ void identification() {
" A properRequestParam(@RequestParam String param);",
"",
" @RequestMapping",
" A properRequestAttribute(@RequestAttribute String param);",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
" A properRequestAttribute(@RequestAttribute String param);",
" A properRequestAttribute(@RequestAttribute String attribute);",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also moving up to retain sorting 😄)

@rickie
Copy link
Member

rickie commented Aug 11, 2022 •
edited by Stephan202
Loading

Suggested commit message:

Have `RequestMappingAnnotation` recognize `@RequestAttribute` parameters (#189)

Stephan202
Stephan202 approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see my sorting suggestions weren't correct. Tnx for doing the right thing @rickie 😄

@Stephan202
Copy link
Member

(Tweaked the suggested commit message. Avoided "exemption" because the connotation doesn't quite match, I think.

@ddeya
Copy link
Contributor Author

ddeya commented Aug 12, 2022

@Stephan202 @rickie Thanks both for the suggestions, have been a little busy since yesterday so I couldn't check earlier.

werli
werli approved these changes Aug 12, 2022
View reviewed changes
Copy link
Member

@werli werli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Congrats @ddeya 💪

nathankooij
nathankooij approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@nathankooij nathankooij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we didn't do it for the other annotations, but wouldn't it be more correct to verify that it's not triggered for any occurrence of @{Patch,Post, ...}Mapping? Either way, not a blocker.

@luis-santos-teampicnic
Copy link

🚀 Nice!

@Stephan202
Copy link
Member

I see we didn't do it for the other annotations, but wouldn't it be more correct to verify that it's not triggered for any occurrence of @{Patch,Post, ...}Mapping? Either way, not a blocker.

@nathankooij yeah, the test tries to assert a bit of everything, without enumerating the full Cartesian product 👍

@Stephan202 Stephan202 merged commit 9bc732b into master Aug 12, 2022
@Stephan202 Stephan202 deleted the ddeya/PSM-1555 branch August 12, 2022 18:56
@rickie rickie mentioned this pull request Aug 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathankooij nathankooij nathankooij approved these changes

@rickie rickie rickie approved these changes

@werli werli werli approved these changes

@Stephan202 Stephan202 Stephan202 approved these changes

Assignees
No one assigned
Labels
improvement
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ddeya @rickie @Stephan202 @luis-santos-teampicnic @werli @nathankooij