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

Remove pseudo headers when converting to Spring headers #4293

Merged
merged 4 commits into from
Jul 1, 2022
Merged

Remove pseudo headers when converting to Spring headers #4293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8380608
Remove pseudo headers when converting to Spring headers
minwoox Jun 8, 2022
f1df421
Dedup converting header logic
minwoox Jun 9, 2022
0b2e5e2
Address the comment by @ikhoon
minwoox Jun 10, 2022
128b420
Rename to key
minwoox Jun 13, 2022
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
26 changes: 26 additions & 0 deletions core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,5 +1078,31 @@ public static boolean isRequestTimeoutResponse(HttpResponse httpResponse) {
"close".equalsIgnoreCase(httpResponse.headers().get(HttpHeaderNames.CONNECTION));
}

/**
* Copies header value pairs of the specified {@linkplain HttpHeaders Armeria headers} to the
* {@link TriConsumer} excluding HTTP/2 pseudo headers that starts with ':'. This also converts
* {@link HttpHeaderNames#AUTHORITY} header to {@link HttpHeaderNames#HOST} header if
* the {@linkplain HttpHeaders Armeria headers} does not have one.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant the opposite

Suggested change
* the {@linkplain HttpHeaders Armeria headers} does not have one.
* the {@linkplain HttpHeaders Armeria headers} has one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually meant it. 😉 Let me change the word one to {@link HttpHeaderNames#HOST} because the original sentence is unclear. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see what you meant 😅

*/
public static <T> void toHttp1Headers(HttpHeaders armeriaHeaders, T output,
TriConsumer<T, AsciiString, String> writer) {
for (Entry<AsciiString, String> e : armeriaHeaders) {
final AsciiString k = e.getKey();
final String v = e.getValue();
if (k.charAt(0) != ':') {
writer.accept(output, k, v);
} else if (HttpHeaderNames.AUTHORITY.equals(k) && !armeriaHeaders.contains(HttpHeaderNames.HOST)) {
// Convert `:authority` to `host`.
writer.accept(output, HttpHeaderNames.HOST, v);
}
}
}

// TODO(minwoox): Will provide this interface to public API
@FunctionalInterface
public interface TriConsumer<T, U, V> {
void accept(T t, U u, V v);
}

private ArmeriaHttpUtil() {}
}
16 changes: 2 additions & 14 deletions jetty9/src/main/java/com/linecorp/armeria/server/jetty/JettyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.linecorp.armeria.server.jetty;

import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.toHttp1Headers;
import static java.util.Objects.requireNonNull;

import java.lang.invoke.MethodHandle;
Expand Down Expand Up @@ -69,7 +70,6 @@
import com.linecorp.armeria.server.ServiceRequestContext;

import io.netty.buffer.ByteBuf;
import io.netty.util.AsciiString;

/**
* An {@link HttpService} that dispatches its requests to a web application running in an embedded
Expand Down Expand Up @@ -375,19 +375,7 @@ private static MetaData.Request toRequestMetadata(ServiceRequestContext ctx, Agg

// Convert HttpHeaders to HttpFields
final HttpFields jHeaders = new HttpFields(aHeaders.size());
aHeaders.forEach(e -> {
final AsciiString key = e.getKey();
if (key.isEmpty()) {
return;
}

if (key.byteAt(0) != ':') {
jHeaders.add(key.toString(), e.getValue());
} else if (HttpHeaderNames.AUTHORITY.equals(key) && !aHeaders.contains(HttpHeaderNames.HOST)) {
// Convert `:authority` to `host`.
jHeaders.add(HttpHeaderNames.HOST.toString(), e.getValue());
}
});
toHttp1Headers(aHeaders, jHeaders, (output, key, value) -> output.add(key.toString(), value));

return new MetaData.Request(aHeaders.get(HttpHeaderNames.METHOD), uri,
ctx.sessionProtocol().isMultiplex() ? HttpVersion.HTTP_2
Expand Down
11 changes: 4 additions & 7 deletions ...ure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaClientHttpResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.linecorp.armeria.spring.web.reactive;

import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.toHttp1Headers;
import static java.util.Objects.requireNonNull;

import org.springframework.core.io.buffer.DataBuffer;
Expand Down Expand Up @@ -92,7 +93,9 @@ public HttpHeaders getHeaders() {
if (httpHeaders != null) {
return httpHeaders;
}
this.httpHeaders = initHttpHeaders();
this.httpHeaders = new HttpHeaders();
toHttp1Headers(headers, this.httpHeaders,
(output, key, value) -> output.add(key.toString(), value));
return this.httpHeaders;
}

Expand All @@ -119,10 +122,4 @@ private MultiValueMap<String, ResponseCookie> initCookies() {
.build()));
return cookies;
}

private HttpHeaders initHttpHeaders() {
final HttpHeaders httpHeaders = new HttpHeaders();
headers.forEach(entry -> httpHeaders.add(entry.getKey().toString(), entry.getValue()));
return httpHeaders;
}
}
21 changes: 14 additions & 7 deletions ...gure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.linecorp.armeria.spring.web.reactive;

import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.toHttp1Headers;
import static java.util.Objects.requireNonNull;

import java.net.InetSocketAddress;
Expand All @@ -38,6 +39,7 @@
import com.linecorp.armeria.common.HttpData;
import com.linecorp.armeria.common.HttpHeaderNames;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.server.ServiceRequestContext;

Expand All @@ -57,7 +59,7 @@ final class ArmeriaServerHttpRequest extends AbstractServerHttpRequest {
ArmeriaServerHttpRequest(ServiceRequestContext ctx,
HttpRequest req,
DataBufferFactoryWrapper<?> factoryWrapper) {
super(uri(req), null, fromArmeriaHttpHeaders(req.headers()));
super(uri(req), null, springHeaders(req.headers()));
this.ctx = requireNonNull(ctx, "ctx");
this.req = req;

Expand All @@ -67,6 +69,12 @@ final class ArmeriaServerHttpRequest extends AbstractServerHttpRequest {
.publishOn(Schedulers.fromExecutor(ctx.eventLoop()));
}

private static HttpHeaders springHeaders(RequestHeaders headers) {
final HttpHeaders springHeaders = new HttpHeaders();
toHttp1Headers(headers, springHeaders, (output, key, value) -> output.add(key.toString(), value));
return springHeaders;
}

private static URI uri(HttpRequest req) {
final String scheme = req.scheme();
final String authority = req.authority();
Expand All @@ -76,12 +84,6 @@ private static URI uri(HttpRequest req) {
return URI.create(scheme + "://" + authority + req.path());
}

private static HttpHeaders fromArmeriaHttpHeaders(com.linecorp.armeria.common.HttpHeaders httpHeaders) {
final HttpHeaders newHttpHeaders = new HttpHeaders();
httpHeaders.forEach(entry -> newHttpHeaders.add(entry.getKey().toString(), entry.getValue()));
return newHttpHeaders;
}

@Override
protected MultiValueMap<String, HttpCookie> initCookies() {
final MultiValueMap<String, HttpCookie> cookies = new LinkedMultiValueMap<>();
Expand Down Expand Up @@ -122,6 +124,11 @@ public Flux<DataBuffer> getBody() {
return body;
}

@Override
public InetSocketAddress getLocalAddress() {
return ctx.localAddress();
}

@Override
public InetSocketAddress getRemoteAddress() {
return ctx.remoteAddress();
Expand Down
10 changes: 10 additions & 0 deletions .../src/test/java/com/linecorp/armeria/spring/web/reactive/ArmeriaClientHttpRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import static org.mockito.Mockito.when;

import java.net.URI;
import java.util.List;
import java.util.Map.Entry;

import org.junit.BeforeClass;
import org.junit.Test;
Expand Down Expand Up @@ -75,6 +77,14 @@ public void completeWithoutBody() {
StepVerifier.create(httpRequest).expectComplete().verify();

await().until(() -> httpRequest.whenComplete().isDone());

// Spring headers does not have pseudo headers.
for (Entry<String, List<String>> header : request.getHeaders().entrySet()) {
assertThat(header.getKey()).doesNotStartWith(":");
}
assertThat(httpRequest.headers().names())
.contains(HttpHeaderNames.METHOD, HttpHeaderNames.AUTHORITY,
HttpHeaderNames.SCHEME, HttpHeaderNames.PATH);
}

@Test
Expand Down
7 changes: 7 additions & 0 deletions ...src/test/java/com/linecorp/armeria/spring/web/reactive/ArmeriaClientHttpResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.awaitility.Awaitility.await;

import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -67,6 +69,11 @@ public void readBodyStream() {
.verify();

await().until(() -> httpResponse.whenComplete().isDone());

// Spring headers does not have pseudo headers.
for (Entry<String, List<String>> header : response.getHeaders().entrySet()) {
assertThat(header.getKey()).doesNotStartWith(":");
}
}

@Test
Expand Down
22 changes: 22 additions & 0 deletions .../src/test/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.awaitility.Awaitility.await;

import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;

Expand Down Expand Up @@ -85,6 +87,26 @@ void readBodyStream() {
await().until(() -> httpRequest.whenComplete().isDone());
}

@Test
void springHeadersDoesNotHavePseudoHeaders() {
final HttpRequest httpRequest = HttpRequest.of(RequestHeaders.builder(HttpMethod.POST, "/")
.scheme("http")
.authority("127.0.0.1")
.build());
final ServiceRequestContext ctx = newRequestContext(httpRequest);
final ArmeriaServerHttpRequest req = request(ctx);
for (Entry<String, List<String>> header : req.getHeaders().entrySet()) {
assertThat(header.getKey()).doesNotStartWith(":");
}
// :authority header is converted to the HOST header.
assertThat(req.getHeaders().getFirst(HttpHeaderNames.HOST.toString())).isEqualTo("127.0.0.1");
final Object nativeRequest = req.getNativeRequest();
assertThat(nativeRequest).isInstanceOf(HttpRequest.class).isEqualTo(httpRequest);
assertThat(((HttpRequest) nativeRequest).headers().names())
.contains(HttpHeaderNames.METHOD, HttpHeaderNames.AUTHORITY,
HttpHeaderNames.SCHEME, HttpHeaderNames.PATH);
}

@Test
void getCookies() {
final HttpRequest httpRequest = HttpRequest.of(RequestHeaders.builder(HttpMethod.POST, "/")
Expand Down
7 changes: 7 additions & 0 deletions ...src/test/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import static org.awaitility.Awaitility.await;

import java.time.Duration;
import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -109,6 +111,11 @@ void returnHeadersOnly() throws Exception {
.verify();

await().until(() -> httpResponse.whenComplete().isDone());

// Spring headers does not have pseudo headers.
for (Entry<String, List<String>> header : response.getHeaders().entrySet()) {
assertThat(header.getKey()).doesNotStartWith(":");
}
}

@Test
Expand Down
24 changes: 3 additions & 21 deletions tomcat9/src/main/java/com/linecorp/armeria/server/tomcat/TomcatService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.linecorp.armeria.server.tomcat;

import static com.google.common.base.Preconditions.checkArgument;
import static com.linecorp.armeria.internal.common.ArmeriaHttpUtil.toHttp1Headers;
import static java.util.Objects.requireNonNull;

import java.io.File;
Expand All @@ -28,7 +29,6 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayDeque;
import java.util.Map.Entry;
import java.util.Queue;

import org.apache.catalina.LifecycleState;
Expand Down Expand Up @@ -528,26 +528,8 @@ private static void convertHeaders(HttpHeaders headers, MimeHeaders cHeaders) {
if (headers.isEmpty()) {
return;
}

for (Entry<AsciiString, String> e : headers) {
final AsciiString k = e.getKey();
final String v = e.getValue();

if (k.isEmpty()) {
continue;
}

if (k.byteAt(0) != ':') {
final byte[] valueBytes = v.getBytes(StandardCharsets.US_ASCII);
cHeaders.addValue(k.array(), k.arrayOffset(), k.length())
.setBytes(valueBytes, 0, valueBytes.length);
} else if (HttpHeaderNames.AUTHORITY.equals(k) && !headers.contains(HttpHeaderNames.HOST)) {
// Convert `:authority` to `host`.
final byte[] valueBytes = v.getBytes(StandardCharsets.US_ASCII);
cHeaders.addValue(HOST_BYTES, 0, HOST_BYTES.length)
.setBytes(valueBytes, 0, valueBytes.length);
}
}
toHttp1Headers(headers, cHeaders,
(output, key, value) -> output.addValue(key.toString()).setString(value));
}

private static ResponseHeaders convertResponse(Response coyoteRes) {
Expand Down