From 673754b1d5897a975ca03f495ded582f377053a3 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 14 Feb 2017 16:37:22 -0500 Subject: [PATCH] Fix get source HEAD requests Get source HEAD requests incorrectly return a content-length header of 0. This commit addresses this by removing the special handling for get source HEAD requests, and just relying on the general mechanism that exists for handling HEAD requests in the REST layer. Relates #23151 --- .../elasticsearch/action/ActionModule.java | 1 - .../action/document/RestGetSourceAction.java | 20 +++++-- .../rest/action/document/RestHeadAction.java | 23 +------- .../rest/Netty4HeadBodyIsEmptyIT.java | 57 ++++++++++++++++++- 4 files changed, 71 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/ActionModule.java b/core/src/main/java/org/elasticsearch/action/ActionModule.java index 9251e29fb96..25b500615fe 100644 --- a/core/src/main/java/org/elasticsearch/action/ActionModule.java +++ b/core/src/main/java/org/elasticsearch/action/ActionModule.java @@ -564,7 +564,6 @@ public class ActionModule extends AbstractModule { registerHandler.accept(new RestGetAction(settings, restController)); registerHandler.accept(new RestGetSourceAction(settings, restController)); registerHandler.accept(new RestHeadAction.Document(settings, restController)); - registerHandler.accept(new RestHeadAction.Source(settings, restController)); registerHandler.accept(new RestMultiGetAction(settings, restController)); registerHandler.accept(new RestDeleteAction(settings, restController)); registerHandler.accept(new org.elasticsearch.rest.action.document.RestCountAction(settings, restController)); diff --git a/core/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java b/core/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java index 83d424ed74d..341c1ddc917 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/document/RestGetSourceAction.java @@ -36,13 +36,19 @@ import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import java.io.IOException; import static org.elasticsearch.rest.RestRequest.Method.GET; +import static org.elasticsearch.rest.RestRequest.Method.HEAD; import static org.elasticsearch.rest.RestStatus.NOT_FOUND; import static org.elasticsearch.rest.RestStatus.OK; +/** + * The REST handler for get source and head source APIs. + */ public class RestGetSourceAction extends BaseRestHandler { - public RestGetSourceAction(Settings settings, RestController controller) { + + public RestGetSourceAction(final Settings settings, final RestController controller) { super(settings); controller.registerHandler(GET, "/{index}/{type}/{id}/_source", this); + controller.registerHandler(HEAD, "/{index}/{type}/{id}/_source", this); } @Override @@ -50,7 +56,7 @@ public class RestGetSourceAction extends BaseRestHandler { final GetRequest getRequest = new GetRequest(request.param("index"), request.param("type"), request.param("id")); getRequest.operationThreaded(true); getRequest.refresh(request.paramAsBoolean("refresh", getRequest.refresh())); - getRequest.routing(request.param("routing")); // order is important, set it after routing, so it will set the routing + getRequest.routing(request.param("routing")); getRequest.parent(request.param("parent")); getRequest.preference(request.param("preference")); getRequest.realtime(request.paramAsBoolean("realtime", getRequest.realtime())); @@ -59,15 +65,16 @@ public class RestGetSourceAction extends BaseRestHandler { return channel -> { if (getRequest.fetchSourceContext() != null && !getRequest.fetchSourceContext().fetchSource()) { - ActionRequestValidationException validationError = new ActionRequestValidationException(); + final ActionRequestValidationException validationError = new ActionRequestValidationException(); validationError.addValidationError("fetching source can not be disabled"); channel.sendResponse(new BytesRestResponse(channel, validationError)); } else { client.get(getRequest, new RestResponseListener(channel) { @Override - public RestResponse buildResponse(GetResponse response) throws Exception { - XContentBuilder builder = channel.newBuilder(request.getXContentType(), false); - if (response.isSourceEmpty()) { // check if doc source (or doc itself) is missing + public RestResponse buildResponse(final GetResponse response) throws Exception { + final XContentBuilder builder = channel.newBuilder(request.getXContentType(), false); + // check if doc source (or doc itself) is missing + if (response.isSourceEmpty()) { return new BytesRestResponse(NOT_FOUND, builder); } else { builder.rawValue(response.getSourceInternal()); @@ -78,4 +85,5 @@ public class RestGetSourceAction extends BaseRestHandler { } }; } + } diff --git a/core/src/main/java/org/elasticsearch/rest/action/document/RestHeadAction.java b/core/src/main/java/org/elasticsearch/rest/action/document/RestHeadAction.java index cc90b2cfa0f..14e412aebe1 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/document/RestHeadAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/document/RestHeadAction.java @@ -48,33 +48,18 @@ public abstract class RestHeadAction extends BaseRestHandler { */ public static class Document extends RestHeadAction { public Document(Settings settings, RestController controller) { - super(settings, false); + super(settings); controller.registerHandler(HEAD, "/{index}/{type}/{id}", this); } } - /** - * Handler to check for document source existence (may be disabled in the mapping). - */ - public static class Source extends RestHeadAction { - public Source(Settings settings, RestController controller) { - super(settings, true); - controller.registerHandler(HEAD, "/{index}/{type}/{id}/_source", this); - } - } - - private final boolean source; - /** * All subclasses must be registered in {@link org.elasticsearch.common.network.NetworkModule}. + * @param settings injected settings * - * @param settings injected settings - * @param source {@code false} to check for {@link GetResponse#isExists()}. - * {@code true} to also check for {@link GetResponse#isSourceEmpty()}. */ - public RestHeadAction(Settings settings, boolean source) { + public RestHeadAction(Settings settings) { super(settings); - this.source = source; } @Override @@ -95,8 +80,6 @@ public abstract class RestHeadAction extends BaseRestHandler { public RestResponse buildResponse(GetResponse response) { if (!response.isExists()) { return new BytesRestResponse(NOT_FOUND, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); - } else if (source && response.isSourceEmpty()) { // doc exists, but source might not (disabled in the mapping) - return new BytesRestResponse(NOT_FOUND, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); } else { return new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY); } 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 index 803f80946f0..ac8e8ee0d0a 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/rest/Netty4HeadBodyIsEmptyIT.java @@ -31,6 +31,9 @@ import java.util.Map; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.elasticsearch.rest.RestStatus.NOT_FOUND; +import static org.elasticsearch.rest.RestStatus.OK; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -43,7 +46,18 @@ public class Netty4HeadBodyIsEmptyIT extends ESRestTestCase { } private void createTestDoc() throws IOException { - client().performRequest("PUT", "test/test/1", emptyMap(), new StringEntity("{\"test\": \"test\"}")); + createTestDoc("test", "test"); + } + + private void createTestDoc(final String indexName, final String typeName) throws IOException { + try (XContentBuilder builder = jsonBuilder()) { + builder.startObject(); + { + builder.field("test", "test"); + } + builder.endObject(); + client().performRequest("PUT", "/" + indexName + "/" + typeName + "/" + "1", emptyMap(), new StringEntity(builder.string())); + } } public void testDocumentExists() throws IOException { @@ -110,9 +124,46 @@ public class Netty4HeadBodyIsEmptyIT extends ESRestTestCase { } } - private void headTestCase(String url, Map params, Matcher matcher) throws IOException { + public void testGetSourceAction() throws IOException { + createTestDoc(); + headTestCase("/test/test/1/_source", emptyMap(), greaterThan(0)); + headTestCase("/test/test/2/_source", emptyMap(), NOT_FOUND.getStatus(), equalTo(0)); + + try (XContentBuilder builder = jsonBuilder()) { + builder.startObject(); + { + builder.startObject("mappings"); + { + builder.startObject("test-no-source"); + { + builder.startObject("_source"); + { + builder.field("enabled", false); + } + builder.endObject(); + } + builder.endObject(); + } + builder.endObject(); + } + builder.endObject(); + client().performRequest("PUT", "/test-no-source", emptyMap(), new StringEntity(builder.string())); + createTestDoc("test-no-source", "test-no-source"); + headTestCase("/test-no-source/test-no-source/1/_source", emptyMap(), NOT_FOUND.getStatus(), equalTo(0)); + } + } + + private void headTestCase(final String url, final Map params, final Matcher matcher) throws IOException { + headTestCase(url, params, OK.getStatus(), matcher); + } + + private void headTestCase( + final String url, + final Map params, + final int expectedStatusCode, + final Matcher matcher) throws IOException { Response response = client().performRequest("HEAD", url, params); - assertEquals(200, response.getStatusLine().getStatusCode()); + assertEquals(expectedStatusCode, response.getStatusLine().getStatusCode()); assertThat(Integer.valueOf(response.getHeader("Content-Length")), matcher); assertNull("HEAD requests shouldn't have a response body but " + url + " did", response.getEntity()); }