ClearScrollRequest to implement ToXContentObject (#24907)

ClearScrollRequest can be created from a request body, but it doesn't support the opposite, meaning printing out its content to an XContentBuilder. This is useful to the high level REST client and allows for better testing of what we parse.

Moved parsing method from RestClearScrollAction to ClearScrollRequest so that fromXContent and toXContent sit close to each other. Added unit tests to verify that body parameters override query_string parameters when both present (there is already a yaml test for this but unit test is even better)

core/src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java

@@ -23,6 +23,9 @@ import org.elasticsearch.action.ActionRequest;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+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.ArrayList;
@@ -31,7 +34,7 @@ import java.util.List;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 
-public class ClearScrollRequest extends ActionRequest {
+public class ClearScrollRequest extends ActionRequest implements ToXContentObject {
 
     private List<String> scrollIds;
 
@@ -83,4 +86,47 @@ public class ClearScrollRequest extends ActionRequest {
         }
     }
 
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+        builder.startArray("scroll_id");
+        for (String scrollId : scrollIds) {
+            builder.value(scrollId);
+        }
+        builder.endArray();
+        builder.endObject();
+        return builder;
+    }
+
+    public void fromXContent(XContentParser parser) throws IOException {
+        scrollIds = null;
+        if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
+            throw new IllegalArgumentException("Malformed content, must start with an object");
+        } else {
+            XContentParser.Token token;
+            String currentFieldName = null;
+            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+                if (token == XContentParser.Token.FIELD_NAME) {
+                    currentFieldName = parser.currentName();
+                } else if ("scroll_id".equals(currentFieldName)){
+                    if (token == XContentParser.Token.START_ARRAY) {
+                        while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
+                            if (token.isValue() == false) {
+                                throw new IllegalArgumentException("scroll_id array element should only contain scroll_id");
+                            }
+                            addScrollId(parser.text());
+                        }
+                    } else {
+                        if (token.isValue() == false) {
+                            throw new IllegalArgumentException("scroll_id element should only contain scroll_id");
+                        }
+                        addScrollId(parser.text());
+                    }
+                } else {
+                    throw new IllegalArgumentException("Unknown parameter [" + currentFieldName
+                            + "] in request body or parameter is of the wrong type[" + token + "] ");
+                }
+            }
+        }
+    }
 }

core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java

@@ -23,7 +23,6 @@ import org.elasticsearch.action.search.ClearScrollRequest;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.rest.BaseRestHandler;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
@@ -49,10 +48,9 @@ public class RestClearScrollAction extends BaseRestHandler {
         clearRequest.setScrollIds(Arrays.asList(splitScrollIds(scrollIds)));
         request.withContentOrSourceParamParserOrNull((xContentParser -> {
             if (xContentParser != null) {
-                // NOTE: if rest request with xcontent body has request parameters, these parameters does not override xcontent value
+                // NOTE: if rest request with xcontent body has request parameters, values parsed from request body have the precedence
-                clearRequest.setScrollIds(null);
                 try {
-                    buildFromContent(xContentParser, clearRequest);
+                    clearRequest.fromXContent(xContentParser);
                 } catch (IOException e) {
                     throw new IllegalArgumentException("Failed to parse request body", e);
                 }
@@ -68,36 +66,4 @@ public class RestClearScrollAction extends BaseRestHandler {
         }
         return Strings.splitStringByCommaToArray(scrollIds);
     }
-
-    public static void buildFromContent(XContentParser parser, ClearScrollRequest clearScrollRequest) throws IOException {
-        if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
-            throw new IllegalArgumentException("Malformed content, must start with an object");
-        } else {
-            XContentParser.Token token;
-            String currentFieldName = null;
-            while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
-                if (token == XContentParser.Token.FIELD_NAME) {
-                    currentFieldName = parser.currentName();
-                } else if ("scroll_id".equals(currentFieldName)){
-                    if (token == XContentParser.Token.START_ARRAY) {
-                        while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
-                            if (token.isValue() == false) {
-                                throw new IllegalArgumentException("scroll_id array element should only contain scroll_id");
-                            }
-                            clearScrollRequest.addScrollId(parser.text());
-                        }
-                    } else {
-                        if (token.isValue() == false) {
-                            throw new IllegalArgumentException("scroll_id element should only contain scroll_id");
-                        }
-                        clearScrollRequest.addScrollId(parser.text());
-                    }
-                } else {
-                    throw new IllegalArgumentException("Unknown parameter [" + currentFieldName
-                            + "] in request body or parameter is of the wrong type[" + token + "] ");
-                }
-            }
-        }
-    }
-
 }

core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java

@@ -0,0 +1,112 @@
+/*
+ * 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.action.search;
+
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
+import org.elasticsearch.test.ESTestCase;
+
+import java.io.IOException;
+
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.startsWith;
+
+public class ClearScrollRequestTests extends ESTestCase {
+
+    public void testFromXContent() throws Exception {
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
+        if (randomBoolean()) {
+            //test that existing values get overridden
+            clearScrollRequest = createClearScrollRequest();
+        }
+        try (XContentParser parser = createParser(XContentFactory.jsonBuilder()
+                .startObject()
+                .array("scroll_id", "value_1", "value_2")
+                .endObject())) {
+            clearScrollRequest.fromXContent(parser);
+        }
+        assertThat(clearScrollRequest.scrollIds(), contains("value_1", "value_2"));
+    }
+
+    public void testFromXContentWithoutArray() throws Exception {
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
+        if (randomBoolean()) {
+            //test that existing values get overridden
+            clearScrollRequest = createClearScrollRequest();
+        }
+        try (XContentParser parser = createParser(XContentFactory.jsonBuilder()
+                .startObject()
+                .field("scroll_id", "value_1")
+                .endObject())) {
+            clearScrollRequest.fromXContent(parser);
+        }
+        assertThat(clearScrollRequest.scrollIds(), contains("value_1"));
+    }
+
+    public void testFromXContentWithUnknownParamThrowsException() throws Exception {
+        XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
+                .startObject()
+                .array("scroll_id", "value_1", "value_2")
+                .field("unknown", "keyword")
+                .endObject());
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
+
+        Exception e = expectThrows(IllegalArgumentException.class, () -> clearScrollRequest.fromXContent(invalidContent));
+        assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
+    }
+
+    public void testToXContent() throws IOException {
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
+        clearScrollRequest.addScrollId("SCROLL_ID");
+        try (XContentBuilder builder = JsonXContent.contentBuilder()) {
+            clearScrollRequest.toXContent(builder, ToXContent.EMPTY_PARAMS);
+            assertEquals("{\"scroll_id\":[\"SCROLL_ID\"]}", builder.string());
+        }
+    }
+
+    public void testFromAndToXContent() throws IOException {
+        XContentType xContentType = randomFrom(XContentType.values());
+        ClearScrollRequest originalRequest = createClearScrollRequest();
+        BytesReference originalBytes = toShuffledXContent(originalRequest, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
+        ClearScrollRequest parsedRequest = new ClearScrollRequest();
+        try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
+            parsedRequest.fromXContent(parser);
+        }
+        assertEquals(originalRequest.scrollIds(), parsedRequest.scrollIds());
+        BytesReference parsedBytes = XContentHelper.toXContent(parsedRequest, xContentType, randomBoolean());
+        assertToXContentEquivalent(originalBytes, parsedBytes, xContentType);
+    }
+
+    public static ClearScrollRequest createClearScrollRequest() {
+        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
+        int numScrolls = randomIntBetween(1, 10);
+        for (int i = 0; i < numScrolls; i++) {
+            clearScrollRequest.addScrollId(randomAlphaOfLengthBetween(3, 10));
+        }
+        return clearScrollRequest;
+    }
+}

core/src/test/java/org/elasticsearch/search/scroll/RestClearScrollActionTests.java

@@ -20,32 +20,29 @@
 package org.elasticsearch.search.scroll;
 
 import org.elasticsearch.action.search.ClearScrollRequest;
+import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.xcontent.XContentFactory;
-import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.action.search.RestClearScrollAction;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.rest.FakeRestChannel;
 import org.elasticsearch.test.rest.FakeRestRequest;
+import org.mockito.ArgumentCaptor;
+
+import java.util.Collections;
+import java.util.List;
 
-import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.startsWith;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyObject;
+import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 public class RestClearScrollActionTests extends ESTestCase {
-    public void testParseClearScrollRequest() throws Exception {
-        XContentParser content = createParser(XContentFactory.jsonBuilder()
-                .startObject()
-                    .array("scroll_id", "value_1", "value_2")
-                .endObject());
-        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
-        RestClearScrollAction.buildFromContent(content, clearScrollRequest);
-        assertThat(clearScrollRequest.scrollIds(), contains("value_1", "value_2"));
-    }
 
     public void testParseClearScrollRequestWithInvalidJsonThrowsException() throws Exception {
         RestClearScrollAction action = new RestClearScrollAction(Settings.EMPTY, mock(RestController.class));
@@ -55,17 +52,22 @@ public class RestClearScrollActionTests extends ESTestCase {
         assertThat(e.getMessage(), equalTo("Failed to parse request body"));
     }
 
-    public void testParseClearScrollRequestWithUnknownParamThrowsException() throws Exception {
+    public void testBodyParamsOverrideQueryStringParams() throws Exception {
-        XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
+        NodeClient nodeClient = mock(NodeClient.class);
-                .startObject()
+        doNothing().when(nodeClient).searchScroll(any(), any());
-                    .array("scroll_id", "value_1", "value_2")
-                    .field("unknown", "keyword")
-                .endObject());
-        ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
 
-        Exception e = expectThrows(IllegalArgumentException.class,
+        RestClearScrollAction action = new RestClearScrollAction(Settings.EMPTY, mock(RestController.class));
-                () -> RestClearScrollAction.buildFromContent(invalidContent, clearScrollRequest));
+        RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
-        assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
+                .withParams(Collections.singletonMap("scroll_id", "QUERY_STRING"))
+                .withContent(new BytesArray("{\"scroll_id\": [\"BODY\"]}"), XContentType.JSON).build();
+        FakeRestChannel channel = new FakeRestChannel(request, false, 0);
+        action.handleRequest(request, channel, nodeClient);
+
+        ArgumentCaptor<ClearScrollRequest> argument = ArgumentCaptor.forClass(ClearScrollRequest.class);
+        verify(nodeClient).clearScroll(argument.capture(), anyObject());
+        ClearScrollRequest clearScrollRequest = argument.getValue();
+        List<String> scrollIds = clearScrollRequest.getScrollIds();
+        assertEquals(1, scrollIds.size());
+        assertEquals("BODY", scrollIds.get(0));
     }
-
 }