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

Warn when the HTTP method is not supposed to have a body #37219

Merged
merged 1 commit into from Dec 4, 2023
Merged

Warn when the HTTP method is not supposed to have a body #37219

merged 1 commit into from Dec 4, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 20, 2023

Closes: #36943

@geoand geoand requested a review from gastaldi November 20, 2023 13:30
@geoand geoand mentioned this pull request Nov 20, 2023
Closed
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 20, 2023 •
edited by github-actions bot

Failing Jobs - Building 136460c

Status Name Step Failures Logs Raw logs Build scan
✖ Native Tests - Data5 Update Docker Client User Agent ⚠️ Check → Logs Raw logs 🚧
✖ Native Tests - Spring Update Docker Client User Agent ⚠️ Check → Logs Raw logs 🚧

You can also consult the Develocity build scans.

fredericBregier
fredericBregier reviewed Nov 24, 2023
View reviewed changes
...on/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java
@@ -626,6 +629,10 @@ private ResourceMethod createResourceMethod(ClassInfo currentClassInfo, ClassInf
"Resource method " + currentMethodInfo + " can only have a single body parameter: "
+ currentMethodInfo.parameterName(i));
bodyParamType = paramType;
if (GET.equals(httpMethod) || HEAD.equals(httpMethod) || OPTIONS.equals(httpMethod)) {
log.warn("Using a body parameter with " + httpMethod + " is strongly discouraged. Offending method is '"
Copy link

Choose a reason for hiding this comment

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

"Strongly" might be too much: I agree that Head and Options should really not have a request body, while for Get, even if unusual, some services use it to pass arguments to filter response that would not fit as headers or path parameters.
Just my 2 cents

gsmet
gsmet approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

This is the correct thing to do. Let's merge it and see how it goes in the field and if we need to be less strict for GET.

@gsmet gsmet merged commit 19ef467 into quarkusio:main Dec 4, 2023
42 of 44 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 4, 2023
@gastaldi gastaldi deleted the #36943 branch December 4, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredericBregier fredericBregier fredericBregier left review comments

@gsmet gsmet gsmet approved these changes

@gastaldi gastaldi Awaiting requested review from gastaldi

Assignees
No one assigned
Labels
kind/bugfix
Projects
None yet
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.

Erroneous resteasy "Attempting a blocking read on io thread"
3 participants
@geoand @fredericBregier @gsmet