Introduce rawPath in RequestTarget #5932
Conversation
yzfeng2020
changed the title
| @@ -645,7 +654,9 @@ private static RequestTarget slowForClient(String reqTarget, | |||
| null, | |||
| -1, | |||
| encodedPath, | |||
| encodedPath, encodedQuery, | |||
| encodedPath, | |||
| encodedPath, | |||
There was a problem hiding this comment.
tbh I am not sure what's the best behavior for client, I just reuse the encodedPath
There was a problem hiding this comment.
I prefer using reqTarget, the original value we get from :path header.
core/src/main/java/com/linecorp/armeria/internal/common/DefaultRequestTarget.java
Show resolved
Hide resolved
|
hoping to get some feedbacks! |
yzfeng2020
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
@ikhoon I hope to get some feedbacks thanks! |
| 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.
Should we also write e2e tests to verify that rawPath is correctly preserved?
There was a problem hiding this comment.
sure, added a new e2e test!
|
Please address the line failure. |
yzfeng2020
force-pushed
the
add-raw-path
branch
3 times, most recently
from
144b37f
to
9d460f4
Compare
yzfeng2020
force-pushed
the
add-raw-path
branch
from
9d460f4
to
5a3bfcf
Compare
| /** | ||
| * Returns the server-side raw path of this {@link RequestTarget} from the ":path" header. | ||
| * Unlike {@link #path()}, the returned string is the original path without any normalization. | ||
| * For client-side target it always returns {@code null}. |
There was a problem hiding this comment.
Would returning a non-null make sense on the client-side? By doing so, this method could be always non-null, which removes the need for null check on the server side as well (i.e. no complaints from NullAway and other linters)
There was a problem hiding this comment.
I thought about that as well, it's not very clear to me what's the semantic, because we need to consider the case for absolute or not, and its transformation along the lines.
Another idea I have is to expose it only in ServiceRequestContext, and keep the one in RequestTarget internal.
There was a problem hiding this comment.
Exposing it only in ServiceRequestContext sounds good to me.
There was a problem hiding this comment.
took a closer look, b/c DefaultRequestTarget and DefaultServiceRequestContext don't live within the same package, it doesn't seems to be very easy, I might be missing something though
| TEST_URLS.add("/%3A%2F%3F%23%5B%5D%40%21%24%26%27%28%29*%2B%2C%3B%3D"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Shall we use @ParameterizedTest with @CsvSource?
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: