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

Avoid setting default Content-Type in content methods without explicit MediaType #5964

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

Avoid setting default Content-Type in content methods without explicit MediaType #5964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7c485e
Avoid setting default Content-Type in `content(String)` to prevent du…
JiwonKKang Oct 30, 2024
fe57782
Add scenarios for testing duplicate content-type header settings
JiwonKKang Oct 30, 2024
c4676b1
Merge branch 'main' into duplicated_content_type_header
JiwonKKang Oct 30, 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
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,12 @@ public BlockingWebClientRequestPreparation content(MediaType contentType, byte[]
return this;
}

@Override
public BlockingWebClientRequestPreparation content(HttpData content) {
delegate.content(content);
return this;
}

@Override
public BlockingWebClientRequestPreparation content(MediaType contentType, HttpData content) {
delegate.content(contentType, content);
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/FutureTransformingRequestPreparation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,12 @@ public FutureTransformingRequestPreparation<T> content(MediaType contentType, by
return this;
}

@Override
public FutureTransformingRequestPreparation<T> content(HttpData content) {
delegate.content(content);
return this;
}

@Override
public FutureTransformingRequestPreparation<T> content(MediaType contentType, HttpData content) {
delegate.content(contentType, content);
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,12 @@ public RestClientPreparation content(MediaType contentType, byte[] content) {
return this;
}

@Override
public RestClientPreparation content(HttpData content) {
delegate.content(content);
return this;
}

@Override
public RestClientPreparation content(MediaType contentType, HttpData content) {
delegate.content(contentType, content);
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/TransformingRequestPreparation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ public TransformingRequestPreparation<T, R> content(MediaType contentType, byte[
return this;
}

@Override
public TransformingRequestPreparation<T, R> content(HttpData content) {
delegate.content(content);
return this;
}

@Override
public TransformingRequestPreparation<T, R> content(MediaType contentType,
HttpData content) {
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,11 @@ public WebClientRequestPreparation content(MediaType contentType, byte[] content
return (WebClientRequestPreparation) super.content(contentType, content);
}

@Override
public WebClientRequestPreparation content(HttpData content) {
return (WebClientRequestPreparation) super.content(content);
}

@Override
public WebClientRequestPreparation content(MediaType contentType, HttpData content) {
return (WebClientRequestPreparation) super.content(contentType, content);
Expand Down
22 changes: 17 additions & 5 deletions core/src/main/java/com/linecorp/armeria/common/AbstractHttpMessageBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,16 @@ public AbstractHttpMessageBuilder headers(

@Override
public AbstractHttpMessageBuilder content(String content) {
return content(MediaType.PLAIN_TEXT_UTF_8, content);
requireNonNull(content, "content");
return content(HttpData.of(MediaType.PLAIN_TEXT_UTF_8.charset(StandardCharsets.UTF_8),
content));
}

@Override
public AbstractHttpMessageBuilder content(MediaType contentType, CharSequence content) {
requireNonNull(contentType, "contentType");
requireNonNull(content, "content");
return content(contentType,
HttpData.of(contentType.charset(StandardCharsets.UTF_8),
return content(contentType, HttpData.of(contentType.charset(StandardCharsets.UTF_8),
content));
}

Expand All @@ -107,13 +108,16 @@ public AbstractHttpMessageBuilder content(MediaType contentType, String content)
requireNonNull(contentType, "contentType");
requireNonNull(content, "content");
return content(contentType, HttpData.of(contentType.charset(StandardCharsets.UTF_8),
content));
content));
}

@Override
@FormatMethod
public AbstractHttpMessageBuilder content(@FormatString String format, Object... content) {
return content(MediaType.PLAIN_TEXT_UTF_8, format, content);
requireNonNull(format, "format");
requireNonNull(content, "content");
return content(HttpData.of(MediaType.PLAIN_TEXT_UTF_8.charset(StandardCharsets.UTF_8),
format, content));
}

@Override
Expand All @@ -133,6 +137,14 @@ public AbstractHttpMessageBuilder content(MediaType contentType, byte[] content)
return content(contentType, HttpData.wrap(content));
}

@Override
public AbstractHttpMessageBuilder content(HttpData content) {
requireNonNull(content, "content");
checkState(publisher == null, "publisher has been set already");
this.content = content;
return this;
}

@Override
public AbstractHttpMessageBuilder content(MediaType contentType, HttpData content) {
requireNonNull(contentType, "contentType");
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/AbstractHttpRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ public AbstractHttpRequestBuilder content(MediaType contentType, byte[] content)
return (AbstractHttpRequestBuilder) super.content(contentType, content);
}

@Override
public AbstractHttpRequestBuilder content(HttpData content) {
return (AbstractHttpRequestBuilder) super.content(content);
}

@Override
public AbstractHttpRequestBuilder content(MediaType contentType, HttpData content) {
return (AbstractHttpRequestBuilder) super.content(contentType, content);
Expand Down
10 changes: 10 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/HttpMessageSetters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,16 @@ HttpMessageSetters content(MediaType contentType, @FormatString String format,
*/
HttpMessageSetters content(MediaType contentType, byte[] content);

/**
* Sets the content for this message without setting a content-type header.
* The {@code content} will be wrapped using {@link HttpData#wrap(byte[])},
* meaning any modifications made to {@code content} will be directly reflected in the request.
*
* <p>Note: This method does not set a content-type header. The caller is responsible
* for setting an appropriate content-type header if required.
*/
HttpMessageSetters content(HttpData content);

/**
* Sets the content for this message.
*/
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/HttpRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ public HttpRequestBuilder content(MediaType contentType, byte[] content) {
return (HttpRequestBuilder) super.content(contentType, content);
}

@Override
public HttpRequestBuilder content(HttpData content) {
return (HttpRequestBuilder) super.content(content);
}

@Override
public HttpRequestBuilder content(MediaType contentType, HttpData content) {
return (HttpRequestBuilder) super.content(contentType, content);
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/HttpRequestSetters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ HttpRequestSetters content(MediaType contentType, @FormatString String format,
@Override
HttpRequestSetters content(MediaType contentType, byte[] content);

/**
* Sets the content for this request.
*/
@Override
HttpRequestSetters content(HttpData content);

/**
* Sets the content for this request.
*/
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/HttpResponseBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,11 @@ public HttpResponseBuilder content(MediaType contentType, byte[] content) {
return (HttpResponseBuilder) super.content(contentType, content);
}

@Override
public HttpResponseBuilder content(HttpData content) {
return (HttpResponseBuilder) super.content(content);
}

/**
* Sets the content for this response.
*/
Expand Down
14 changes: 7 additions & 7 deletions core/src/test/java/com/linecorp/armeria/client/HttpClientExpect100HeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ void continueToSendRequestOnHttp1() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo\n")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo\n")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -167,7 +167,7 @@ void continueToSendRequestOnHttp2() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -217,7 +217,7 @@ void expectationFailedHttp1() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo\n")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo\n")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -268,7 +268,7 @@ void expectationFailedHttp2() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -314,7 +314,7 @@ void receiveResponseWithStandardFlowOnHttp1() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo\n")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo\n")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -369,7 +369,7 @@ void receiveResponseWithStandardFlowOnHttp2() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down Expand Up @@ -411,7 +411,7 @@ void timeoutFor100Continue() throws Exception {
final CompletableFuture<AggregatedHttpResponse> future =
client.prepare()
.post("/")
.content("foo\n")
.content(MediaType.PLAIN_TEXT_UTF_8, "foo\n")
.header(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE)
.execute()
.aggregate();
Expand Down
40 changes: 37 additions & 3 deletions core/src/test/java/com/linecorp/armeria/common/HttpResponseBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.nio.charset.StandardCharsets;
import java.util.List;

import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -102,7 +103,23 @@ void buildWithPlainContent() {
final AggregatedHttpResponse aggregatedRes = res.aggregate().join();
assertThat(aggregatedRes.status()).isEqualTo(HttpStatus.OK);
assertThat(aggregatedRes.contentUtf8()).isEqualTo("Armeriaはいろんな使い方がアルメリア");
assertThat(aggregatedRes.contentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8);
assertThat(aggregatedRes.contentType()).isNull();
}

@Test
void buildWithPlainAndContentTypeHeader() {
final HttpResponse res = HttpResponse.builder()
.ok()
.content("Armeriaはいろんな使い方がアルメリア")
.header(HttpHeaderNames.CONTENT_TYPE, MediaType.JSON)
.build();
final AggregatedHttpResponse aggregatedRes = res.aggregate().join();
assertThat(aggregatedRes.status()).isEqualTo(HttpStatus.OK);
assertThat(aggregatedRes.contentUtf8()).isEqualTo("Armeriaはいろんな使い方がアルメリア");

final List<String> contentTypeHeaders = aggregatedRes.headers().getAll(HttpHeaderNames.CONTENT_TYPE);
assertThat(contentTypeHeaders).hasSize(1);
assertThat(contentTypeHeaders).containsExactly("application/json");
}

@Test
Expand All @@ -116,7 +133,24 @@ void buildWithPlainFormat() {
final AggregatedHttpResponse aggregatedRes = res.aggregate().join();
assertThat(aggregatedRes.status()).isEqualTo(HttpStatus.OK);
assertThat(aggregatedRes.contentUtf8()).isEqualTo("Armeriaはいろんな使い方がアルメリア");
assertThat(aggregatedRes.contentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8);
assertThat(aggregatedRes.contentType()).isNull();
}

@Test
void buildWithPlainFormatAndContentTypeHeader() {
final HttpResponse res = HttpResponse.builder()
.ok()
.content("%sはいろんな使い方が%s",
"Armeria", "アルメリア")
.header(HttpHeaderNames.CONTENT_TYPE, MediaType.JSON)
.build();
final AggregatedHttpResponse aggregatedRes = res.aggregate().join();
assertThat(aggregatedRes.status()).isEqualTo(HttpStatus.OK);
assertThat(aggregatedRes.contentUtf8()).isEqualTo("Armeriaはいろんな使い方がアルメリア");

final List<String> contentTypeHeaders = aggregatedRes.headers().getAll(HttpHeaderNames.CONTENT_TYPE);
assertThat(contentTypeHeaders).hasSize(1);
assertThat(contentTypeHeaders).containsExactly("application/json");
}

@Test
Expand Down Expand Up @@ -256,7 +290,7 @@ void buildComplex() {
assertThat(aggregatedRes.headers().getAll(HttpHeaderNames.ACCEPT_ENCODING))
.containsExactly("gzip", "deflate", "gzip");
assertThat(aggregatedRes.contentUtf8()).isEqualTo("Armeriaはいろんな使い方がアルメリア");
assertThat(aggregatedRes.contentType()).isEqualTo(MediaType.PLAIN_TEXT_UTF_8);
assertThat(aggregatedRes.contentType()).isNull();
assertThat(aggregatedRes.trailers().contains("trailer-name")).isTrue();
assertThat(aggregatedRes.trailers().get("trailer-name")).isEqualTo("trailer-value");
}
Expand Down
1 change: 1 addition & 0 deletions core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ protected void configure(ServerBuilder sb) {
return HttpResponse.builder()
.ok()
.content("OK\n")
.header(HttpHeaderNames.CONTENT_TYPE, "text/plain; charset=utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can simply alter the behavior since it can result in breaking changes like seen in the tests.

Rather, I think a more reasonable approach would be:

  1. Remember if a String content has been set for a request/response
  2. At build time, if a content-type hasn't been set, set text/plain as the content-type

.header(HttpHeaderNames.CONNECTION, "close")
.build();
});
Expand Down
17 changes: 16 additions & 1 deletion ...la_2.13/src/main/scala/com/linecorp/armeria/client/scala/ScalaRestClientPreparation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,23 @@ import com.linecorp.armeria.client.{
RestClientPreparation
}
import com.linecorp.armeria.common.annotation.UnstableApi
import com.linecorp.armeria.common.{Cookie, ExchangeType, HttpData, HttpResponse, MediaType, ResponseEntity}
import com.linecorp.armeria.common.{
Cookie,
ExchangeType,
HttpData,
HttpMessageSetters,
HttpResponse,
MediaType,
ResponseEntity
}
import com.linecorp.armeria.scala.implicits._
import io.netty.util.AttributeKey

import java.lang.{Iterable => JIterable}
import java.time.Duration
import java.util.{Map => JMap}
import org.reactivestreams.Publisher

import scala.collection.immutable
import scala.concurrent.Future

Expand Down Expand Up @@ -186,6 +196,11 @@ final class ScalaRestClientPreparation private[scala] (delegate: RestClientPrepa
this
}

override def content(content: HttpData): ScalaRestClientPreparation = {
delegate.content(content)
this
}

override def content(contentType: MediaType, content: HttpData): ScalaRestClientPreparation = {
delegate.content(contentType, content)
this
Expand Down
Loading