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

Introduce rawPath in RequestTarget #5932

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Introduce rawPath in RequestTarget #5932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a3bfcf
feat: rawPath
yzfeng2020 Oct 22, 2024
3be7d04
fix lint and add comments
yzfeng2020 Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/main/java/com/linecorp/armeria/common/AbstractRequestContextBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ protected AbstractRequestContextBuilder(boolean server, RpcRequest rpcReq, URI u
} else {
reqTarget = DefaultRequestTarget.createWithoutValidation(
RequestTargetForm.ORIGIN, null, null, null, -1,
uri.getRawPath(), uri.getRawPath(), uri.getRawQuery(), uri.getRawFragment());
uri.getRawPath(), uri.getRawPath(), null,
uri.getRawQuery(), uri.getRawFragment());
}
}

Expand Down
8 changes: 8 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/RequestTarget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,14 @@ static RequestTarget forClient(String reqTarget, @Nullable String prefix) {
*/
String maybePathWithMatrixVariables();

/**
* 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}.
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Exposing it only in ServiceRequestContext sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
@Nullable
String rawPath();

/**
* Returns the query of this {@link RequestTarget}.
*/
Expand Down
52 changes: 36 additions & 16 deletions core/src/main/java/com/linecorp/armeria/internal/common/DefaultRequestTarget.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,16 +163,8 @@ boolean mustPreserveEncoding(int cp) {
private static final Bytes EMPTY_BYTES = new Bytes(0);
private static final Bytes SLASH_BYTES = new Bytes(new byte[] { '/' });

private static final RequestTarget INSTANCE_ASTERISK = createWithoutValidation(
RequestTargetForm.ASTERISK,
null,
null,
null,
-1,
"*",
"*",
null,
null);
private static final RequestTarget CLIENT_INSTANCE_ASTERISK = createAsterisk(false);
private static final RequestTarget SERVER_INSTANCE_ASTERISK = createAsterisk(true);

/**
* The main implementation of {@link RequestTarget#forServer(String)}.
Expand Down Expand Up @@ -233,9 +225,9 @@ public static RequestTarget forClient(String reqTarget, @Nullable String prefix)
public static RequestTarget createWithoutValidation(
RequestTargetForm form, @Nullable String scheme, @Nullable String authority,
@Nullable String host, int port, String path, String pathWithMatrixVariables,
@Nullable String query, @Nullable String fragment) {
@Nullable String rawPath, @Nullable String query, @Nullable String fragment) {
return new DefaultRequestTarget(
form, scheme, authority, host, port, path, pathWithMatrixVariables, query, fragment);
form, scheme, authority, host, port, path, pathWithMatrixVariables, rawPath, query, fragment);
}

private final RequestTargetForm form;
Expand All @@ -249,14 +241,16 @@ public static RequestTarget createWithoutValidation(
private final String path;
private final String maybePathWithMatrixVariables;
@Nullable
private final String rawPath;
@Nullable
private final String query;
@Nullable
private final String fragment;
private boolean cached;

private DefaultRequestTarget(RequestTargetForm form, @Nullable String scheme,
@Nullable String authority, @Nullable String host, int port,
String path, String maybePathWithMatrixVariables,
String path,String maybePathWithMatrixVariables, @Nullable String rawPath,
@Nullable String query, @Nullable String fragment) {

assert (scheme != null && authority != null && host != null) ||
Expand All @@ -270,6 +264,7 @@ private DefaultRequestTarget(RequestTargetForm form, @Nullable String scheme,
this.port = port;
this.path = path;
this.maybePathWithMatrixVariables = maybePathWithMatrixVariables;
this.rawPath = rawPath;
this.query = query;
this.fragment = fragment;
}
Expand Down Expand Up @@ -312,6 +307,12 @@ public String maybePathWithMatrixVariables() {
return maybePathWithMatrixVariables;
}

@Override
@Nullable
public String rawPath() {
return rawPath;
}

@Nullable
@Override
public String query() {
Expand Down Expand Up @@ -380,6 +381,21 @@ public String toString() {
}
}

private static RequestTarget createAsterisk(boolean server) {
final String rawPath = server ? "*" : null;
return createWithoutValidation(
RequestTargetForm.ASTERISK,
null,
null,
null,
-1,
"*",
"*",
rawPath,
null,
null);
}

@Nullable
private static RequestTarget slowForServer(String reqTarget, boolean allowSemicolonInPathComponent,
boolean allowDoubleDotsInQueryString) {
Expand Down Expand Up @@ -411,7 +427,7 @@ private static RequestTarget slowForServer(String reqTarget, boolean allowSemico
// Reject a relative path and accept an asterisk (e.g. OPTIONS * HTTP/1.1).
if (isRelativePath(path)) {
if (query == null && path.length == 1 && path.data[0] == '*') {
return INSTANCE_ASTERISK;
return SERVER_INSTANCE_ASTERISK;
} else {
// Do not accept a relative path.
return null;
Expand Down Expand Up @@ -443,6 +459,7 @@ private static RequestTarget slowForServer(String reqTarget, boolean allowSemico
-1,
matrixVariablesRemovedPath,
encodedPath,
reqTarget,
yzfeng2020 marked this conversation as resolved.
Show resolved Hide resolved
encodeQueryToPercents(query),
null);
}
Expand Down Expand Up @@ -622,7 +639,7 @@ private static RequestTarget slowForClient(String reqTarget,

// Accept an asterisk (e.g. OPTIONS * HTTP/1.1).
if (query == null && path.length == 1 && path.data[0] == '*') {
return INSTANCE_ASTERISK;
return CLIENT_INSTANCE_ASTERISK;
}

final String encodedPath;
Expand All @@ -645,7 +662,9 @@ private static RequestTarget slowForClient(String reqTarget,
null,
-1,
encodedPath,
encodedPath, encodedQuery,
encodedPath,
null,
encodedQuery,
encodedFragment);
}
}
Expand Down Expand Up @@ -692,6 +711,7 @@ private static DefaultRequestTarget newAbsoluteTarget(
port,
encodedPath,
encodedPath,
null,
encodedQuery,
encodedFragment);
}
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/server/RoutingContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ default RoutingContext withPath(String path) {
oldReqTarget.port(),
pathWithoutMatrixVariables,
path,
path,
yzfeng2020 marked this conversation as resolved.
Show resolved Hide resolved
oldReqTarget.query(),
oldReqTarget.fragment());

Expand Down
110 changes: 65 additions & 45 deletions core/src/test/java/com/linecorp/armeria/internal/common/DefaultRequestTargetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,27 +143,27 @@ void serverShouldAcceptGoodDoubleDotPatterns(String pattern) {
void dotsAndEqualsInNameValueQuery() {
QUERY_SEPARATORS.forEach(qs -> {
assertThat(forServer("/?a=..=" + qs + "b=..=")).satisfies(res -> {
assertThat(res).isNotNull();
assertThat(res.query()).isEqualTo("a=..=" + qs + "b=..=");
assertThat(QueryParams.fromQueryString(res.query(), true)).containsExactly(
assertThat(res.requestTarget).isNotNull();
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, added a new e2e test!

assertThat(res.requestTarget.query()).isEqualTo("a=..=" + qs + "b=..=");
assertThat(QueryParams.fromQueryString(res.requestTarget.query(), true)).containsExactly(
Maps.immutableEntry("a", "..="),
Maps.immutableEntry("b", "..=")
);
});

assertThat(forServer("/?a==.." + qs + "b==..")).satisfies(res -> {
assertThat(res).isNotNull();
assertThat(res.query()).isEqualTo("a==.." + qs + "b==..");
assertThat(QueryParams.fromQueryString(res.query(), true)).containsExactly(
assertThat(res.requestTarget.query()).isEqualTo("a==.." + qs + "b==..");
assertThat(QueryParams.fromQueryString(res.requestTarget.query(), true)).containsExactly(
Maps.immutableEntry("a", "=.."),
Maps.immutableEntry("b", "=..")
);
});

assertThat(forServer("/?a==..=" + qs + "b==..=")).satisfies(res -> {
assertThat(res).isNotNull();
assertThat(res.query()).isEqualTo("a==..=" + qs + "b==..=");
assertThat(QueryParams.fromQueryString(res.query(), true)).containsExactly(
assertThat(res.requestTarget.query()).isEqualTo("a==..=" + qs + "b==..=");
assertThat(QueryParams.fromQueryString(res.requestTarget.query(), true)).containsExactly(
Maps.immutableEntry("a", "=..="),
Maps.immutableEntry("b", "=..=")
);
Expand Down Expand Up @@ -500,9 +500,9 @@ void clientShouldAcceptAbsoluteUri(String uri,
String expectedScheme, String expectedAuthority, String expectedPath,
@Nullable String expectedQuery, @Nullable String expectedFragment) {

final RequestTarget res = forClient(uri);
assertThat(res.scheme()).isEqualTo(expectedScheme);
assertThat(res.authority()).isEqualTo(expectedAuthority);
final RequestTargetWithRawPath res = forClient(uri);
assertThat(res.requestTarget.scheme()).isEqualTo(expectedScheme);
assertThat(res.requestTarget.authority()).isEqualTo(expectedAuthority);
assertAccepted(res, expectedPath, emptyToNull(expectedQuery), emptyToNull(expectedFragment));
}

Expand Down Expand Up @@ -531,15 +531,15 @@ void shouldYieldEmptyStringForEmptyQueryAndFragment(Mode mode) {
@ParameterizedTest
@EnumSource(Mode.class)
void testToString(Mode mode) {
assertThat(parse(mode, "/")).asString().isEqualTo("/");
assertThat(parse(mode, "/?")).asString().isEqualTo("/?");
assertThat(parse(mode, "/?a=b")).asString().isEqualTo("/?a=b");
assertThat(parse(mode, "/").requestTarget).asString().isEqualTo("/");
assertThat(parse(mode, "/?").requestTarget).asString().isEqualTo("/?");
assertThat(parse(mode, "/?a=b").requestTarget).asString().isEqualTo("/?a=b");

if (mode == Mode.CLIENT) {
assertThat(forClient("/#")).asString().isEqualTo("/#");
assertThat(forClient("/?#")).asString().isEqualTo("/?#");
assertThat(forClient("/?a=b#c=d")).asString().isEqualTo("/?a=b#c=d");
assertThat(forClient("http://foo/bar?a=b#c=d")).asString().isEqualTo("http://foo/bar?a=b#c=d");
assertThat(forClient("/#").requestTarget).asString().isEqualTo("/#");
assertThat(forClient("/?#").requestTarget).asString().isEqualTo("/?#");
assertThat(forClient("/?a=b#c=d").requestTarget).asString().isEqualTo("/?a=b#c=d");
assertThat(forClient("http://foo/bar?a=b#c=d").requestTarget).asString().isEqualTo("http://foo/bar?a=b#c=d");
}
}

Expand Down Expand Up @@ -572,32 +572,32 @@ void testRemoveMatrixVariables() {
assertThat(removeMatrixVariables("/prefix/;a=b")).isNull();
}

private static void assertAccepted(@Nullable RequestTarget res, String expectedPath) {
private static void assertAccepted(RequestTargetWithRawPath res, String expectedPath) {
assertAccepted(res, expectedPath, null, null);
}

private static void assertAccepted(@Nullable RequestTarget res,
private static void assertAccepted(RequestTargetWithRawPath res,
String expectedPath,
@Nullable String expectedQuery) {
assertAccepted(res, expectedPath, expectedQuery, null);
}

private static void assertAccepted(@Nullable RequestTarget res,
private static void assertAccepted(RequestTargetWithRawPath res,
String expectedPath,
@Nullable String expectedQuery,
@Nullable String expectedFragment) {
assertThat(res).isNotNull();
assertThat(res.path()).isEqualTo(expectedPath);
assertThat(res.query()).isEqualTo(expectedQuery);
assertThat(res.fragment()).isEqualTo(expectedFragment);
assertThat(res.requestTarget).isNotNull();
assertThat(res.requestTarget.path()).isEqualTo(expectedPath);
assertThat(res.requestTarget.query()).isEqualTo(expectedQuery);
assertThat(res.requestTarget.fragment()).isEqualTo(expectedFragment);
assertThat(res.requestTarget.rawPath()).isEqualTo(res.rawPath);
}

private static void assertRejected(@Nullable RequestTarget res) {
assertThat(res).isNull();
private static void assertRejected(RequestTargetWithRawPath res) {
assertThat(res.requestTarget).isNull();
}

@Nullable
private static RequestTarget parse(Mode mode, String rawPath) {
private static RequestTargetWithRawPath parse(Mode mode, String rawPath) {
switch (mode) {
case SERVER:
return forServer(rawPath);
Expand All @@ -608,37 +608,57 @@ private static RequestTarget parse(Mode mode, String rawPath) {
}
}

@Nullable
private static RequestTarget forServer(String rawPath) {
private static class RequestTargetWithRawPath {
@Nullable
final String rawPath;
@Nullable
final RequestTarget requestTarget;

RequestTargetWithRawPath(@Nullable String rawPath, @Nullable RequestTarget requestTarget) {
this.rawPath = rawPath;
this.requestTarget = requestTarget;
}

@Override
public String toString() {
return "RequestTargetWithRawPath{" +
"rawPath='" + rawPath + '\'' +
", requestTarget=" + requestTarget +
'}';
}
}

private static RequestTargetWithRawPath forServer(String rawPath) {
return forServer(rawPath, false);
}

@Nullable
private static RequestTarget forServer(String rawPath, boolean allowSemicolonInPathComponent) {
final RequestTarget res = DefaultRequestTarget.forServer(rawPath, allowSemicolonInPathComponent, false);
if (res != null) {
logger.info("forServer({}) => path: {}, query: {}", rawPath, res.path(), res.query());
private static RequestTargetWithRawPath forServer(String rawPath, boolean allowSemicolonInPathComponent) {
final RequestTarget target = DefaultRequestTarget.forServer(
rawPath,
allowSemicolonInPathComponent,
false);
if (target != null) {
logger.info("forServer({}) => path: {}, query: {}", rawPath, target.path(), target.query());
} else {
logger.info("forServer({}) => null", rawPath);
}
return res;
return new RequestTargetWithRawPath(rawPath, target);
}

@Nullable
private static RequestTarget forClient(String rawPath) {
private static RequestTargetWithRawPath forClient(String rawPath) {
return forClient(rawPath, null);
}

@Nullable
private static RequestTarget forClient(String rawPath, @Nullable String prefix) {
final RequestTarget res = DefaultRequestTarget.forClient(rawPath, prefix);
if (res != null) {
logger.info("forClient({}, {}) => path: {}, query: {}, fragment: {}", rawPath, prefix, res.path(),
res.query(), res.fragment());
private static RequestTargetWithRawPath forClient(String rawPath, @Nullable String prefix) {
final RequestTarget target = DefaultRequestTarget.forClient(rawPath, prefix);
if (target != null) {
logger.info("forClient({}, {}) => path: {}, query: {}, fragment: {}",
rawPath, prefix, target.path(),
target.query(), target.fragment());
} else {
logger.info("forClient({}, {}) => null", rawPath, prefix);
}
return res;
return new RequestTargetWithRawPath(null, target);
}

private static String toAbsolutePath(String pattern) {
Expand Down
Loading
Loading