From 9c3e4d6e228a16aa6660b77f80797999a8ee629f Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 25 Oct 2016 23:08:19 -0400 Subject: [PATCH] Add correct Content-Length on HEAD requests This commit fixes responses to HEAD requests so that the value of the Content-Length is correct per the HTTP spec. Namely, the value of this header should be equal to the Content-Length if the request were not a HEAD request. This commit also fixes a memory leak on HEAD requests to the main action that arose from the bytes on a builder not being released due to them being dropped on the floor to ensure that the response to the main action did not have a body. Relates #21123 --- .../rest/action/RestMainAction.java | 3 -- .../rest/action/RestMainActionTests.java | 4 +-- .../http/netty3/Netty3HttpChannel.java | 8 ++++- .../rest/Netty3HeadBodyIsEmptyIT.java | 23 +++++++++++++ .../http/netty4/Netty4HttpChannel.java | 10 ++++-- .../rest/Netty4HeadBodyIsEmptyIT.java | 23 +++++++++++++ .../rest/HeadBodyIsEmptyIntegTestCase.java | 32 ++++++++++--------- 7 files changed, 80 insertions(+), 23 deletions(-) create mode 100644 modules/transport-netty3/src/test/java/org/elasticsearch/rest/Netty3HeadBodyIsEmptyIT.java create mode 100644 modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java rename distribution/integ-test-zip/src/test/java/org/elasticsearch/test/rest/HeadBodyIsEmptyIT.java => test/framework/src/main/java/org/elasticsearch/rest/HeadBodyIsEmptyIntegTestCase.java (70%) diff --git a/core/src/main/java/org/elasticsearch/rest/action/RestMainAction.java b/core/src/main/java/org/elasticsearch/rest/action/RestMainAction.java index c1c1e220b28..210ccb2e227 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/RestMainAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/RestMainAction.java @@ -60,9 +60,6 @@ public class RestMainAction extends BaseRestHandler { static BytesRestResponse convertMainResponse(MainResponse response, RestRequest request, XContentBuilder builder) throws IOException { RestStatus status = response.isAvailable() ? RestStatus.OK : RestStatus.SERVICE_UNAVAILABLE; - if (request.method() == RestRequest.Method.HEAD) { - return new BytesRestResponse(status, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); - } // Default to pretty printing, but allow ?pretty=false to disable if (request.hasParam("pretty") == false) { diff --git a/core/src/test/java/org/elasticsearch/rest/action/RestMainActionTests.java b/core/src/test/java/org/elasticsearch/rest/action/RestMainActionTests.java index bef1ed44ac6..449f5852cfa 100644 --- a/core/src/test/java/org/elasticsearch/rest/action/RestMainActionTests.java +++ b/core/src/test/java/org/elasticsearch/rest/action/RestMainActionTests.java @@ -61,9 +61,9 @@ public class RestMainActionTests extends ESTestCase { BytesRestResponse response = RestMainAction.convertMainResponse(mainResponse, restRequest, builder); assertNotNull(response); assertEquals(expectedStatus, response.status()); - assertEquals(0, response.content().length()); - assertEquals(0, builder.bytes().length()); + // the empty responses are handled in the HTTP layer so we do + // not assert on them here } public void testGetResponse() throws Exception { diff --git a/modules/transport-netty3/src/main/java/org/elasticsearch/http/netty3/Netty3HttpChannel.java b/modules/transport-netty3/src/main/java/org/elasticsearch/http/netty3/Netty3HttpChannel.java index a715abfd877..d79fae21b8c 100644 --- a/modules/transport-netty3/src/main/java/org/elasticsearch/http/netty3/Netty3HttpChannel.java +++ b/modules/transport-netty3/src/main/java/org/elasticsearch/http/netty3/Netty3HttpChannel.java @@ -33,6 +33,7 @@ import org.elasticsearch.rest.AbstractRestChannel; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.jboss.netty.buffer.ChannelBuffer; +import org.jboss.netty.buffer.ChannelBuffers; import org.jboss.netty.channel.Channel; import org.jboss.netty.channel.ChannelFuture; import org.jboss.netty.channel.ChannelFutureListener; @@ -41,6 +42,7 @@ import org.jboss.netty.handler.codec.http.CookieDecoder; import org.jboss.netty.handler.codec.http.CookieEncoder; import org.jboss.netty.handler.codec.http.DefaultHttpResponse; import org.jboss.netty.handler.codec.http.HttpHeaders; +import org.jboss.netty.handler.codec.http.HttpMethod; import org.jboss.netty.handler.codec.http.HttpResponse; import org.jboss.netty.handler.codec.http.HttpResponseStatus; import org.jboss.netty.handler.codec.http.HttpVersion; @@ -109,7 +111,11 @@ public final class Netty3HttpChannel extends AbstractRestChannel { boolean addedReleaseListener = false; try { buffer = Netty3Utils.toChannelBuffer(content); - resp.setContent(buffer); + if (HttpMethod.HEAD.equals(nettyRequest.getMethod())) { + resp.setContent(ChannelBuffers.EMPTY_BUFFER); + } else { + resp.setContent(buffer); + } // If our response doesn't specify a content-type header, set one setHeaderField(resp, HttpHeaders.Names.CONTENT_TYPE, response.contentType(), false); diff --git a/modules/transport-netty3/src/test/java/org/elasticsearch/rest/Netty3HeadBodyIsEmptyIT.java b/modules/transport-netty3/src/test/java/org/elasticsearch/rest/Netty3HeadBodyIsEmptyIT.java new file mode 100644 index 00000000000..82e5ff9b666 --- /dev/null +++ b/modules/transport-netty3/src/test/java/org/elasticsearch/rest/Netty3HeadBodyIsEmptyIT.java @@ -0,0 +1,23 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest; + +public class Netty3HeadBodyIsEmptyIT extends HeadBodyIsEmptyIntegTestCase { +} diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java index e9bb129cf6d..efcc8d1f2ea 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpChannel.java @@ -20,6 +20,7 @@ package org.elasticsearch.http.netty4; import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; import io.netty.channel.Channel; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelPromise; @@ -29,6 +30,7 @@ import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpHeaders; +import io.netty.handler.codec.http.HttpMethod; import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.HttpVersion; @@ -87,13 +89,17 @@ final class Netty4HttpChannel extends AbstractRestChannel { return new ReleasableBytesStreamOutput(transport.bigArrays); } - @Override public void sendResponse(RestResponse response) { // if the response object was created upstream, then use it; // otherwise, create a new one ByteBuf buffer = Netty4Utils.toByteBuf(response.content()); - FullHttpResponse resp = newResponse(buffer); + final FullHttpResponse resp; + if (HttpMethod.HEAD.equals(nettyRequest.method())) { + resp = newResponse(Unpooled.EMPTY_BUFFER); + } else { + resp = newResponse(buffer); + } resp.setStatus(getStatus(response.status())); Netty4CorsHandler.setCorsResponseHeaders(nettyRequest, resp, transport.getCorsConfig()); diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java new file mode 100644 index 00000000000..8716f59ee00 --- /dev/null +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java @@ -0,0 +1,23 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest; + +public class Netty4HeadBodyIsEmptyIT extends HeadBodyIsEmptyIntegTestCase { +} diff --git a/distribution/integ-test-zip/src/test/java/org/elasticsearch/test/rest/HeadBodyIsEmptyIT.java b/test/framework/src/main/java/org/elasticsearch/rest/HeadBodyIsEmptyIntegTestCase.java similarity index 70% rename from distribution/integ-test-zip/src/test/java/org/elasticsearch/test/rest/HeadBodyIsEmptyIT.java rename to test/framework/src/main/java/org/elasticsearch/rest/HeadBodyIsEmptyIntegTestCase.java index c3a19e5ab34..0e43814b75c 100644 --- a/distribution/integ-test-zip/src/test/java/org/elasticsearch/test/rest/HeadBodyIsEmptyIT.java +++ b/test/framework/src/main/java/org/elasticsearch/rest/HeadBodyIsEmptyIntegTestCase.java @@ -17,10 +17,12 @@ * under the License. */ -package org.elasticsearch.test.rest; +package org.elasticsearch.rest; import org.apache.http.entity.StringEntity; import org.elasticsearch.client.Response; +import org.elasticsearch.test.rest.ESRestTestCase; +import org.hamcrest.Matcher; import java.io.IOException; import java.io.UnsupportedEncodingException; @@ -28,15 +30,17 @@ import java.util.Map; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; /** * Tests that HTTP HEAD requests don't respond with a body. */ -public class HeadBodyIsEmptyIT extends ESRestTestCase { +public class HeadBodyIsEmptyIntegTestCase extends ESRestTestCase { public void testHeadRoot() throws IOException { - headTestCase("/", emptyMap()); - headTestCase("/", singletonMap("pretty", "")); - headTestCase("/", singletonMap("pretty", "true")); + headTestCase("/", emptyMap(), greaterThan(0)); + headTestCase("/", singletonMap("pretty", ""), greaterThan(0)); + headTestCase("/", singletonMap("pretty", "true"), greaterThan(0)); } private void createTestDoc() throws UnsupportedEncodingException, IOException { @@ -45,28 +49,26 @@ public class HeadBodyIsEmptyIT extends ESRestTestCase { public void testDocumentExists() throws IOException { createTestDoc(); - headTestCase("test/test/1", emptyMap()); - headTestCase("test/test/1", singletonMap("pretty", "true")); + headTestCase("test/test/1", emptyMap(), equalTo(0)); + headTestCase("test/test/1", singletonMap("pretty", "true"), equalTo(0)); } public void testIndexExists() throws IOException { createTestDoc(); - headTestCase("test", emptyMap()); - headTestCase("test", singletonMap("pretty", "true")); + headTestCase("test", emptyMap(), equalTo(0)); + headTestCase("test", singletonMap("pretty", "true"), equalTo(0)); } public void testTypeExists() throws IOException { createTestDoc(); - headTestCase("test/test", emptyMap()); - headTestCase("test/test", singletonMap("pretty", "true")); + headTestCase("test/test", emptyMap(), equalTo(0)); + headTestCase("test/test", singletonMap("pretty", "true"), equalTo(0)); } - private void headTestCase(String url, Map params) throws IOException { + private void headTestCase(String url, Map params, Matcher matcher) throws IOException { Response response = client().performRequest("HEAD", url, params); assertEquals(200, response.getStatusLine().getStatusCode()); - /* Check that the content-length header is always 0. This isn't what we should be doing in the long run but it is what we expect - * that we are *actually* doing. */ - assertEquals("We expect HEAD requests to have 0 Content-Length but " + url + " didn't", "0", response.getHeader("Content-Length")); + assertThat(Integer.valueOf(response.getHeader("Content-Length")), matcher); assertNull("HEAD requests shouldn't have a response body but " + url + " did", response.getEntity()); } }