diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 98611e4c9d8ab..36189381c5b22 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java @@ -99,6 +99,10 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter")); } + if (request.hasContent() && request.isContentConsumed() == false) { + throw new IllegalArgumentException("request [" + request.method() + " " + request.path() + "] does not support having a body"); + } + usageCount.increment(); // execute the action action.accept(channel); diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 813d6feb55167..fe976ee4ddce6 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -66,6 +66,12 @@ public class RestRequest implements ToXContent.Params { private final HttpRequest httpRequest; private final HttpChannel httpChannel; + private boolean contentConsumed = false; + + public boolean isContentConsumed() { + return contentConsumed; + } + protected RestRequest(NamedXContentRegistry xContentRegistry, Map params, String path, Map> headers, HttpRequest httpRequest, HttpChannel httpChannel) { final XContentType xContentType; @@ -173,10 +179,15 @@ public final String path() { } public boolean hasContent() { - return content().length() > 0; + return content(false).length() > 0; } public BytesReference content() { + return content(true); + } + + protected BytesReference content(final boolean contentConsumed) { + this.contentConsumed = this.contentConsumed | contentConsumed; return httpRequest.content(); } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java index 6ec4cec77193e..2d9d691d2c71a 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java @@ -34,7 +34,8 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public class RestForceMergeAction extends BaseRestHandler { - public RestForceMergeAction(Settings settings, RestController controller) { + + public RestForceMergeAction(final Settings settings, final RestController controller) { super(settings); controller.registerHandler(POST, "/_forcemerge", this); controller.registerHandler(POST, "/{index}/_forcemerge", this); @@ -47,14 +48,12 @@ public String getName() { @Override public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { - if (request.hasContent()) { - throw new IllegalArgumentException("forcemerge takes arguments in query parameters, not in the request body"); - } - ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index"))); + final ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index"))); mergeRequest.indicesOptions(IndicesOptions.fromRequest(request, mergeRequest.indicesOptions())); mergeRequest.maxNumSegments(request.paramAsInt("max_num_segments", mergeRequest.maxNumSegments())); mergeRequest.onlyExpungeDeletes(request.paramAsBoolean("only_expunge_deletes", mergeRequest.onlyExpungeDeletes())); mergeRequest.flush(request.paramAsBoolean("flush", mergeRequest.flush())); return channel -> client.admin().indices().forceMerge(mergeRequest, new RestToXContentListener<>(channel)); } + } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java index aeb5beb09e2fc..2d4093d8525d9 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java @@ -28,6 +28,7 @@ import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestChannel; import org.elasticsearch.test.rest.FakeRestRequest; import static org.hamcrest.Matchers.equalTo; @@ -39,9 +40,12 @@ public void testBodyRejection() throws Exception { final RestForceMergeAction handler = new RestForceMergeAction(Settings.EMPTY, mock(RestController.class)); String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString(); final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) - .withContent(new BytesArray(json), XContentType.JSON).build(); + .withContent(new BytesArray(json), XContentType.JSON) + .withPath("/_forcemerge") + .build(); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> handler.prepareRequest(request, mock(NodeClient.class))); - assertThat(e.getMessage(), equalTo("forcemerge takes arguments in query parameters, not in the request body")); + () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class))); + assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body")); } + } diff --git a/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java b/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java index 835dd7cd9fab0..68e3c8416b9c2 100644 --- a/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java @@ -21,7 +21,11 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.Table; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.rest.action.cat.AbstractCatAction; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestChannel; @@ -232,4 +236,78 @@ public String getName() { assertTrue(executed.get()); } + public void testConsumedBody() throws Exception { + final AtomicBoolean executed = new AtomicBoolean(); + final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { + @Override + protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { + request.content(); + return channel -> executed.set(true); + } + + @Override + public String getName() { + return "test_consumed_body"; + } + + }; + + try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) { + final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .withContent(new BytesArray(builder.toString()), XContentType.JSON) + .build(); + final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); + handler.handleRequest(request, channel, mock(NodeClient.class)); + assertTrue(executed.get()); + } + } + + public void testUnconsumedNoBody() throws Exception { + final AtomicBoolean executed = new AtomicBoolean(); + final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { + @Override + protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { + return channel -> executed.set(true); + } + + @Override + public String getName() { + return "test_unconsumed_body"; + } + + }; + + final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build(); + final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); + handler.handleRequest(request, channel, mock(NodeClient.class)); + assertTrue(executed.get()); + } + + public void testUnconsumedBody() throws IOException { + final AtomicBoolean executed = new AtomicBoolean(); + final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) { + @Override + protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException { + return channel -> executed.set(true); + } + + @Override + public String getName() { + return "test_unconsumed_body"; + } + + }; + + try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) { + final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .withContent(new BytesArray(builder.toString()), XContentType.JSON) + .build(); + final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class))); + assertThat(e, hasToString(containsString("request [GET /] does not support having a body"))); + assertFalse(executed.get()); + } + } + } diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 3ad9c61de3c8e..8a2994a69816b 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -20,12 +20,15 @@ package org.elasticsearch.rest; import org.elasticsearch.ElasticsearchParseException; -import org.elasticsearch.common.Strings; +import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.MapBuilder; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.http.HttpChannel; +import org.elasticsearch.http.HttpRequest; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestRequest; @@ -41,8 +44,62 @@ import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class RestRequestTests extends ESTestCase { + + public void testContentConsumesContent() { + runConsumesContentTest(RestRequest::content, true); + } + + public void testRequiredContentConsumesContent() { + runConsumesContentTest(RestRequest::requiredContent, true); + } + + public void testContentParserConsumesContent() { + runConsumesContentTest(RestRequest::contentParser, true); + } + + public void testContentOrSourceParamConsumesContent() { + runConsumesContentTest(RestRequest::contentOrSourceParam, true); + } + + public void testContentOrSourceParamsParserConsumesContent() { + runConsumesContentTest(RestRequest::contentOrSourceParamParser, true); + } + + public void testWithContentOrSourceParamParserOrNullConsumesContent() { + @SuppressWarnings("unchecked") CheckedConsumer consumer = mock(CheckedConsumer.class); + runConsumesContentTest(request -> request.withContentOrSourceParamParserOrNull(consumer), true); + } + + public void testApplyContentParserConsumesContent() { + @SuppressWarnings("unchecked") CheckedConsumer consumer = mock(CheckedConsumer.class); + runConsumesContentTest(request -> request.applyContentParser(consumer), true); + } + + public void testHasContentDoesNotConsumesContent() { + runConsumesContentTest(RestRequest::hasContent, false); + } + + private void runConsumesContentTest( + final CheckedConsumer consumer, final boolean expected) { + final HttpRequest httpRequest = mock(HttpRequest.class); + when (httpRequest.uri()).thenReturn(""); + when (httpRequest.content()).thenReturn(new BytesArray(new byte[1])); + final RestRequest request = + RestRequest.request(mock(NamedXContentRegistry.class), httpRequest, mock(HttpChannel.class)); + request.setXContentType(XContentType.JSON); + assertFalse(request.isContentConsumed()); + try { + consumer.accept(request); + } catch (final Exception e) { + throw new RuntimeException(e); + } + assertThat(request.isContentConsumed(), equalTo(expected)); + } + public void testContentParser() throws IOException { Exception e = expectThrows(ElasticsearchParseException.class, () -> contentRestRequest("", emptyMap()).contentParser()); @@ -211,14 +268,10 @@ public String uri() { return restRequest.uri(); } - @Override - public boolean hasContent() { - return Strings.hasLength(content()); - } - @Override public BytesReference content() { return restRequest.content(); } } + } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 2aec495390b6c..a659d6af5c6aa 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -48,11 +48,6 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http super(xContentRegistry, params, httpRequest.uri(), httpRequest.getHeaders(), httpRequest, httpChannel); } - @Override - public boolean hasContent() { - return content() != null; - } - private static class FakeHttpRequest implements HttpRequest { private final Method method; @@ -166,7 +161,7 @@ public static class Builder { private Map params = new HashMap<>(); - private BytesReference content; + private BytesReference content = BytesArray.EMPTY; private String path = "/";