honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Require explicit query in _delete_by_query API (#23632) 

As the query of a search request defaults to match_all,
calling _delete_by_query without an explicit query may
result in deleting all data.

In order to protect users against falling into that
pitfall, this commit adds a check to require the explicit
setting of a query.

Closes #23629
This commit is contained in:
Dimitris Athanasiou 2017-03-28 15:44:57 +01:00 committed by GitHub
parent 8359dd05c9
commit 34f116eae3
9 changed files with 82 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java
View File

@ -74,6 +74,8 @@ public class DeleteByQueryRequest extends AbstractBulkByScrollRequest<DeleteByQu
}
if (getSearchRequest() == null || getSearchRequest().source() == null) {
e = addValidationError("source is missing", e);
} else if (getSearchRequest().source().query() == null) {
e = addValidationError("query is missing", e);
}
return e;
}

28
core/src/test/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequestTests.java
View File

@ -19,10 +19,16 @@
package org.elasticsearch.action.bulk.byscroll;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.index.query.QueryBuilders;
import static org.apache.lucene.util.TestUtil.randomSimpleString;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
public class DeleteByQueryRequestTests extends AbstractBulkByScrollRequestTestCase<DeleteByQueryRequest> {
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()));
}
}

6
docs/reference/migration/migrate_6_0/java.asciidoc
View File

@ -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.

6
docs/reference/migration/migrate_6_0/rest.asciidoc
View File

@ -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.

15
modules/reindex/src/test/java/org/elasticsearch/index/reindex/CancelTests.java
View File

@ -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 + "]"));
}

16
modules/reindex/src/test/java/org/elasticsearch/index/reindex/DeleteByQueryBasicTests.java
View File

@ -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);
}

9
modules/reindex/src/test/java/org/elasticsearch/index/reindex/RethrottleTests.java
View File

@ -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<IndexRequestBuilder> 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);
}

6
modules/reindex/src/test/java/org/elasticsearch/index/reindex/RetryTests.java
View File

@ -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)

13
modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yaml
View File

@ -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: