From c66aa8fb116cc6af6262d09eeff7ae29d74d1c00 Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 26 May 2017 17:43:15 +0200 Subject: [PATCH 1/2] ClearScrollRequest to implement ToXContentObject 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 | 113 ++++++++++++++++++ .../scroll/RestClearScrollActionTests.java | 50 ++++---- 4 files changed, 188 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 23c5c3747fbf4..4770818867c84 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.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 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 void writeTo(StreamOutput out) throws IOException { } } + @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 c7281da23f137..80d833ed3118a 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.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 RestChannelConsumer prepareRequest(final RestRequest request, final NodeC 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 @@ private static String[] splitScrollIds(String scrollIds) { } 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 0000000000000..a9829db5a50f7 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java @@ -0,0 +1,113 @@ +/* + * 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); + System.out.println(builder.string()); + 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 ae8ad66ac8d94..905bddbcf161a 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 void testParseClearScrollRequestWithInvalidJsonThrowsException() throws E 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)); + } } From ae440242512b0b4c15e4e49ecdc9e6204450fca4 Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 26 May 2017 18:14:13 +0200 Subject: [PATCH 2/2] remove leftover system.out --- .../org/elasticsearch/action/search/ClearScrollRequestTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java b/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java index a9829db5a50f7..6414e510069a0 100644 --- a/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java +++ b/core/src/test/java/org/elasticsearch/action/search/ClearScrollRequestTests.java @@ -84,7 +84,6 @@ public void testToXContent() throws IOException { clearScrollRequest.addScrollId("SCROLL_ID"); try (XContentBuilder builder = JsonXContent.contentBuilder()) { clearScrollRequest.toXContent(builder, ToXContent.EMPTY_PARAMS); - System.out.println(builder.string()); assertEquals("{\"scroll_id\":[\"SCROLL_ID\"]}", builder.string()); } }