From 056b9f4ae5a5be1a6eb77edc098f9839efcb8d65 Mon Sep 17 00:00:00 2001 From: gaobinlong Date: Tue, 9 Jan 2024 22:11:04 +0800 Subject: [PATCH] Enable must_exist parameter for update aliases API (#11210) * Enable must_exist parameter for update aliases API Signed-off-by: Gao Binlong * modify changelog Signed-off-by: Gao Binlong * Fix test failure Signed-off-by: Gao Binlong * Fix test failure Signed-off-by: Gao Binlong * Remove silently when all aliases are non-existing and must_exist is false Signed-off-by: Gao Binlong * Modify some comments to run gradle check again Signed-off-by: Gao Binlong * Add more yaml test Signed-off-by: Gao Binlong --------- Signed-off-by: Gao Binlong (cherry picked from commit a1e4602b8dcd5676d1e7b410d86242d7a555d619) --- CHANGELOG.md | 1 + .../40_remove_with_must_exist.yml | 141 ++++++++++++++++++ .../indices/alias/IndicesAliasesRequest.java | 14 +- .../alias/TransportIndicesAliasesAction.java | 27 +++- .../cluster/metadata/AliasAction.java | 5 +- .../indices/alias/AliasActionsTests.java | 7 + .../MetadataIndexAliasesServiceTests.java | 7 +- .../alias/RandomAliasActionsGenerator.java | 5 + 8 files changed, 197 insertions(+), 10 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_remove_with_must_exist.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index c942e593d9566..b12ed5e47b3fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Implement on behalf of token passing for extensions ([#8679](https://github.com/opensearch-project/OpenSearch/pull/8679)) - Provide service accounts tokens to extensions ([#9618](https://github.com/opensearch-project/OpenSearch/pull/9618)) - [Streaming Indexing] Introduce new experimental server HTTP transport based on Netty 4 and Project Reactor (Reactor Netty) ([#9672](https://github.com/opensearch-project/OpenSearch/pull/9672)) +- Enable must_exist parameter for update aliases API ([#11210](https://github.com/opensearch-project/OpenSearch/pull/11210)) - Add back half_float BKD based sort query optimization ([#11024](https://github.com/opensearch-project/OpenSearch/pull/11024)) - Request level coordinator slow logs ([#11246](https://github.com/opensearch-project/OpenSearch/pull/11246)) - Add template snippets support for field and target_field in KV ingest processor ([#10040](https://github.com/opensearch-project/OpenSearch/pull/10040)) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_remove_with_must_exist.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_remove_with_must_exist.yml new file mode 100644 index 0000000000000..5d7a0798f09b4 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_remove_with_must_exist.yml @@ -0,0 +1,141 @@ +--- +"Throw aliases missing exception when removing non-existing alias with setting must_exist to true": + - skip: + version: " - 2.11.99" + reason: "introduced in 2.12.0" + + - do: + indices.create: + index: test_index + + - do: + indices.exists_alias: + name: test_alias + + - is_false: '' + + - do: + catch: /aliases \[test_alias\] missing/ + indices.update_aliases: + body: + actions: + - remove: + index: test_index + alias: test_alias + must_exist: true + + - do: + catch: /aliases \[testAlias\*\] missing/ + indices.update_aliases: + body: + actions: + - remove: + index: test_index + aliases: [ testAlias* ] + must_exist: true + + - do: + catch: /\[aliases\] can't be empty/ + indices.update_aliases: + body: + actions: + - remove: + index: test_index + aliases: [] + must_exist: true + +--- +"Throw aliases missing exception when all of the specified aliases are non-existing": + - skip: + version: " - 2.99.99" + reason: "introduced in 3.0" + + - do: + indices.create: + index: test_index + + - do: + indices.exists_alias: + name: test_alias + + - is_false: '' + + - do: + catch: /aliases \[test\_alias\] missing/ + indices.update_aliases: + body: + actions: + - remove: + index: test_index + alias: test_alias + + - do: + catch: /aliases \[test\_alias\*\] missing/ + indices.update_aliases: + body: + actions: + - remove: + indices: [ test_index ] + aliases: [ test_alias* ] + +--- +"Remove successfully when some specified aliases are non-existing": + - skip: + version: " - 2.99.99" + reason: "introduced in 3.0" + + - do: + indices.create: + index: test_index + + - do: + indices.exists_alias: + name: test_alias + + - is_false: '' + + - do: + indices.update_aliases: + body: + actions: + - add: + indices: [ test_index ] + aliases: [ test_alias ] + + - do: + indices.update_aliases: + body: + actions: + - remove: + index: test_index + aliases: [test_alias, test_alias1, test_alias2] + must_exist: false + + - match: { acknowledged: true } + +--- +"Remove silently when all of the specified aliases are non-existing and must_exist is false": + - skip: + version: " - 2.99.99" + reason: "introduced in 3.0" + + - do: + indices.create: + index: test_index + + - do: + indices.exists_alias: + name: test_alias + + - is_false: '' + + - do: + indices.update_aliases: + body: + actions: + - remove: + index: test_index + aliases: [test_alias, test_alias1, test_alias2] + must_exist: false + + - match: { acknowledged: true } diff --git a/server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java b/server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java index 26ccb793e2e2c..62f67a27df938 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java @@ -225,9 +225,11 @@ private static ObjectParser parser(String name, Supplier REMOVE_PARSER = parser(REMOVE.getPreferredName(), AliasActions::remove); + static { + REMOVE_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN); + } private static final ObjectParser REMOVE_INDEX_PARSER = parser( REMOVE_INDEX.getPreferredName(), AliasActions::removeIndex @@ -575,6 +577,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (null != isHidden) { builder.field(IS_HIDDEN.getPreferredName(), isHidden); } + if (null != mustExist) { + builder.field(MUST_EXIST.getPreferredName(), mustExist); + } builder.endObject(); builder.endObject(); return builder; @@ -603,6 +608,8 @@ public String toString() { + searchRouting + ",writeIndex=" + writeIndex + + ",mustExist=" + + mustExist + "]"; } @@ -621,12 +628,13 @@ public boolean equals(Object obj) { && Objects.equals(indexRouting, other.indexRouting) && Objects.equals(searchRouting, other.searchRouting) && Objects.equals(writeIndex, other.writeIndex) - && Objects.equals(isHidden, other.isHidden); + && Objects.equals(isHidden, other.isHidden) + && Objects.equals(mustExist, other.mustExist); } @Override public int hashCode() { - return Objects.hash(type, indices, aliases, filter, routing, indexRouting, searchRouting, writeIndex, isHidden); + return Objects.hash(type, indices, aliases, filter, routing, indexRouting, searchRouting, writeIndex, isHidden, mustExist); } } diff --git a/server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java b/server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java index 77cc0662a1c96..b15c420335792 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java @@ -50,6 +50,7 @@ import org.opensearch.cluster.metadata.MetadataIndexAliasesService; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.inject.Inject; +import org.opensearch.common.regex.Regex; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.index.Index; @@ -59,6 +60,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -220,13 +222,34 @@ private static String[] concreteAliases(IndicesAliasesRequest.AliasActions actio // for DELETE we expand the aliases String[] indexAsArray = { concreteIndex }; final Map> aliasMetadata = metadata.findAliases(action, indexAsArray); - List finalAliases = new ArrayList<>(); + Set finalAliases = new HashSet<>(); for (final List curAliases : aliasMetadata.values()) { for (AliasMetadata aliasMeta : curAliases) { finalAliases.add(aliasMeta.alias()); } } - return finalAliases.toArray(new String[finalAliases.size()]); + + // must_exist can only be set in the Remove Action in Update aliases API, + // we check the value here to make the behavior consistent with Delete aliases API + if (action.mustExist() != null) { + // if must_exist is false, we should make the remove action execute silently, + // so we return the original specified aliases to avoid AliasesNotFoundException + if (!action.mustExist()) { + return action.aliases(); + } + + // if there is any non-existing aliases specified in the request and must_exist is true, throw exception in advance + if (finalAliases.isEmpty()) { + throw new AliasesNotFoundException(action.aliases()); + } + String[] nonExistingAliases = Arrays.stream(action.aliases()) + .filter(originalAlias -> finalAliases.stream().noneMatch(finalAlias -> Regex.simpleMatch(originalAlias, finalAlias))) + .toArray(String[]::new); + if (nonExistingAliases.length != 0) { + throw new AliasesNotFoundException(nonExistingAliases); + } + } + return finalAliases.toArray(new String[0]); } else { // for ADD and REMOVE_INDEX we just return the current aliases return action.aliases(); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/AliasAction.java b/server/src/main/java/org/opensearch/cluster/metadata/AliasAction.java index 47c6cad04343e..dac6ecaa13781 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/AliasAction.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/AliasAction.java @@ -35,6 +35,7 @@ import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest; import org.opensearch.common.Nullable; import org.opensearch.core.common.Strings; +import org.opensearch.rest.action.admin.indices.AliasesNotFoundException; /** * Individual operation to perform on the cluster state as part of an {@link IndicesAliasesRequest}. @@ -225,8 +226,8 @@ boolean removeIndex() { @Override boolean apply(NewAliasValidator aliasValidator, Metadata.Builder metadata, IndexMetadata index) { if (false == index.getAliases().containsKey(alias)) { - if (mustExist != null && mustExist.booleanValue()) { - throw new IllegalArgumentException("required alias [" + alias + "] does not exist"); + if (mustExist != null && mustExist) { + throw new AliasesNotFoundException(alias); } return false; } diff --git a/server/src/test/java/org/opensearch/action/admin/indices/alias/AliasActionsTests.java b/server/src/test/java/org/opensearch/action/admin/indices/alias/AliasActionsTests.java index 8ba8d226715ed..d463782a70506 100644 --- a/server/src/test/java/org/opensearch/action/admin/indices/alias/AliasActionsTests.java +++ b/server/src/test/java/org/opensearch/action/admin/indices/alias/AliasActionsTests.java @@ -241,6 +241,7 @@ public void testParseRemove() throws IOException { String[] indices = generateRandomStringArray(10, 5, false, false); String[] aliases = generateRandomStringArray(10, 5, false, false); XContentBuilder b = XContentBuilder.builder(randomFrom(XContentType.values()).xContent()); + boolean mustExist = randomBoolean(); b.startObject(); { b.startObject("remove"); @@ -255,6 +256,9 @@ public void testParseRemove() throws IOException { } else { b.field("alias", aliases[0]); } + if (mustExist) { + b.field("must_exist", true); + } } b.endObject(); } @@ -265,6 +269,9 @@ public void testParseRemove() throws IOException { assertEquals(AliasActions.Type.REMOVE, action.actionType()); assertThat(action.indices(), equalTo(indices)); assertThat(action.aliases(), equalTo(aliases)); + if (mustExist) { + assertThat(action.mustExist(), equalTo(true)); + } } } diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexAliasesServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexAliasesServiceTests.java index bf66f577e182b..9fb4551f106ec 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexAliasesServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataIndexAliasesServiceTests.java @@ -40,6 +40,7 @@ import org.opensearch.common.util.set.Sets; import org.opensearch.core.index.Index; import org.opensearch.index.IndexNotFoundException; +import org.opensearch.rest.action.admin.indices.AliasesNotFoundException; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.VersionUtils; @@ -164,11 +165,11 @@ public void testMustExist() { // Show that removing non-existing alias with mustExist == true fails final ClusterState finalCS = after; - final IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, + final AliasesNotFoundException iae = expectThrows( + AliasesNotFoundException.class, () -> service.applyAliasActions(finalCS, singletonList(new AliasAction.Remove(index, "test_2", true))) ); - assertThat(iae.getMessage(), containsString("required alias [test_2] does not exist")); + assertThat(iae.getMessage(), containsString("aliases [test_2] missing")); } public void testMultipleIndices() { diff --git a/test/framework/src/main/java/org/opensearch/index/alias/RandomAliasActionsGenerator.java b/test/framework/src/main/java/org/opensearch/index/alias/RandomAliasActionsGenerator.java index ce862d2b69d8f..fb9db9a578b4a 100644 --- a/test/framework/src/main/java/org/opensearch/index/alias/RandomAliasActionsGenerator.java +++ b/test/framework/src/main/java/org/opensearch/index/alias/RandomAliasActionsGenerator.java @@ -102,6 +102,11 @@ public static AliasActions randomAliasAction(boolean useStringAsFilter) { action.isHidden(randomBoolean()); } } + if (action.actionType() == AliasActions.Type.REMOVE) { + if (randomBoolean()) { + action.mustExist(randomBoolean()); + } + } return action; }