SearchScrollRequest to implement ToXContentObject (#24906)

SearchScrollRequest 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 RestSearchScrollAction to SearchScrollRequest 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/SearchScrollRequest.java

@@ -24,6 +24,9 @@ import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.common.xcontent.ToXContentObject;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.search.Scroll;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.tasks.TaskId;
@@ -33,7 +36,7 @@ import java.util.Objects;
 
 import static org.elasticsearch.action.ValidateActions.addValidationError;
 
-public class SearchScrollRequest extends ActionRequest {
+public class SearchScrollRequest extends ActionRequest implements ToXContentObject {
 
     private String scrollId;
     private Scroll scroll;
@@ -145,4 +148,39 @@ public class SearchScrollRequest extends ActionRequest {
         return "scrollId[" + scrollId + "], scroll[" + scroll + "]";
     }
 
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+        builder.field("scroll_id", scrollId);
+        if (scroll != null) {
+            builder.field("scroll", scroll.keepAlive().getStringRep());
+        }
+        builder.endObject();
+        return builder;
+    }
+
+    /**
+     * Parse a search scroll request from a request body provided through the REST layer.
+     * Values that are already be set and are also found while parsing will be overridden.
+     */
+    public void fromXContent(XContentParser parser) 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) && token == XContentParser.Token.VALUE_STRING) {
+                    scrollId(parser.text());
+                } else if ("scroll".equals(currentFieldName) && token == XContentParser.Token.VALUE_STRING) {
+                    scroll(new Scroll(TimeValue.parseTimeValue(parser.text(), null, "scroll")));
+                } 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/RestSearchScrollAction.java

@@ -22,8 +22,6 @@ package org.elasticsearch.rest.action.search;
 import org.elasticsearch.action.search.SearchScrollRequest;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.rest.BaseRestHandler;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
@@ -58,34 +56,13 @@ public class RestSearchScrollAction extends BaseRestHandler {
 
         request.withContentOrSourceParamParserOrNull(xContentParser -> {
             if (xContentParser != null) {
-                // NOTE: if rest request with xcontent body has request parameters, these parameters override xcontent values
+                // NOTE: if rest request with xcontent body has request parameters, values parsed from request body have the precedence
                 try {
-                    buildFromContent(xContentParser, searchScrollRequest);
+                    searchScrollRequest.fromXContent(xContentParser);
                 } catch (IOException e) {
                     throw new IllegalArgumentException("Failed to parse request body", e);
                 }
             }});
         return channel -> client.searchScroll(searchScrollRequest, new RestStatusToXContentListener<>(channel));
     }
-
-    public static void buildFromContent(XContentParser parser, SearchScrollRequest searchScrollRequest) 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) && token == XContentParser.Token.VALUE_STRING) {
-                    searchScrollRequest.scrollId(parser.text());
-                } else if ("scroll".equals(currentFieldName) && token == XContentParser.Token.VALUE_STRING) {
-                    searchScrollRequest.scroll(new Scroll(TimeValue.parseTimeValue(parser.text(), null, "scroll")));
-                } 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/SearchScrollRequestTests.java

@@ -19,15 +19,25 @@
 
 package org.elasticsearch.action.search;
 
+import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.unit.TimeValue;
+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.search.internal.InternalScrollSearchRequest;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
 
 import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
+import static org.hamcrest.Matchers.startsWith;
 
 public class SearchScrollRequestTests extends ESTestCase {
 
@@ -60,6 +70,60 @@ public class SearchScrollRequestTests extends ESTestCase {
         }
     }
 
+    public void testFromXContent() throws Exception {
+        SearchScrollRequest searchScrollRequest = new SearchScrollRequest();
+        if (randomBoolean()) {
+            //test that existing values get overridden
+            searchScrollRequest = createSearchScrollRequest();
+        }
+        try (XContentParser parser = createParser(XContentFactory.jsonBuilder()
+                .startObject()
+                .field("scroll_id", "SCROLL_ID")
+                .field("scroll", "1m")
+                .endObject())) {
+            searchScrollRequest.fromXContent(parser);
+        }
+        assertEquals("SCROLL_ID", searchScrollRequest.scrollId());
+        assertEquals(TimeValue.parseTimeValue("1m", null, "scroll"), searchScrollRequest.scroll().keepAlive());
+    }
+
+    public void testFromXContentWithUnknownParamThrowsException() throws Exception {
+        SearchScrollRequest searchScrollRequest = new SearchScrollRequest();
+        XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
+                .startObject()
+                .field("scroll_id", "value_2")
+                .field("unknown", "keyword")
+                .endObject());
+
+        Exception e = expectThrows(IllegalArgumentException.class,
+                () -> searchScrollRequest.fromXContent(invalidContent));
+        assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
+    }
+
+    public void testToXContent() throws IOException {
+        SearchScrollRequest searchScrollRequest = new SearchScrollRequest();
+        searchScrollRequest.scrollId("SCROLL_ID");
+        searchScrollRequest.scroll("1m");
+        try (XContentBuilder builder = JsonXContent.contentBuilder()) {
+            searchScrollRequest.toXContent(builder, ToXContent.EMPTY_PARAMS);
+            assertEquals("{\"scroll_id\":\"SCROLL_ID\",\"scroll\":\"1m\"}", builder.string());
+        }
+    }
+
+    public void testToAndFromXContent() throws IOException {
+        XContentType xContentType = randomFrom(XContentType.values());
+        boolean humanReadable = randomBoolean();
+        SearchScrollRequest originalRequest = createSearchScrollRequest();
+        BytesReference originalBytes = toShuffledXContent(originalRequest, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
+        SearchScrollRequest parsedRequest = new SearchScrollRequest();
+        try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
+            parsedRequest.fromXContent(parser);
+        }
+        assertEquals(originalRequest, parsedRequest);
+        BytesReference parsedBytes = XContentHelper.toXContent(parsedRequest, xContentType, humanReadable);
+        assertToXContentEquivalent(originalBytes, parsedBytes, xContentType);
+    }
+
     public void testEqualsAndHashcode() {
         checkEqualsAndHashCode(createSearchScrollRequest(), SearchScrollRequestTests::copyRequest, SearchScrollRequestTests::mutate);
     }

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

@@ -20,36 +20,29 @@
 package org.elasticsearch.search.scroll;
 
 import org.elasticsearch.action.search.SearchScrollRequest;
+import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
-import org.elasticsearch.common.unit.TimeValue;
-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.RestSearchScrollAction;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.rest.FakeRestChannel;
 import org.elasticsearch.test.rest.FakeRestRequest;
+import org.mockito.ArgumentCaptor;
+
+import java.util.HashMap;
+import java.util.Map;
 
 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 RestSearchScrollActionTests extends ESTestCase {
-    public void testParseSearchScrollRequest() throws Exception {
-        XContentParser content = createParser(XContentFactory.jsonBuilder()
-            .startObject()
-                .field("scroll_id", "SCROLL_ID")
-                .field("scroll", "1m")
-            .endObject());
-
-        SearchScrollRequest searchScrollRequest = new SearchScrollRequest();
-        RestSearchScrollAction.buildFromContent(content, searchScrollRequest);
-
-        assertThat(searchScrollRequest.scrollId(), equalTo("SCROLL_ID"));
-        assertThat(searchScrollRequest.scroll().keepAlive(), equalTo(TimeValue.parseTimeValue("1m", null, "scroll")));
-    }
 
     public void testParseSearchScrollRequestWithInvalidJsonThrowsException() throws Exception {
         RestSearchScrollAction action = new RestSearchScrollAction(Settings.EMPTY, mock(RestController.class));
@@ -59,16 +52,24 @@ public class RestSearchScrollActionTests extends ESTestCase {
         assertThat(e.getMessage(), equalTo("Failed to parse request body"));
     }
 
-    public void testParseSearchScrollRequestWithUnknownParamThrowsException() throws Exception {
+    public void testBodyParamsOverrideQueryStringParams() throws Exception {
-        SearchScrollRequest searchScrollRequest = new SearchScrollRequest();
+        NodeClient nodeClient = mock(NodeClient.class);
-        XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
+        doNothing().when(nodeClient).searchScroll(any(), any());
-                .startObject()
-                    .field("scroll_id", "value_2")
-                    .field("unknown", "keyword")
-                .endObject());
 
-        Exception e = expectThrows(IllegalArgumentException.class,
+        RestSearchScrollAction action = new RestSearchScrollAction(Settings.EMPTY, mock(RestController.class));
-                () -> RestSearchScrollAction.buildFromContent(invalidContent, searchScrollRequest));
+        Map<String, String> params = new HashMap<>();
-        assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
+        params.put("scroll_id", "QUERY_STRING");
+        params.put("scroll", "1000m");
+        RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
+                .withParams(params)
+                .withContent(new BytesArray("{\"scroll_id\":\"BODY\", \"scroll\":\"1m\"}"), XContentType.JSON).build();
+        FakeRestChannel channel = new FakeRestChannel(request, false, 0);
+        action.handleRequest(request, channel, nodeClient);
+
+        ArgumentCaptor<SearchScrollRequest> argument = ArgumentCaptor.forClass(SearchScrollRequest.class);
+        verify(nodeClient).searchScroll(argument.capture(), anyObject());
+        SearchScrollRequest searchScrollRequest = argument.getValue();
+        assertEquals("BODY", searchScrollRequest.scrollId());
+        assertEquals("1m", searchScrollRequest.scroll().keepAlive().getStringRep());
     }
 }