Skip to content

Commit 52f75ea

Browse files
javannajavanna
committed
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)
1 parent fb6a353 commit 52f75ea

File tree

4 files changed

+184
-66
lines changed

4 files changed

+184
-66
lines changed

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

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
import org.elasticsearch.action.ActionRequestValidationException;
2424
import org.elasticsearch.common.io.stream.StreamInput;
2525
import org.elasticsearch.common.io.stream.StreamOutput;
26+
import org.elasticsearch.common.xcontent.ToXContentObject;
27+
import org.elasticsearch.common.xcontent.XContentBuilder;
28+
import org.elasticsearch.common.xcontent.XContentParser;
2629

2730
import java.io.IOException;
2831
import java.util.ArrayList;
@@ -31,9 +34,7 @@
3134

3235
import static org.elasticsearch.action.ValidateActions.addValidationError;
3336

34-
/**
35-
*/
36-
public class ClearScrollRequest extends ActionRequest {
37+
public class ClearScrollRequest extends ActionRequest implements ToXContentObject {
3738

3839
private List<String> scrollIds;
3940

@@ -85,4 +86,47 @@ public void writeTo(StreamOutput out) throws IOException {
8586
}
8687
}
8788

89+
@Override
90+
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
91+
builder.startObject();
92+
builder.startArray("scroll_id");
93+
for (String scrollId : scrollIds) {
94+
builder.value(scrollId);
95+
}
96+
builder.endArray();
97+
builder.endObject();
98+
return builder;
99+
}
100+
101+
public void fromXContent(XContentParser parser) throws IOException {
102+
scrollIds = null;
103+
if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
104+
throw new IllegalArgumentException("Malformed content, must start with an object");
105+
} else {
106+
XContentParser.Token token;
107+
String currentFieldName = null;
108+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
109+
if (token == XContentParser.Token.FIELD_NAME) {
110+
currentFieldName = parser.currentName();
111+
} else if ("scroll_id".equals(currentFieldName)){
112+
if (token == XContentParser.Token.START_ARRAY) {
113+
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
114+
if (token.isValue() == false) {
115+
throw new IllegalArgumentException("scroll_id array element should only contain scroll_id");
116+
}
117+
addScrollId(parser.text());
118+
}
119+
} else {
120+
if (token.isValue() == false) {
121+
throw new IllegalArgumentException("scroll_id element should only contain scroll_id");
122+
}
123+
addScrollId(parser.text());
124+
}
125+
} else {
126+
throw new IllegalArgumentException("Unknown parameter [" + currentFieldName
127+
+ "] in request body or parameter is of the wrong type[" + token + "] ");
128+
}
129+
}
130+
}
131+
}
88132
}

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

Lines changed: 2 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.elasticsearch.common.Strings;
2525
import org.elasticsearch.common.bytes.BytesReference;
2626
import org.elasticsearch.common.settings.Settings;
27-
import org.elasticsearch.common.xcontent.XContentParser;
2827
import org.elasticsearch.rest.BaseRestHandler;
2928
import org.elasticsearch.rest.RestController;
3029
import org.elasticsearch.rest.RestRequest;
@@ -57,10 +56,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
5756
clearRequest.setScrollIds(Arrays.asList(splitScrollIds(bodyScrollIds)));
5857
}
5958
} else {
60-
// NOTE: if rest request with xcontent body has request parameters, these parameters does not override xcontent value
61-
clearRequest.setScrollIds(null);
59+
// NOTE: if rest request with xcontent body has request parameters, values parsed from request body have the precedence
6260
try {
63-
buildFromContent(xContentParser, clearRequest);
61+
clearRequest.fromXContent(xContentParser);
6462
} catch (IOException e) {
6563
throw new IllegalArgumentException("Failed to parse request body", e);
6664
}
@@ -81,36 +79,4 @@ private static String[] splitScrollIds(String scrollIds) {
8179
}
8280
return Strings.splitStringByCommaToArray(scrollIds);
8381
}
84-
85-
public static void buildFromContent(XContentParser parser, ClearScrollRequest clearScrollRequest) throws IOException {
86-
if (parser.nextToken() != XContentParser.Token.START_OBJECT) {
87-
throw new IllegalArgumentException("Malformed content, must start with an object");
88-
} else {
89-
XContentParser.Token token;
90-
String currentFieldName = null;
91-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
92-
if (token == XContentParser.Token.FIELD_NAME) {
93-
currentFieldName = parser.currentName();
94-
} else if ("scroll_id".equals(currentFieldName)){
95-
if (token == XContentParser.Token.START_ARRAY) {
96-
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
97-
if (token.isValue() == false) {
98-
throw new IllegalArgumentException("scroll_id array element should only contain scroll_id");
99-
}
100-
clearScrollRequest.addScrollId(parser.text());
101-
}
102-
} else {
103-
if (token.isValue() == false) {
104-
throw new IllegalArgumentException("scroll_id element should only contain scroll_id");
105-
}
106-
clearScrollRequest.addScrollId(parser.text());
107-
}
108-
} else {
109-
throw new IllegalArgumentException("Unknown parameter [" + currentFieldName
110-
+ "] in request body or parameter is of the wrong type[" + token + "] ");
111-
}
112-
}
113-
}
114-
}
115-
11682
}

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

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.action.search;
21+
22+
import org.elasticsearch.common.bytes.BytesReference;
23+
import org.elasticsearch.common.xcontent.ToXContent;
24+
import org.elasticsearch.common.xcontent.XContentBuilder;
25+
import org.elasticsearch.common.xcontent.XContentFactory;
26+
import org.elasticsearch.common.xcontent.XContentHelper;
27+
import org.elasticsearch.common.xcontent.XContentParser;
28+
import org.elasticsearch.common.xcontent.XContentType;
29+
import org.elasticsearch.common.xcontent.json.JsonXContent;
30+
import org.elasticsearch.test.ESTestCase;
31+
32+
import java.io.IOException;
33+
34+
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
35+
import static org.hamcrest.Matchers.contains;
36+
import static org.hamcrest.Matchers.startsWith;
37+
38+
public class ClearScrollRequestTests extends ESTestCase {
39+
40+
public void testFromXContent() throws Exception {
41+
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
42+
if (randomBoolean()) {
43+
//test that existing values get overridden
44+
clearScrollRequest = createClearScrollRequest();
45+
}
46+
try (XContentParser parser = createParser(XContentFactory.jsonBuilder()
47+
.startObject()
48+
.array("scroll_id", "value_1", "value_2")
49+
.endObject())) {
50+
clearScrollRequest.fromXContent(parser);
51+
}
52+
assertThat(clearScrollRequest.scrollIds(), contains("value_1", "value_2"));
53+
}
54+
55+
public void testFromXContentWithoutArray() throws Exception {
56+
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
57+
if (randomBoolean()) {
58+
//test that existing values get overridden
59+
clearScrollRequest = createClearScrollRequest();
60+
}
61+
try (XContentParser parser = createParser(XContentFactory.jsonBuilder()
62+
.startObject()
63+
.field("scroll_id", "value_1")
64+
.endObject())) {
65+
clearScrollRequest.fromXContent(parser);
66+
}
67+
assertThat(clearScrollRequest.scrollIds(), contains("value_1"));
68+
}
69+
70+
public void testFromXContentWithUnknownParamThrowsException() throws Exception {
71+
XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
72+
.startObject()
73+
.array("scroll_id", "value_1", "value_2")
74+
.field("unknown", "keyword")
75+
.endObject());
76+
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
77+
78+
Exception e = expectThrows(IllegalArgumentException.class, () -> clearScrollRequest.fromXContent(invalidContent));
79+
assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
80+
}
81+
82+
public void testToXContent() throws IOException {
83+
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
84+
clearScrollRequest.addScrollId("SCROLL_ID");
85+
try (XContentBuilder builder = JsonXContent.contentBuilder()) {
86+
clearScrollRequest.toXContent(builder, ToXContent.EMPTY_PARAMS);
87+
assertEquals("{\"scroll_id\":[\"SCROLL_ID\"]}", builder.string());
88+
}
89+
}
90+
91+
public void testFromAndToXContent() throws IOException {
92+
XContentType xContentType = randomFrom(XContentType.values());
93+
ClearScrollRequest originalRequest = createClearScrollRequest();
94+
BytesReference originalBytes = toShuffledXContent(originalRequest, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
95+
ClearScrollRequest parsedRequest = new ClearScrollRequest();
96+
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
97+
parsedRequest.fromXContent(parser);
98+
}
99+
assertEquals(originalRequest.scrollIds(), parsedRequest.scrollIds());
100+
BytesReference parsedBytes = XContentHelper.toXContent(parsedRequest, xContentType, randomBoolean());
101+
assertToXContentEquivalent(originalBytes, parsedBytes, xContentType);
102+
}
103+
104+
public static ClearScrollRequest createClearScrollRequest() {
105+
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
106+
int numScrolls = randomIntBetween(1, 10);
107+
for (int i = 0; i < numScrolls; i++) {
108+
clearScrollRequest.addScrollId(randomAlphaOfLengthBetween(3, 10));
109+
}
110+
return clearScrollRequest;
111+
}
112+
}

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

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,13 @@
2525
import org.elasticsearch.common.bytes.BytesArray;
2626
import org.elasticsearch.common.settings.Settings;
2727
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
28-
import org.elasticsearch.common.xcontent.XContentFactory;
29-
import org.elasticsearch.common.xcontent.XContentParser;
3028
import org.elasticsearch.common.xcontent.XContentType;
3129
import org.elasticsearch.rest.RestChannel;
3230
import org.elasticsearch.rest.RestController;
3331
import org.elasticsearch.rest.RestRequest;
3432
import org.elasticsearch.rest.action.search.RestClearScrollAction;
3533
import org.elasticsearch.test.ESTestCase;
34+
import org.elasticsearch.test.rest.FakeRestChannel;
3635
import org.elasticsearch.test.rest.FakeRestRequest;
3736
import org.mockito.ArgumentCaptor;
3837

@@ -41,23 +40,14 @@
4140
import java.util.List;
4241
import java.util.stream.Collectors;
4342

44-
import static org.elasticsearch.mock.orig.Mockito.verify;
45-
import static org.hamcrest.Matchers.contains;
4643
import static org.hamcrest.Matchers.equalTo;
47-
import static org.hamcrest.Matchers.startsWith;
4844
import static org.mockito.Matchers.any;
45+
import static org.mockito.Matchers.anyObject;
46+
import static org.mockito.Mockito.doNothing;
4947
import static org.mockito.Mockito.mock;
48+
import static org.mockito.Mockito.verify;
5049

5150
public class RestClearScrollActionTests extends ESTestCase {
52-
public void testParseClearScrollRequest() throws Exception {
53-
XContentParser content = createParser(XContentFactory.jsonBuilder()
54-
.startObject()
55-
.array("scroll_id", "value_1", "value_2")
56-
.endObject());
57-
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
58-
RestClearScrollAction.buildFromContent(content, clearScrollRequest);
59-
assertThat(clearScrollRequest.scrollIds(), contains("value_1", "value_2"));
60-
}
6151

6252
public void testParseClearScrollRequestWithInvalidJsonThrowsException() throws Exception {
6353
RestClearScrollAction action = new RestClearScrollAction(Settings.EMPTY, mock(RestController.class));
@@ -67,17 +57,23 @@ public void testParseClearScrollRequestWithInvalidJsonThrowsException() throws E
6757
assertThat(e.getMessage(), equalTo("Failed to parse request body"));
6858
}
6959

70-
public void testParseClearScrollRequestWithUnknownParamThrowsException() throws Exception {
71-
XContentParser invalidContent = createParser(XContentFactory.jsonBuilder()
72-
.startObject()
73-
.array("scroll_id", "value_1", "value_2")
74-
.field("unknown", "keyword")
75-
.endObject());
76-
ClearScrollRequest clearScrollRequest = new ClearScrollRequest();
60+
public void testBodyParamsOverrideQueryStringParams() throws Exception {
61+
NodeClient nodeClient = mock(NodeClient.class);
62+
doNothing().when(nodeClient).searchScroll(any(), any());
63+
64+
RestClearScrollAction action = new RestClearScrollAction(Settings.EMPTY, mock(RestController.class));
65+
RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
66+
.withParams(Collections.singletonMap("scroll_id", "QUERY_STRING"))
67+
.withContent(new BytesArray("{\"scroll_id\": [\"BODY\"]}"), XContentType.JSON).build();
68+
FakeRestChannel channel = new FakeRestChannel(request, false, 0);
69+
action.handleRequest(request, channel, nodeClient);
7770

78-
Exception e = expectThrows(IllegalArgumentException.class,
79-
() -> RestClearScrollAction.buildFromContent(invalidContent, clearScrollRequest));
80-
assertThat(e.getMessage(), startsWith("Unknown parameter [unknown]"));
71+
ArgumentCaptor<ClearScrollRequest> argument = ArgumentCaptor.forClass(ClearScrollRequest.class);
72+
verify(nodeClient).clearScroll(argument.capture(), anyObject());
73+
ClearScrollRequest clearScrollRequest = argument.getValue();
74+
List<String> scrollIds = clearScrollRequest.getScrollIds();
75+
assertEquals(1, scrollIds.size());
76+
assertEquals("BODY", scrollIds.get(0));
8177
}
8278

8379
public void testParseClearScrollPlaintext() throws Exception {
@@ -86,9 +82,9 @@ public void testParseClearScrollPlaintext() throws Exception {
8682
final List<String> scrollIds = Arrays.asList(generateRandomStringArray(4, 30, false, false));
8783
final String content = scrollIds.stream().collect(Collectors.joining(","));
8884
FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
89-
.withContent(new BytesArray(content), null)
90-
.withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain")))
91-
.build();
85+
.withContent(new BytesArray(content), null)
86+
.withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain")))
87+
.build();
9288
action.handleRequest(fakeRestRequest, mock(RestChannel.class), mockNodeClient);
9389
ArgumentCaptor<ClearScrollRequest> captor = ArgumentCaptor.forClass(ClearScrollRequest.class);
9490
verify(mockNodeClient).clearScroll(captor.capture(), any(ActionListener.class));

0 commit comments

Comments
 (0)