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.

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

Fixes missing Content-Length header when body is empty #1778

Merged
merged 3 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 17 additions & 6 deletions core/src/main/java/feign/Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class Default implements Client {

/**
* Disable the request body internal buffering for {@code HttpURLConnection}.
*
*
* @see HttpURLConnection#setFixedLengthStreamingMode(int)
* @see HttpURLConnection#setFixedLengthStreamingMode(long)
* @see HttpURLConnection#setChunkedStreamingMode(int)
Expand All @@ -74,7 +74,7 @@ class Default implements Client {

/**
* Create a new client, which disable request buffering by default.
*
*
* @param sslContextFactory SSLSocketFactory for secure https URL connections.
* @param hostnameVerifier the host name verifier.
*/
Expand All @@ -86,7 +86,7 @@ public Default(SSLSocketFactory sslContextFactory, HostnameVerifier hostnameVeri

/**
* Create a new client.
*
*
* @param sslContextFactory SSLSocketFactory for secure https URL connections.
* @param hostnameVerifier the host name verifier.
* @param disableRequestBuffering Disable the request body internal buffering for
Expand Down Expand Up @@ -196,8 +196,19 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
connection.addRequestProperty("Accept", "*/*");
}

if (request.body() != null) {
if (disableRequestBuffering) {
boolean hasEmptyBody = false;
byte[] body = request.body();
if (body == null && request.httpMethod().isWithBody()) {
body = new byte[0];
hasEmptyBody = true;
}

if (body != null) {
/*
* Ignore disableRequestBuffering flag if the empty body was set, to ensure that internal
* retry logic applies to such requests.
*/
if (disableRequestBuffering && !hasEmptyBody) {
if (contentLength != null) {
connection.setFixedLengthStreamingMode(contentLength);
} else {
Expand All @@ -212,7 +223,7 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
out = new DeflaterOutputStream(out);
}
try {
out.write(request.body());
out.write(body);
} finally {
try {
out.close();
Expand Down
16 changes: 15 additions & 1 deletion core/src/main/java/feign/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,21 @@
public final class Request implements Serializable {

public enum HttpMethod {
GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE, PATCH
GET, HEAD, POST(true), PUT(true), DELETE, CONNECT, OPTIONS, TRACE, PATCH(true);

private final boolean withBody;

HttpMethod() {
this(false);
}

HttpMethod(boolean withBody) {
this.withBody = withBody;
}

public boolean isWithBody() {
return this.withBody;
}
}

public enum ProtocolVersion {
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/feign/client/AbstractClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ protected void log(String configKey, String format, Object... args) {}
}

@Test
public void noResponseBodyForPost() {
public void noResponseBodyForPost() throws Exception {
server.enqueue(new MockResponse());

TestInterface api = newBuilder()
Expand All @@ -213,7 +213,7 @@ public void noResponseBodyForPost() {
}

@Test
public void noResponseBodyForPut() {
public void noResponseBodyForPut() throws Exception {
server.enqueue(new MockResponse());

TestInterface api = newBuilder()
Expand Down
51 changes: 32 additions & 19 deletions core/src/test/java/feign/client/DefaultClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,17 @@
*/
package feign.client;

import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.core.Is.isA;
import static org.junit.Assert.assertEquals;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import feign.Client;
import feign.Client.Proxied;
import feign.Feign;
import feign.Feign.Builder;
import feign.RetryableException;
import feign.assertj.MockWebServerAssertions;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.SocketPolicy;
import org.junit.Test;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLSession;
import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.InetSocketAddress;
Expand All @@ -26,20 +32,11 @@
import java.net.Proxy.Type;
import java.net.SocketAddress;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.zip.DeflaterOutputStream;
import java.util.zip.GZIPOutputStream;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLSession;
import okio.Buffer;
import org.junit.Test;
import feign.Client;
import feign.Client.Proxied;
import feign.Feign;
import feign.Feign.Builder;
import feign.RetryableException;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.SocketPolicy;
import java.util.Collections;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;
import static org.hamcrest.core.Is.isA;
import static org.junit.Assert.assertEquals;

/**
* Tests client-specific behavior, such as ensuring Content-Length is sent when specified.
Expand Down Expand Up @@ -97,6 +94,22 @@ public void testPatch() throws Exception {
super.testPatch();
}

@Override
public void noResponseBodyForPost() throws Exception {
super.noResponseBodyForPost();
MockWebServerAssertions.assertThat(server.takeRequest())
.hasMethod("POST")
.hasHeaders(entry("Content-Length", Collections.singletonList("0")));
}

@Override
public void noResponseBodyForPut() throws Exception {
super.noResponseBodyForPut();
MockWebServerAssertions.assertThat(server.takeRequest())
.hasMethod("PUT")
.hasHeaders(entry("Content-Length", Collections.singletonList("0")));
}

@Test
@Override
public void noResponseBodyForPatch() {
Expand Down
10 changes: 0 additions & 10 deletions java11/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,6 @@
<scope>test</scope>
</dependency>

<dependency>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicated dependencies

<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.github.openfeign</groupId>
<artifactId>feign-core</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion jaxrs2/src/test/java/feign/jaxrs2/JAXRSClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public void testPatch() throws Exception {
}

@Override
public void noResponseBodyForPut() {
public void noResponseBodyForPut() throws Exception {
try {
super.noResponseBodyForPut();
} catch (final IllegalStateException e) {
Expand Down
5 changes: 1 addition & 4 deletions okhttp/src/main/java/feign/okhttp/OkHttpClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,7 @@ static Request toOkHttpRequest(feign.Request input) {
}

byte[] inputBody = input.body();
boolean isMethodWithBody =
HttpMethod.POST == input.httpMethod() || HttpMethod.PUT == input.httpMethod()
|| HttpMethod.PATCH == input.httpMethod();
if (isMethodWithBody) {
if (input.httpMethod().isWithBody()) {
requestBuilder.removeHeader("Content-Type");
if (inputBody == null) {
// write an empty BODY to conform with okhttp 2.4.0+
Expand Down
4 changes: 0 additions & 4 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -517,13 +517,9 @@
</dependencies>
</plugin>

<!-- Ensures checksums are added to published jars -->
<plugin>
<artifactId>maven-install-plugin</artifactId>
<version>${maven-install-plugin.version}</version>
<configuration>
<createChecksum>true</createChecksum>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

</configuration>
</plugin>

<plugin>
Expand Down