fix: preserve HTTP method on 307 and 308 redirects#6658
Conversation
Previously, 307 Temporary Redirect was only recognized as a redirect for GET and HEAD requests, and 308 Permanent Redirect converted all methods to GET. Both violate the method preservation requirement of RFC 9110 §15.4.8 (307) and RFC 7538 §3 (308). - HTTPSampleResult.isRedirect(): recognize 307 as redirect for all HTTP methods, removing the GET/HEAD restriction - HTTPSamplerBase.computeMethodForRedirect(): accept response code parameter; preserve original method for 307 and 308 - HTTPSamplerBase.followRedirects(): pass current hop's response code (lastRes, not res) to support mixed redirect chains (e.g. 307->301) This is a behavior change: POST/PUT requests receiving 307 or 308 will now be redirected with the original method preserved, whereas previously 307 POST was not followed and 308 POST was converted to GET. Fixes apache#6080
dlwldnjs1009
force-pushed
the
gh6080-fix-307-308-redirect-method
branch
from
dbe46fd to
a3ff630
Compare
dlwldnjs1009
changed the title
FSchumacher
left a comment
FSchumacher
left a comment
There was a problem hiding this comment.
Only looked at the changes, but they look good to me.
| for (int statusCode : Arrays.asList(301, 302, 303, 307, 308)) { | ||
| for (String method : httpMethods) { | ||
| boolean shouldRedirect = statusCode != 307 || ("GET".equals(method) || "HEAD".equals(method)); | ||
| boolean shouldRedirect = true; |
There was a problem hiding this comment.
If this is always true, why not inline it into the next line?
There was a problem hiding this comment.
Well. If the value is shouldRedirect=true, should we rather remove the parameter then?
There was a problem hiding this comment.
Thanks, good point. I inlined true in the redirect block.
I kept the parameter because the non-redirect cases in the second loop (300, 304, 305, 306) still use false.
| Arrays.stream(HTTPSamplerFactory.getImplementations()).forEach(httpImpl -> { | ||
| for (int statusCode : Arrays.asList(301, 302, 303, 307, 308)) { | ||
| for (String method : httpMethods) { | ||
| String expectedMethod; |
There was a problem hiding this comment.
As we now use Java 17, we could use a switch expression:
String expectedMethod = switch (statusCode) {
case 307, 308 -> method;
default -> switch (method) {
case "HEAD" -> "HEAD";
default -> "GET";
};
};The inner switch could even be replaced with "HEAD".equals(method) ? "HEAD" : "GET". But I think the switch would be nicer.
There was a problem hiding this comment.
Good suggestion, updated to use a switch expression here.
For the default branch I kept the HEAD check as a ternary since there are only two cases.
- Inline shouldRedirect variable directly into Arguments.of() - Use Java 17 switch expression for expectedMethod computation
| // Nested for depth is 2 (max allowed is 1). [NestedForDepth] | ||
| List<String> httpMethods = Arrays.asList("HEAD", "GET", "POST", "PUT", "DELETE"); | ||
| Arrays.stream(HTTPSamplerFactory.getImplementations()).forEach(httpImpl -> { |
There was a problem hiding this comment.
This is ugly, and I am gravitating towards lifting NestedForDepth requirements. It can be in a separate PR though.
There was a problem hiding this comment.
Agreed, the stream wrapper makes the setup less readable. I'll remove the NestedForDepth comments here. I'd prefer to keep this PR focused on the redirect fix, and leave any cleanup around that for a separate PR.
|
Thanks for the PR! |
3 participants
Summary
Fixes #6080
This change fixes redirect handling in JMeter's
Follow Redirectspath for HTTP 307 and 308 responses.At the moment, JMeter only treats 307 as a redirect for
GETandHEAD, so requests such asPOST 307are not followed. In addition, redirected requests for 307/308 can still lose the original HTTP method.This change updates
HTTPSampleResult.isRedirect()so 307 is recognized as a redirect for all HTTP methods, and updates redirect method selection so 307/308 preserve the original method while keeping the existing 301/302/303 behavior unchanged.It also uses
lastRes.getResponseCode()infollowRedirects(), so mixed redirect chains such as307 -> 301are handled correctly.Tests
testRedirectfor 307 redirect recognitionTestRedirects307 -> 301)./gradlew :src:protocol:http:check