From 1fdce478c0ede531720dfe23fe980e78685afccd Mon Sep 17 00:00:00 2001 From: Yu Date: Mon, 19 Nov 2018 22:55:50 +0100 Subject: [PATCH 1/2] Restore "size" field in the body of AbstractBulkByScrollRequest The "size" field in the body of AbstractBulkByScrollRequest(DeleteByQuery/UpdateByQuery) is not taken into consideration anymore during some previous changes, this is to fix it. --- .../AbstractBulkByQueryRestHandler.java | 5 +- .../reindex/RestDeleteByQueryActionTests.java | 49 ++++++++++++++++++- .../reindex/RestUpdateByQueryActionTests.java | 48 ++++++++++++++++++ .../reindex/AbstractBulkByScrollRequest.java | 1 - 4 files changed, 100 insertions(+), 3 deletions(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java index fab94494fe13d..1b9f798196aa0 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java @@ -55,7 +55,10 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest, RestSearchAction.parseSearchRequest(searchRequest, restRequest, parser, internal::setSize); } - searchRequest.source().size(restRequest.paramAsInt("scroll_size", searchRequest.source().size())); + if (restRequest.hasParam("size") == false && searchRequest.source().size() >= 0) { + internal.setSize(searchRequest.source().size()); + } + searchRequest.source().size(restRequest.paramAsInt("scroll_size", AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE)); String conflicts = restRequest.param("conflicts"); if (conflicts != null) { diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java index 1f972cd282425..6e75dadb85c01 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java @@ -19,23 +19,70 @@ package org.elasticsearch.index.reindex; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestController; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.search.SearchModule; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestRequest; +import org.junit.AfterClass; +import org.junit.BeforeClass; import java.io.IOException; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonMap; import static org.mockito.Mockito.mock; public class RestDeleteByQueryActionTests extends ESTestCase { + private static NamedXContentRegistry xContentRegistry; + private static RestDeleteByQueryAction action; + + @BeforeClass + public static void init() { + xContentRegistry = new NamedXContentRegistry(new SearchModule(Settings.EMPTY, false, emptyList()).getNamedXContents()); + action = new RestDeleteByQueryAction(Settings.EMPTY, mock(RestController.class)); + } + + @AfterClass + public static void cleanup() { + xContentRegistry = null; + action = null; + } + public void testParseEmpty() throws IOException { - RestDeleteByQueryAction action = new RestDeleteByQueryAction(Settings.EMPTY, mock(RestController.class)); DeleteByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())) .build()); assertEquals(AbstractBulkByScrollRequest.SIZE_ALL_MATCHES, request.getSize()); assertEquals(AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE, request.getSearchRequest().source().size()); } + + public void testParseWithSize() throws IOException { + { + FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()) + .withPath("index/type/_delete_by_query") + .withParams(singletonMap("size", "2")) + .build(); + DeleteByQueryRequest request = action.buildRequest(restRequest); + assertEquals(2, request.getSize()); + } + { + final String requestContent = "{\"query\" : {\"match_all\": {}}, \"size\": 2 }"; + FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.POST) + .withPath("index/type/_delete_by_query") + .withContent(new BytesArray(requestContent), XContentType.JSON) + .build(); + DeleteByQueryRequest request = action.buildRequest(restRequest); + assertEquals(2, request.getSize()); + } + } + + @Override + protected NamedXContentRegistry xContentRegistry() { + return xContentRegistry; + } } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java index efb6e20a20089..e9e5b34607366 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java @@ -19,18 +19,40 @@ package org.elasticsearch.index.reindex; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestController; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.search.SearchModule; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestRequest; +import org.junit.AfterClass; +import org.junit.BeforeClass; import java.io.IOException; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonMap; import static org.mockito.Mockito.mock; public class RestUpdateByQueryActionTests extends ESTestCase { + private static NamedXContentRegistry xContentRegistry; + private static RestUpdateByQueryAction action; + + @BeforeClass + public static void init() { + xContentRegistry = new NamedXContentRegistry(new SearchModule(Settings.EMPTY, false, emptyList()).getNamedXContents()); + action = new RestUpdateByQueryAction(Settings.EMPTY, mock(RestController.class)); + } + + @AfterClass + public static void cleanup() { + xContentRegistry = null; + action = null; + } + public void testParseEmpty() throws IOException { RestUpdateByQueryAction action = new RestUpdateByQueryAction(Settings.EMPTY, mock(RestController.class)); UpdateByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())) @@ -38,4 +60,30 @@ public void testParseEmpty() throws IOException { assertEquals(AbstractBulkByScrollRequest.SIZE_ALL_MATCHES, request.getSize()); assertEquals(AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE, request.getSearchRequest().source().size()); } + + public void testParseWithSize() throws IOException { + { + FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()) + .withPath("index/type/_update_by_query") + .withParams(singletonMap("size", "2")) + .build(); + UpdateByQueryRequest request = action.buildRequest(restRequest); + assertEquals(2, request.getSize()); + } + { + final String requestContent = "{\"query\" : {\"match_all\": {}}, \"size\": 2 }"; + FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()) + .withMethod(RestRequest.Method.POST) + .withPath("index/type/_update_by_query") + .withContent(new BytesArray(requestContent), XContentType.JSON) + .build(); + UpdateByQueryRequest request = action.buildRequest(restRequest); + assertEquals(2, request.getSize()); + } + } + + @Override + protected NamedXContentRegistry xContentRegistry() { + return xContentRegistry; + } } diff --git a/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java b/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java index 4aa9bc5ce146c..e7245301328e0 100644 --- a/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java +++ b/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java @@ -130,7 +130,6 @@ public AbstractBulkByScrollRequest(SearchRequest searchRequest, boolean setDefau if (setDefaults) { searchRequest.scroll(DEFAULT_SCROLL_TIMEOUT); searchRequest.source(new SearchSourceBuilder()); - searchRequest.source().size(DEFAULT_SCROLL_SIZE); } } From 3febee5401dc3881b038a8dece90ee9f7aab5c84 Mon Sep 17 00:00:00 2001 From: Yu Date: Sat, 8 Dec 2018 15:46:47 +0800 Subject: [PATCH 2/2] Add a setSize comsumer for search XContent parsing --- .../AbstractBulkByQueryRestHandler.java | 6 ++-- .../reindex/RestDeleteByQueryActionTests.java | 3 +- .../reindex/RestUpdateByQueryActionTests.java | 3 +- .../reindex/AbstractBulkByScrollRequest.java | 1 + .../rest/action/search/RestSearchAction.java | 4 +-- .../search/builder/SearchSourceBuilder.java | 29 ++++++++++++------- 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java index 1b9f798196aa0..742e49ac3b8a9 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java @@ -55,9 +55,9 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest, RestSearchAction.parseSearchRequest(searchRequest, restRequest, parser, internal::setSize); } - if (restRequest.hasParam("size") == false && searchRequest.source().size() >= 0) { - internal.setSize(searchRequest.source().size()); - } +// if (restRequest.hasParam("size") == false && searchRequest.source().size() != AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE) { +// internal.setSize(searchRequest.source().size()); +// } searchRequest.source().size(restRequest.paramAsInt("scroll_size", AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE)); String conflicts = restRequest.param("conflicts"); diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java index 6e75dadb85c01..54f371f3113a2 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestDeleteByQueryActionTests.java @@ -54,8 +54,7 @@ public static void cleanup() { } public void testParseEmpty() throws IOException { - DeleteByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())) - .build()); + DeleteByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())).build()); assertEquals(AbstractBulkByScrollRequest.SIZE_ALL_MATCHES, request.getSize()); assertEquals(AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE, request.getSearchRequest().source().size()); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java index e9e5b34607366..a561cd7372ed3 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RestUpdateByQueryActionTests.java @@ -55,8 +55,7 @@ public static void cleanup() { public void testParseEmpty() throws IOException { RestUpdateByQueryAction action = new RestUpdateByQueryAction(Settings.EMPTY, mock(RestController.class)); - UpdateByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())) - .build()); + UpdateByQueryRequest request = action.buildRequest(new FakeRestRequest.Builder(new NamedXContentRegistry(emptyList())).build()); assertEquals(AbstractBulkByScrollRequest.SIZE_ALL_MATCHES, request.getSize()); assertEquals(AbstractBulkByScrollRequest.DEFAULT_SCROLL_SIZE, request.getSearchRequest().source().size()); } diff --git a/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java b/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java index e7245301328e0..4aa9bc5ce146c 100644 --- a/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java +++ b/server/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java @@ -130,6 +130,7 @@ public AbstractBulkByScrollRequest(SearchRequest searchRequest, boolean setDefau if (setDefaults) { searchRequest.scroll(DEFAULT_SCROLL_TIMEOUT); searchRequest.source(new SearchSourceBuilder()); + searchRequest.source().size(DEFAULT_SCROLL_SIZE); } } diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index 60fd77e46aa3f..caa983e470fef 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -103,7 +103,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC * * @param requestContentParser body of the request to read. This method does not attempt to read the body from the {@code request} * parameter - * @param setSize how the size url parameter is handled. {@code udpate_by_query} and regular search differ here. + * @param setSize how the size parameter is handled. {@code udpate_by_query} and regular search differ here. */ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest request, XContentParser requestContentParser, @@ -114,7 +114,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r } searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index"))); if (requestContentParser != null) { - searchRequest.source().parseXContent(requestContentParser, true); + searchRequest.source().parseXContent(requestContentParser, true, setSize); } final int batchedReduceSize = request.paramAsInt("batched_reduce_size", searchRequest.getBatchedReduceSize()); diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index a199ce3a37776..dde92565db448 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -65,6 +65,7 @@ import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.function.IntConsumer; import java.util.stream.Collectors; import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder; @@ -989,6 +990,10 @@ public void parseXContent(XContentParser parser) throws IOException { parseXContent(parser, true); } + public void parseXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException { + parseXContent(parser, checkTrailingTokens, null); + } + /** * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent. Use this if you need to set up * different defaults than a regular SearchSourceBuilder would have and use {@link #fromXContent(XContentParser, boolean)} if you have @@ -996,13 +1001,14 @@ public void parseXContent(XContentParser parser) throws IOException { * * @param parser The xContent parser. * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found after the main object. + * @param setSize how the size field is handled. {@code udpate_by_query} and regular search differ here. */ - public void parseXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException { + public void parseXContent(XContentParser parser, boolean checkTrailingTokens, IntConsumer setSize) throws IOException { XContentParser.Token token = parser.currentToken(); String currentFieldName = null; if (token != XContentParser.Token.START_OBJECT && (token = parser.nextToken()) != XContentParser.Token.START_OBJECT) { throw new ParsingException(parser.getTokenLocation(), "Expected [" + XContentParser.Token.START_OBJECT + - "] but found [" + token + "]", parser.getTokenLocation()); + "] but found [" + token + "]", parser.getTokenLocation()); } while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -1012,6 +1018,9 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th from = parser.intValue(); } else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { size = parser.intValue(); + if (setSize != null) { + setSize.accept(size); + } } else if (TIMEOUT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { timeout = TimeValue.parseTimeValue(parser.text(), null, TIMEOUT_FIELD.getPreferredName()); } else if (TERMINATE_AFTER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { @@ -1037,7 +1046,7 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th profile = parser.booleanValue(); } else { throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation()); + parser.getTokenLocation()); } } else if (token == XContentParser.Token.START_OBJECT) { if (QUERY_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { @@ -1065,7 +1074,7 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th } } } else if (AGGREGATIONS_FIELD.match(currentFieldName, parser.getDeprecationHandler()) - || AGGS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + || AGGS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { aggregations = AggregatorFactories.parseAggregators(parser); } else if (HIGHLIGHT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { highlightBuilder = HighlightBuilder.fromXContent(parser); @@ -1086,8 +1095,8 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th SearchExtBuilder searchExtBuilder = parser.namedObject(SearchExtBuilder.class, extSectionName, null); if (searchExtBuilder.getWriteableName().equals(extSectionName) == false) { throw new IllegalStateException("The parsed [" + searchExtBuilder.getClass().getName() + "] object has a " - + "different writeable name compared to the name of the section that it was parsed from: found [" - + searchExtBuilder.getWriteableName() + "] expected [" + extSectionName + "]"); + + "different writeable name compared to the name of the section that it was parsed from: found [" + + searchExtBuilder.getWriteableName() + "] expected [" + extSectionName + "]"); } extBuilders.add(searchExtBuilder); } @@ -1098,7 +1107,7 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th collapse = CollapseBuilder.fromXContent(parser); } else { throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation()); + parser.getTokenLocation()); } } else if (token == XContentParser.Token.START_ARRAY) { if (STORED_FIELDS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { @@ -1126,7 +1135,7 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th stats.add(parser.text()); } else { throw new ParsingException(parser.getTokenLocation(), "Expected [" + XContentParser.Token.VALUE_STRING + - "] in [" + currentFieldName + "] but found [" + token + "]", parser.getTokenLocation()); + "] in [" + currentFieldName + "] but found [" + token + "]", parser.getTokenLocation()); } } } else if (_SOURCE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { @@ -1135,11 +1144,11 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th searchAfterBuilder = SearchAfterBuilder.fromXContent(parser); } else { throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation()); + parser.getTokenLocation()); } } else { throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation()); + parser.getTokenLocation()); } } if (checkTrailingTokens) {