RestController should not consume request content

client/rest-high-level/src/main/java/org/elasticsearch/client/RollupRequestConverters.java

@@ -90,9 +90,7 @@ static Request deleteJob(final DeleteRollupJobRequest deleteRollupJobRequest) th
             .addPathPartAsIs("_rollup", "job")
             .addPathPart(deleteRollupJobRequest.getId())
             .build();
-        Request request = new Request(HttpDelete.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(deleteRollupJobRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpDelete.METHOD_NAME, endpoint);
     }
 
     static Request search(final SearchRequest request) throws IOException {
@@ -114,18 +112,14 @@ static Request getRollupCaps(final GetRollupCapsRequest getRollupCapsRequest) th
             .addPathPartAsIs("_rollup", "data")
             .addPathPart(getRollupCapsRequest.getIndexPattern())
             .build();
-        Request request = new Request(HttpGet.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(getRollupCapsRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpGet.METHOD_NAME, endpoint);
     }
 
     static Request getRollupIndexCaps(final GetRollupIndexCapsRequest getRollupIndexCapsRequest) throws IOException {
         String endpoint = new RequestConverters.EndpointBuilder()
             .addCommaSeparatedPathParts(getRollupIndexCapsRequest.indices())
             .addPathPartAsIs("_rollup", "data")
             .build();
-        Request request = new Request(HttpGet.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(getRollupIndexCapsRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpGet.METHOD_NAME, endpoint);
     }
 }

client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/DeleteRollupJobRequest.java

@@ -19,22 +19,14 @@
 package org.elasticsearch.client.rollup;
 
 import org.elasticsearch.client.Validatable;
-import org.elasticsearch.common.ParseField;
-import org.elasticsearch.common.xcontent.ConstructingObjectParser;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentParser;
 
-import java.io.IOException;
 import java.util.Objects;
 
 
-public class DeleteRollupJobRequest implements Validatable, ToXContentObject {
+public class DeleteRollupJobRequest implements Validatable {
 
-    private static final ParseField ID_FIELD = new ParseField("id");
     private final String id;
 
-
     public DeleteRollupJobRequest(String id) {
         this.id = Objects.requireNonNull(id, "id parameter must not be null");
     }
@@ -43,27 +35,6 @@ public String getId() {
         return id;
     }
 
-    private static final ConstructingObjectParser<DeleteRollupJobRequest, Void> PARSER =
-        new ConstructingObjectParser<>("request",  a -> {
-            return new DeleteRollupJobRequest((String) a[0]);
-        });
-
-    static {
-        PARSER.declareString(ConstructingObjectParser.constructorArg(), ID_FIELD);
-    }
-
-    public static DeleteRollupJobRequest fromXContent(XContentParser parser) {
-        return PARSER.apply(parser, null);
-    }
-
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        builder.field(ID_FIELD.getPreferredName(), this.id);
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupCapsRequest.java

@@ -21,14 +21,11 @@
 import org.elasticsearch.client.Validatable;
 import org.elasticsearch.cluster.metadata.Metadata;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 
-import java.io.IOException;
 import java.util.Objects;
 
-public class GetRollupCapsRequest implements Validatable, ToXContentObject {
-    private static final String ID = "id";
+public class GetRollupCapsRequest implements Validatable {
+
     private final String indexPattern;
 
     public GetRollupCapsRequest(final String indexPattern) {
@@ -43,14 +40,6 @@ public String getIndexPattern() {
         return indexPattern;
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        builder.field(ID, indexPattern);
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public int hashCode() {
         return Objects.hash(indexPattern);

client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupIndexCapsRequest.java

@@ -21,16 +21,11 @@
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.client.Validatable;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 
-import java.io.IOException;
 import java.util.Arrays;
 import java.util.Objects;
 
-public class GetRollupIndexCapsRequest implements Validatable, ToXContentObject {
-    private static final String INDICES = "indices";
-    private static final String INDICES_OPTIONS = "indices_options";
+public class GetRollupIndexCapsRequest implements Validatable {
 
     private String[] indices;
     private IndicesOptions options;
@@ -60,21 +55,6 @@ public String[] indices() {
         return indices;
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        {
-            builder.array(INDICES, indices);
-            builder.startObject(INDICES_OPTIONS);
-            {
-                options.toXContent(builder, params);
-            }
-            builder.endObject();
-        }
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public int hashCode() {
         return Objects.hash(Arrays.hashCode(indices), options);

client/rest-high-level/src/test/java/org/elasticsearch/client/rollup/DeleteRollupJobRequestTests.java

@@ -18,39 +18,12 @@
  */
 package org.elasticsearch.client.rollup;
 
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.test.AbstractXContentTestCase;
-import org.junit.Before;
+import org.elasticsearch.test.ESTestCase;
 
-import java.io.IOException;
-
-public class DeleteRollupJobRequestTests extends AbstractXContentTestCase<DeleteRollupJobRequest> {
-
-    private String jobId;
-
-    @Before
-    public void setUpOptionalId() {
-        jobId = randomAlphaOfLengthBetween(1, 10);
-    }
-
-    @Override
-    protected DeleteRollupJobRequest createTestInstance() {
-        return new DeleteRollupJobRequest(jobId);
-    }
-
-    @Override
-    protected DeleteRollupJobRequest doParseInstance(final XContentParser parser) throws IOException {
-        return DeleteRollupJobRequest.fromXContent(parser);
-    }
-
-    @Override
-    protected boolean supportsUnknownFields() {
-        return false;
-    }
+public class DeleteRollupJobRequestTests extends ESTestCase {
 
     public void testRequireConfiguration() {
         final NullPointerException e = expectThrows(NullPointerException.class, ()-> new DeleteRollupJobRequest(null));
         assertEquals("id parameter must not be null", e.getMessage());
     }
-
 }

server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -222,7 +222,7 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th
     }
 
     private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception {
-        final int contentLength = request.content().length();
+        final int contentLength = request.contentLength();
         if (contentLength > 0) {
             final XContentType xContentType = request.getXContentType();
             if (xContentType == null) {

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -209,15 +209,15 @@ public final String path() {
     }
 
     public boolean hasContent() {
-        return content(false).length() > 0;
+        return contentLength() > 0;
     }
 
-    public BytesReference content() {
-        return content(true);
+    public int contentLength() {
+        return httpRequest.content().length();
     }
 
-    protected BytesReference content(final boolean contentConsumed) {
-        this.contentConsumed = this.contentConsumed | contentConsumed;
+    public BytesReference content() {
+        this.contentConsumed = true;
         return httpRequest.content();
     }
 

server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java

@@ -19,23 +19,29 @@
 
 package org.elasticsearch.action.admin.indices.forcemerge;
 
-import org.elasticsearch.client.node.NodeClient;
+import org.apache.lucene.util.SetOnce;
 import org.elasticsearch.common.bytes.BytesArray;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
+import org.elasticsearch.rest.AbstractRestChannel;
+import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.RestResponse;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction;
-import org.elasticsearch.test.rest.FakeRestChannel;
 import org.elasticsearch.test.rest.FakeRestRequest;
 import org.elasticsearch.test.rest.RestActionTestCase;
 import org.junit.Before;
 
 import java.util.HashMap;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.equalTo;
-import static org.mockito.Mockito.mock;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 
 public class RestForceMergeActionTests extends RestActionTestCase {
 
@@ -44,16 +50,27 @@ public void setUpAction() {
         controller().registerHandler(new RestForceMergeAction());
     }
 
+
     public void testBodyRejection() throws Exception {
-        final RestForceMergeAction handler = new RestForceMergeAction();
         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)
+                .withMethod(RestRequest.Method.POST)
                 .withPath("/_forcemerge")
                 .build();
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-            () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class)));
-        assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body"));
+
+        final SetOnce<RestResponse> responseSetOnce = new SetOnce<>();
+        dispatchRequest(request, new AbstractRestChannel(request, true) {
+            @Override
+            public void sendResponse(RestResponse response) {
+                responseSetOnce.set(response);
+            }
+        });
+
+        final RestResponse response = responseSetOnce.get();
+        assertThat(response, notNullValue());
+        assertThat(response.status(), is(RestStatus.BAD_REQUEST));
+        assertThat(response.content().utf8ToString(), containsString("request [POST /_forcemerge] does not support having a body"));
     }
 
     public void testDeprecationMessage() {
@@ -75,4 +92,9 @@ public void testDeprecationMessage() {
         assertWarnings("setting only_expunge_deletes and max_num_segments at the same time is deprecated " +
             "and will be rejected in a future version");
     }
+
+    protected void dispatchRequest(final RestRequest request, final RestChannel channel) {
+        ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
+        controller().dispatchRequest(request, channel, threadContext);
+    }
 }

server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

@@ -31,6 +31,7 @@
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.yaml.YamlXContent;
 import org.elasticsearch.core.internal.io.IOUtils;
@@ -482,6 +483,38 @@ public void testDispatchBadRequest() {
         assertThat(channel.getRestResponse().content().utf8ToString(), containsString("bad request"));
     }
 
+    public void testDoesNotConsumeContent() throws Exception {
+        final RestRequest.Method method = randomFrom(RestRequest.Method.values());
+        restController.registerHandler(method, "/notconsumed", new RestHandler() {
+            @Override
+            public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
+                channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+            }
+
+            @Override
+            public boolean canTripCircuitBreaker() {
+                return false;
+            }
+        });
+
+        final XContentBuilder content = XContentBuilder.builder(randomFrom(XContentType.values()).xContent())
+            .startObject().field("field", "value").endObject();
+        final FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry())
+            .withPath("/notconsumed")
+            .withMethod(method)
+            .withContent(BytesReference.bytes(content), content.contentType())
+            .build();
+
+        final AssertingChannel channel = new AssertingChannel(restRequest, true, RestStatus.OK);
+        assertFalse(channel.getSendResponseCalled());
+        assertFalse(restRequest.isContentConsumed());
+
+        restController.dispatchRequest(restRequest, channel, new ThreadContext(Settings.EMPTY));
+
+        assertTrue(channel.getSendResponseCalled());
+        assertFalse("RestController must not consume request content", restRequest.isContentConsumed());
+    }
+
     public void testDispatchBadRequestUnknownCause() {
         final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
         final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST);

server/src/test/java/org/elasticsearch/rest/RestRequestTests.java

@@ -83,6 +83,10 @@ public void testHasContentDoesNotConsumesContent() {
         runConsumesContentTest(RestRequest::hasContent, false);
     }
 
+    public void testContentLengthDoesNotConsumesContent() {
+        runConsumesContentTest(RestRequest::contentLength, false);
+    }
+
     private <T extends Exception> void runConsumesContentTest(
             final CheckedConsumer<RestRequest, T> consumer, final boolean expected) {
         final HttpRequest httpRequest = mock(HttpRequest.class);