-
Notifications
You must be signed in to change notification settings - Fork 915
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.
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
Introduce rawPath in RequestTarget #5932
base: main
Are you sure you want to change the base?
Conversation
@@ -645,7 +654,9 @@ private static RequestTarget slowForClient(String reqTarget, | |||
null, | |||
-1, | |||
encodedPath, | |||
encodedPath, encodedQuery, | |||
encodedPath, | |||
encodedPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I am not sure what's the best behavior for client, I just reuse the encodedPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer using reqTarget
, the original value we get from :path
header.
@@ -169,6 +169,7 @@ default RoutingContext withPath(String path) { | |||
oldReqTarget.port(), | |||
pathWithoutMatrixVariables, | |||
path, | |||
path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, it seems like it's only going to modify the path but not query or fragment.
@@ -443,6 +451,7 @@ private static RequestTarget slowForServer(String reqTarget, boolean allowSemico | |||
-1, | |||
matrixVariablesRemovedPath, | |||
encodedPath, | |||
reqTarget, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my read of the code is that the reqTarget here is the original one in Http2RequestDecoder but it actually may be modified at Http1RequestDecoder.
hoping to get some feedbacks! |
🔍 Build Scan® (commit: 6bd1e20) |
@ikhoon I hope to get some feedbacks thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks nice.
assertThat(res).isNotNull(); | ||
assertThat(res.query()).isEqualTo("a=..=" + qs + "b=..="); | ||
assertThat(QueryParams.fromQueryString(res.query(), true)).containsExactly( | ||
assertThat(res.requestTarget).isNotNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also write e2e tests to verify that rawPath
is correctly preserved?
Please address the line failure.
|
d463761
to
6bd1e20
Compare
Motivation:
It may be useful if we have access to the original request path unmodified. For example we have use cases where the decoding rules are different from the ones at
decodedPath
.Modifications:
Result: