From ea00d343aeb33b5b3437c577456b0567b172e283 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Mon, 29 May 2017 11:00:20 +0200 Subject: [PATCH] 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) --- .../action/search/ClearScrollRequest.java | 48 +++++++- .../action/search/RestClearScrollAction.java | 38 +----- .../search/ClearScrollRequestTests.java | 112 ++++++++++++++++++ .../scroll/RestClearScrollActionTests.java | 50 ++++---- 4 files changed, 187 insertions(+), 61 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java diff --git a/core/src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java b/core/src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java index 23c5c3747fb..4770818867c 100644 --- a/core/src/main/java/org/elasticsearch/action/search/ClearScrollRequest.java +++ b/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 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 + "] "); + } + } + } + } } diff --git a/core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java b/core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java index c7281da23f1..80d833ed311 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/search/RestClearScrollAction.java +++ b/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 - clearRequest.setScrollIds(null); + // NOTE: if rest request with xcontent body has request parameters, values parsed from request body have the precedence 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 + "] "); - } - } - } - } - } diff --git a/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java b/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java new file mode 100644 index 00000000000..6414e510069 --- /dev/null +++ b/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; + } +} diff --git a/core/src/test/java/org/elasticsearch/search/scroll/RestClearScrollActionTests.java b/core/src/test/java/org/elasticsearch/search/scroll/RestClearScrollActionTests.java index ae8ad66ac8d..905bddbcf16 100644 --- a/core/src/test/java/org/elasticsearch/search/scroll/RestClearScrollActionTests.java +++ b/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 { - XContentParser invalidContent = createParser(XContentFactory.jsonBuilder() - .startObject() - .array("scroll_id", "value_1", "value_2") - .field("unknown", "keyword") - .endObject()); - ClearScrollRequest clearScrollRequest = new ClearScrollRequest(); + public void testBodyParamsOverrideQueryStringParams() throws Exception { + NodeClient nodeClient = mock(NodeClient.class); + doNothing().when(nodeClient).searchScroll(any(), any()); - Exception e = expectThrows(IllegalArgumentException.class, - () -> RestClearScrollAction.buildFromContent(invalidContent, clearScrollRequest)); - assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]")); + RestClearScrollAction action = new RestClearScrollAction(Settings.EMPTY, mock(RestController.class)); + RestRequest request = new FakeRestRequest.Builder(xContentRegistry()) + .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 argument = ArgumentCaptor.forClass(ClearScrollRequest.class); + verify(nodeClient).clearScroll(argument.capture(), anyObject()); + ClearScrollRequest clearScrollRequest = argument.getValue(); + List scrollIds = clearScrollRequest.getScrollIds(); + assertEquals(1, scrollIds.size()); + assertEquals("BODY", scrollIds.get(0)); } - }