diff --git a/core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java b/core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java index f625152d407..2644d0d9496 100644 --- a/core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java +++ b/core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java @@ -74,6 +74,8 @@ public class DeleteByQueryRequest extends AbstractBulkByScrollRequest { public void testDeleteteByQueryRequestImplementsIndicesRequestReplaceable() { @@ -96,4 +102,26 @@ public class DeleteByQueryRequestTests extends AbstractBulkByScrollRequestTestCa request.types(types); assertArrayEquals(request.types(), types); } + + public void testValidateGivenNoQuery() { + SearchRequest searchRequest = new SearchRequest(); + DeleteByQueryRequest deleteByQueryRequest = new DeleteByQueryRequest(searchRequest); + deleteByQueryRequest.indices("*"); + + ActionRequestValidationException e = deleteByQueryRequest.validate(); + + assertThat(e, is(not(nullValue()))); + assertThat(e.getMessage(), containsString("query is missing")); + } + + public void testValidateGivenValid() { + SearchRequest searchRequest = new SearchRequest(); + DeleteByQueryRequest deleteByQueryRequest = new DeleteByQueryRequest(searchRequest); + deleteByQueryRequest.indices("*"); + searchRequest.source().query(QueryBuilders.matchAllQuery()); + + ActionRequestValidationException e = deleteByQueryRequest.validate(); + + assertThat(e, is(nullValue())); + } } diff --git a/docs/reference/migration/migrate_6_0/java.asciidoc b/docs/reference/migration/migrate_6_0/java.asciidoc index e5f251c1c43..991fe165fb2 100644 --- a/docs/reference/migration/migrate_6_0/java.asciidoc +++ b/docs/reference/migration/migrate_6_0/java.asciidoc @@ -7,3 +7,9 @@ Previously the `setSource` methods and other methods that accepted byte/string r an object source did not require the XContentType to be specified. The auto-detection of the content type is no longer used, so these methods now require the XContentType as an additional argument when providing the source in bytes or as a string. + +=== `DeleteByQueryRequest` requires an explicitly set query + +In previous versions of Elasticsearch, delete by query requests without an explicit query +were accepted, match_all was used as the default query and all documents were deleted +as a result. From version 6.0.0, a `DeleteByQueryRequest` requires an explicit query be set. diff --git a/docs/reference/migration/migrate_6_0/rest.asciidoc b/docs/reference/migration/migrate_6_0/rest.asciidoc index 934d2c2e647..5ef09a15bff 100644 --- a/docs/reference/migration/migrate_6_0/rest.asciidoc +++ b/docs/reference/migration/migrate_6_0/rest.asciidoc @@ -47,3 +47,9 @@ requests. Refresh requests that are broadcast to multiple shards that can have one or more shards fail during the request now return a 500 response instead of a 200 response in the event there is at least one failure. + +=== Delete by Query API requires an explicit query + +In previous versions of Elasticsearch, delete by query requests without an explicit query +were accepted, match_all was used as the default query and all documents were deleted +as a result. From version 6.0.0, delete by query requests require an explicit query. diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java index 01c5977e822..59c18320959 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java @@ -32,6 +32,7 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.engine.Engine; import org.elasticsearch.index.engine.Engine.Operation.Origin; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.shard.IndexingOperationListener; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.ingest.IngestTestPlugin; @@ -216,9 +217,10 @@ public class CancelTests extends ReindexTestCase { } public void testDeleteByQueryCancel() throws Exception { - testCancel(DeleteByQueryAction.NAME, deleteByQuery().source(INDEX), (response, total, modified) -> { - assertThat(response, matcher().deleted(modified).reasonCancelled(equalTo("by user request"))); - assertHitCount(client().prepareSearch(INDEX).setSize(0).get(), total - modified); + testCancel(DeleteByQueryAction.NAME, deleteByQuery().source(INDEX).filter(QueryBuilders.matchAllQuery()), + (response, total, modified) -> { + assertThat(response, matcher().deleted(modified).reasonCancelled(equalTo("by user request"))); + assertHitCount(client().prepareSearch(INDEX).setSize(0).get(), total - modified); }, equalTo("delete-by-query [" + INDEX + "]")); } @@ -250,9 +252,10 @@ public class CancelTests extends ReindexTestCase { } public void testDeleteByQueryCancelWithWorkers() throws Exception { - testCancel(DeleteByQueryAction.NAME, deleteByQuery().source(INDEX).setSlices(5), (response, total, modified) -> { - assertThat(response, matcher().deleted(modified).reasonCancelled(equalTo("by user request")).slices(hasSize(5))); - assertHitCount(client().prepareSearch(INDEX).setSize(0).get(), total - modified); + testCancel(DeleteByQueryAction.NAME, deleteByQuery().source(INDEX).filter(QueryBuilders.matchAllQuery()).setSlices(5), + (response, total, modified) -> { + assertThat(response, matcher().deleted(modified).reasonCancelled(equalTo("by user request")).slices(hasSize(5))); + assertHitCount(client().prepareSearch(INDEX).setSize(0).get(), total - modified); }, equalTo("delete-by-query [" + INDEX + "]")); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryBasicTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryBasicTests.java index 4d920600a5d..aaab77f543f 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryBasicTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryBasicTests.java @@ -56,7 +56,7 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 5); // Deletes the two first docs with limit by size - DeleteByQueryRequestBuilder request = deleteByQuery().source("test").size(2).refresh(true); + DeleteByQueryRequestBuilder request = deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()).size(2).refresh(true); request.source().addSort("foo.keyword", SortOrder.ASC); assertThat(request.get(), matcher().deleted(2)); assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 3); @@ -66,7 +66,7 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 3); // Deletes all remaining docs - assertThat(deleteByQuery().source("test").refresh(true).get(), matcher().deleted(3)); + assertThat(deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()).refresh(true).get(), matcher().deleted(3)); assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 0); } @@ -79,7 +79,7 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { } indexRandom(true, true, true, builders); - assertThat(deleteByQuery().source("t*").refresh(true).get(), matcher().deleted(docs)); + assertThat(deleteByQuery().source("t*").filter(QueryBuilders.matchAllQuery()).refresh(true).get(), matcher().deleted(docs)); assertHitCount(client().prepareSearch("test").setSize(0).get(), 0); } @@ -122,7 +122,7 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { assertHitCount(client().prepareSearch().setSize(0).get(), 1); try { - deleteByQuery().source("missing").get(); + deleteByQuery().source("missing").filter(QueryBuilders.matchAllQuery()).get(); fail("should have thrown an exception because of a missing index"); } catch (IndexNotFoundException e) { // Ok @@ -151,7 +151,7 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { long expected = client().prepareSearch().setSize(0).setRouting(routing).get().getHits().getTotalHits(); logger.info("--> delete all documents with routing [{}] with a delete-by-query", routing); - DeleteByQueryRequestBuilder delete = deleteByQuery().source("test"); + DeleteByQueryRequestBuilder delete = deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()); delete.source().setRouting(routing); assertThat(delete.refresh(true).get(), matcher().deleted(expected)); @@ -202,7 +202,8 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { try { enableIndexBlock("test", IndexMetaData.SETTING_READ_ONLY); - assertThat(deleteByQuery().source("test").refresh(true).get(), matcher().deleted(0).failures(docs)); + assertThat(deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()).refresh(true).get(), + matcher().deleted(0).failures(docs)); } finally { disableIndexBlock("test", IndexMetaData.SETTING_READ_ONLY); } @@ -228,7 +229,8 @@ public class DeleteByQueryBasicTests extends ReindexTestCase { assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 5); // Delete remaining docs - DeleteByQueryRequestBuilder request = deleteByQuery().source("test").refresh(true).setSlices(5); + DeleteByQueryRequestBuilder request = deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()).refresh(true) + .setSlices(5); assertThat(request.get(), matcher().deleted(5).slices(hasSize(5))); assertHitCount(client().prepareSearch("test").setTypes("test").setSize(0).get(), 0); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RethrottleTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RethrottleTests.java index a29b7238d4e..91c184e16a6 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RethrottleTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RethrottleTests.java @@ -26,6 +26,7 @@ import org.elasticsearch.action.bulk.byscroll.AbstractBulkByScrollRequestBuilder import org.elasticsearch.action.bulk.byscroll.BulkByScrollResponse; import org.elasticsearch.action.bulk.byscroll.BulkByScrollTask; import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.tasks.TaskId; import java.util.ArrayList; @@ -57,7 +58,7 @@ public class RethrottleTests extends ReindexTestCase { } public void testDeleteByQuery() throws Exception { - testCase(deleteByQuery().source("test"), DeleteByQueryAction.NAME); + testCase(deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()), DeleteByQueryAction.NAME); } public void testReindexWithWorkers() throws Exception { @@ -69,13 +70,13 @@ public class RethrottleTests extends ReindexTestCase { } public void testDeleteByQueryWithWorkers() throws Exception { - testCase(deleteByQuery().source("test").setSlices(between(2, 10)), DeleteByQueryAction.NAME); + testCase(deleteByQuery().source("test").filter(QueryBuilders.matchAllQuery()).setSlices(between(2, 10)), DeleteByQueryAction.NAME); } private void testCase(AbstractBulkByScrollRequestBuilder request, String actionName) throws Exception { logger.info("Starting test for [{}] with [{}] slices", actionName, request.request().getSlices()); /* Add ten documents per slice so most slices will have many documents to process, having to go to multiple batches. - * we can't rely on all of them doing so, but + * we can't rely on all of them doing so, but */ List docs = new ArrayList<>(); for (int i = 0; i < request.request().getSlices() * 10; i++) { @@ -158,7 +159,7 @@ public class RethrottleTests extends ReindexTestCase { * are rethrottled, the finished ones just keep whatever requests per second they had while they were running. But it might * also be less than newRequestsPerSecond because the newRequestsPerSecond is divided among running sub-requests and then the * requests are rethrottled. If one request finishes in between the division and the application of the new throttle then it - * won't be rethrottled, thus only contributing its lower total. */ + * won't be rethrottled, thus only contributing its lower total. */ assertEquals(totalRequestsPerSecond, status.getRequestsPerSecond(), totalRequestsPerSecond * 0.0001f); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RetryTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RetryTests.java index e94dcfeb122..6e8da59eee3 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RetryTests.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RetryTests.java @@ -35,6 +35,8 @@ import org.elasticsearch.common.network.NetworkModule; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.util.concurrent.EsRejectedExecutionException; +import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.reindex.remote.RemoteInfo; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESSingleNodeTestCase; @@ -134,8 +136,8 @@ public class RetryTests extends ESSingleNodeTestCase { } public void testDeleteByQuery() throws Exception { - testCase(DeleteByQueryAction.NAME, DeleteByQueryAction.INSTANCE.newRequestBuilder(client()).source("source"), - matcher().deleted(DOC_COUNT)); + testCase(DeleteByQueryAction.NAME, DeleteByQueryAction.INSTANCE.newRequestBuilder(client()).source("source") + .filter(QueryBuilders.matchAllQuery()), matcher().deleted(DOC_COUNT)); } private void testCase(String action, AbstractBulkByScrollRequestBuilder request, BulkIndexByScrollResponseMatcher matcher) diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yaml b/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yaml index 4aa63facc24..7527db94842 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yaml +++ b/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yaml @@ -5,6 +5,19 @@ delete_by_query: index: _all +--- +"no query fails": + + - skip: + version: " - 5.99.99" + reason: explicit query is required since 6.0.0 + + - do: + catch: /query is missing/ + delete_by_query: + index: _all + body: {} + --- "invalid conflicts fails": - do: