From e08e92d934e8c97c94f19f5811507175dcec3c58 Mon Sep 17 00:00:00 2001 From: olcbean Date: Mon, 22 May 2017 09:00:22 +0200 Subject: [PATCH] Deleting a document from a non-existing index creates the should not auto create it, unless using EXTERNAL* versioning (#24518) Currently a `delete document` request against a non-existing index actually **creates** this index. With this change the `delete document` no longer creates the previously non-existing index and throws an `index_not_found` exception instead. However as discussed in https://github.com/elastic/elasticsearch/pull/15451#issuecomment-165772026, if an external version is explicitly used, the current behavior is preserved and the index is still created and the document is marked for deletion. Fixes #15425 --- .../java/org/elasticsearch/client/CrudIT.java | 21 ++- .../action/bulk/TransportBulkAction.java | 6 + ...ActionIndicesThatCannotBeCreatedTests.java | 3 +- .../action/bulk/TransportBulkActionTests.java | 135 ++++++++++++++++++ docs/reference/docs/delete.asciidoc | 3 +- .../migration/migrate_6_0/indices.asciidoc | 6 + 6 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java index 8db50535cc8..4abfd713bcd 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java @@ -55,7 +55,6 @@ import org.elasticsearch.threadpool.ThreadPool; import java.io.IOException; import java.util.Collections; import java.util.Map; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import static java.util.Collections.singletonMap; @@ -63,16 +62,6 @@ import static java.util.Collections.singletonMap; public class CrudIT extends ESRestHighLevelClientTestCase { public void testDelete() throws IOException { - { - // Testing non existing document - String docId = "does_not_exist"; - DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId); - DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync); - assertEquals("index", deleteResponse.getIndex()); - assertEquals("type", deleteResponse.getType()); - assertEquals(docId, deleteResponse.getId()); - assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult()); - } { // Testing deletion String docId = "id"; @@ -87,6 +76,16 @@ public class CrudIT extends ESRestHighLevelClientTestCase { assertEquals(docId, deleteResponse.getId()); assertEquals(DocWriteResponse.Result.DELETED, deleteResponse.getResult()); } + { + // Testing non existing document + String docId = "does_not_exist"; + DeleteRequest deleteRequest = new DeleteRequest("index", "type", docId); + DeleteResponse deleteResponse = execute(deleteRequest, highLevelClient()::delete, highLevelClient()::deleteAsync); + assertEquals("index", deleteResponse.getIndex()); + assertEquals("type", deleteResponse.getType()); + assertEquals(docId, deleteResponse.getId()); + assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult()); + } { // Testing version conflict String docId = "version_conflict"; diff --git a/core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java b/core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java index 16c22524e2d..ebf50e770bc 100644 --- a/core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java +++ b/core/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java @@ -54,6 +54,7 @@ import org.elasticsearch.common.util.concurrent.AbstractRunnable; import org.elasticsearch.common.util.concurrent.AtomicArray; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; +import org.elasticsearch.index.VersionType; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.indices.IndexClosedException; import org.elasticsearch.ingest.IngestService; @@ -144,6 +145,11 @@ public class TransportBulkAction extends HandledTransportAction indices = bulkRequest.requests.stream() + // delete requests should not attempt to create the index (if the index does not + // exists), unless an external versioning is used + .filter(request -> request.opType() != DocWriteRequest.OpType.DELETE + || request.versionType() == VersionType.EXTERNAL + || request.versionType() == VersionType.EXTERNAL_GTE) .map(DocWriteRequest::index) .collect(Collectors.toSet()); /* Step 2: filter that to indices that don't exist and we can create. At the same time build a map of indices we can't create diff --git a/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionIndicesThatCannotBeCreatedTests.java b/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionIndicesThatCannotBeCreatedTests.java index a02939ad206..5ab9e1ea535 100644 --- a/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionIndicesThatCannotBeCreatedTests.java +++ b/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionIndicesThatCannotBeCreatedTests.java @@ -31,6 +31,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.AtomicArray; import org.elasticsearch.index.IndexNotFoundException; +import org.elasticsearch.index.VersionType; import org.elasticsearch.tasks.Task; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.transport.TransportService; @@ -66,7 +67,7 @@ public class TransportBulkActionIndicesThatCannotBeCreatedTests extends ESTestCa BulkRequest bulkRequest = new BulkRequest(); bulkRequest.add(new IndexRequest("no")); bulkRequest.add(new IndexRequest("can't")); - bulkRequest.add(new DeleteRequest("do")); + bulkRequest.add(new DeleteRequest("do").version(0).versionType(VersionType.EXTERNAL)); bulkRequest.add(new UpdateRequest("nothin", randomAlphaOfLength(5), randomAlphaOfLength(5))); indicesThatCannotBeCreatedTestCase(new HashSet<>(Arrays.asList("no", "can't", "do", "nothin")), bulkRequest, index -> { throw new IndexNotFoundException("Can't make it because I say so"); diff --git a/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java b/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java new file mode 100644 index 00000000000..50fb348834f --- /dev/null +++ b/core/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java @@ -0,0 +1,135 @@ +/* + * 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.bulk; + +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.admin.indices.create.CreateIndexResponse; +import org.elasticsearch.action.bulk.TransportBulkActionTookTests.Resolver; +import org.elasticsearch.action.delete.DeleteRequest; +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.AutoCreateIndex; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.index.IndexNotFoundException; +import org.elasticsearch.index.VersionType; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.transport.CapturingTransport; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import org.junit.After; +import org.junit.Before; + +import java.util.Collections; +import java.util.concurrent.TimeUnit; + +import static org.elasticsearch.test.ClusterServiceUtils.createClusterService; + +public class TransportBulkActionTests extends ESTestCase { + + /** Services needed by bulk action */ + private TransportService transportService; + private ClusterService clusterService; + private ThreadPool threadPool; + + private TestTransportBulkAction bulkAction; + + class TestTransportBulkAction extends TransportBulkAction { + + boolean indexCreated = false; // set when the "real" index is created + + TestTransportBulkAction() { + super(Settings.EMPTY, TransportBulkActionTests.this.threadPool, transportService, clusterService, null, null, + null, new ActionFilters(Collections.emptySet()), new Resolver(Settings.EMPTY), + new AutoCreateIndex(Settings.EMPTY, clusterService.getClusterSettings(), new Resolver(Settings.EMPTY))); + } + + @Override + protected boolean needToCheck() { + return true; + } + + @Override + void createIndex(String index, TimeValue timeout, ActionListener listener) { + indexCreated = true; + listener.onResponse(null); + } + } + + @Before + public void setUp() throws Exception { + super.setUp(); + threadPool = new TestThreadPool("TransportBulkActionTookTests"); + clusterService = createClusterService(threadPool); + CapturingTransport capturingTransport = new CapturingTransport(); + transportService = new TransportService(clusterService.getSettings(), capturingTransport, threadPool, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, + boundAddress -> clusterService.localNode(), null); + transportService.start(); + transportService.acceptIncomingRequests(); + bulkAction = new TestTransportBulkAction(); + } + + @After + public void tearDown() throws Exception { + ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); + threadPool = null; + clusterService.close(); + super.tearDown(); + } + + public void testDeleteNonExistingDocDoesNotCreateIndex() throws Exception { + BulkRequest bulkRequest = new BulkRequest().add(new DeleteRequest("index", "type", "id")); + + bulkAction.execute(null, bulkRequest, ActionListener.wrap(response -> { + assertFalse(bulkAction.indexCreated); + BulkItemResponse[] bulkResponses = ((BulkResponse) response).getItems(); + assertEquals(bulkResponses.length, 1); + assertTrue(bulkResponses[0].isFailed()); + assertTrue(bulkResponses[0].getFailure().getCause() instanceof IndexNotFoundException); + assertEquals("index", bulkResponses[0].getFailure().getIndex()); + }, exception -> { + throw new AssertionError(exception); + })); + } + + public void testDeleteNonExistingDocExternalVersionCreatesIndex() throws Exception { + BulkRequest bulkRequest = new BulkRequest() + .add(new DeleteRequest("index", "type", "id").versionType(VersionType.EXTERNAL).version(0)); + + bulkAction.execute(null, bulkRequest, ActionListener.wrap(response -> { + assertTrue(bulkAction.indexCreated); + }, exception -> { + throw new AssertionError(exception); + })); + } + + public void testDeleteNonExistingDocExternalGteVersionCreatesIndex() throws Exception { + BulkRequest bulkRequest = new BulkRequest() + .add(new DeleteRequest("index2", "type", "id").versionType(VersionType.EXTERNAL_GTE).version(0)); + + bulkAction.execute(null, bulkRequest, ActionListener.wrap(response -> { + assertTrue(bulkAction.indexCreated); + }, exception -> { + throw new AssertionError(exception); + })); + } +} \ No newline at end of file diff --git a/docs/reference/docs/delete.asciidoc b/docs/reference/docs/delete.asciidoc index 6fbe5e33ce2..f26a7fc64d0 100644 --- a/docs/reference/docs/delete.asciidoc +++ b/docs/reference/docs/delete.asciidoc @@ -105,7 +105,8 @@ thrown instead. [[delete-index-creation]] === Automatic index creation -The delete operation automatically creates an index if it has not been +If an <> is used, +the delete operation automatically creates an index if it has not been created before (check out the <> for manually creating an index), and also automatically creates a dynamic type mapping for the specific type if it has not been created diff --git a/docs/reference/migration/migrate_6_0/indices.asciidoc b/docs/reference/migration/migrate_6_0/indices.asciidoc index 0a05fd55139..95d0253fe63 100644 --- a/docs/reference/migration/migrate_6_0/indices.asciidoc +++ b/docs/reference/migration/migrate_6_0/indices.asciidoc @@ -44,3 +44,9 @@ The default value of the `allow_no_indices` option for the Open/Close index API has been changed from `false` to `true` so it is aligned with the behaviour of the Delete index API. As a result, Open/Close index API don't return an error by default when a provided wildcard expression doesn't match any closed/open index. + +==== Delete a document + +Delete a document from non-existing index has been modified to not create the index. +However if an external versioning is used the index will be created and the document +will be marked for deletion.