Enable must_exist parameter for update aliases API (opensearch-projec…

…t#11210) * Enable must_exist parameter for update aliases API Signed-off-by: Gao Binlong <gbinlong@amazon.com> * modify changelog Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Fix test failure Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Fix test failure Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Remove silently when all aliases are non-existing and must_exist is false Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Modify some comments to run gradle check again Signed-off-by: Gao Binlong <gbinlong@amazon.com> * Add more yaml test Signed-off-by: Gao Binlong <gbinlong@amazon.com> --------- Signed-off-by: Gao Binlong <gbinlong@amazon.com>
rayshrey · Mar 18, 2024 · 9108a79 · 9108a79
1 parent 2a614f0
commit 9108a79
Show file tree

Hide file tree

Showing 8 changed files with 196 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -107,6 +107,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 ([#10650](https://github.com/opensearch-project/OpenSearch/pull/10650))
 - 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/...rc/main/resources/rest-api-spec/test/indices.update_aliases/40_remove_with_must_exist.yml b/...rc/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.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
+                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
@@ -220,9 +220,11 @@ private static ObjectParser<AliasActions, Void> parser(String name, Supplier<Ali
             ADD_PARSER.declareField(AliasActions::searchRouting, XContentParser::text, SEARCH_ROUTING, ValueType.INT);
             ADD_PARSER.declareField(AliasActions::writeIndex, XContentParser::booleanValue, IS_WRITE_INDEX, ValueType.BOOLEAN);
             ADD_PARSER.declareField(AliasActions::isHidden, XContentParser::booleanValue, IS_HIDDEN, ValueType.BOOLEAN);
-            ADD_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN);
         }
         private static final ObjectParser<AliasActions, Void> REMOVE_PARSER = parser(REMOVE.getPreferredName(), AliasActions::remove);
+        static {
+            REMOVE_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN);
+        }
         private static final ObjectParser<AliasActions, Void> REMOVE_INDEX_PARSER = parser(
             REMOVE_INDEX.getPreferredName(),
             AliasActions::removeIndex
@@ -554,6 +556,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;
@@ -582,6 +587,8 @@ public String toString() {
                 + searchRouting
                 + ",writeIndex="
                 + writeIndex
+                + ",mustExist="
+                + mustExist
                 + "]";
         }
 
@@ -600,12 +607,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/...rc/main/java/org/opensearch/action/admin/indices/alias/TransportIndicesAliasesAction.java b/...rc/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,12 +222,33 @@ private static String[] concreteAliases(IndicesAliasesRequest.AliasActions actio
             // for DELETE we expand the aliases
             String[] indexAsArray = { concreteIndex };
             final Map<String, List<AliasMetadata>> aliasMetadata = metadata.findAliases(action, indexAsArray);
-            List<String> finalAliases = new ArrayList<>();
+            Set<String> finalAliases = new HashSet<>();
             for (final List<AliasMetadata> curAliases : aliasMetadata.values()) {
                 for (AliasMetadata aliasMeta : curAliases) {
                     finalAliases.add(aliasMeta.alias());
                 }
             }
+
+            // 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

diff --git 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
@@ -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
@@ -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
@@ -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;
     }