Fix remove alias with must_exist (#65141)

henningandersen · javanna · web-flow · commit 67e1128e05b0 · 2020-11-18T16:00:15.000+01:00
Remove alias now parses the must_exist flag and results in a 404 (not found), if the index does not have the alias. Closes #62642 Relates #58100 Co-Authored-By: Luca Cavanna <javanna@users.noreply.github.com>
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_must_exist.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.update_aliases/40_must_exist.yml
@@ -0,0 +1,84 @@
+---
+"Remove alias with must_exist":
+
+  - skip:
+      version: " - 7.99.99" # TODO: 7.10.99
+      reason: "must_exist does not work until 7.11"
+
+  - do:
+      indices.create:
+        index: test_index
+
+  - do:
+      indices.update_aliases:
+        body:
+          actions:
+            - add:
+                index: test_index
+                aliases: test_alias1
+  - do:
+      indices.exists_alias:
+        name: test_alias1
+  - is_true: ''
+
+  - do:
+      indices.update_aliases:
+        body:
+          actions:
+            - remove:
+                index: test_index
+                alias: test_alias1
+                must_exist: true
+            - add:
+                index: test_index
+                alias: test_alias2
+  - do:
+      indices.exists_alias:
+        name: test_alias1
+  - is_false: ''
+  - do:
+      indices.exists_alias:
+        name: test_alias2
+  - is_true: ''
+
+  - do:
+      catch: missing
+      indices.update_aliases:
+        body:
+          actions:
+            - remove:
+                index: test_index
+                alias: test_alias1
+                must_exist: true
+            - add:
+                index: test_index
+                alias: test_alias3
+  - do:
+      indices.exists_alias:
+        name: test_alias3
+  - is_false: ''
+
+---
+"Alias must_exist only on remove":
+  - do:
+      indices.create:
+        index: test_index
+
+  - do:
+      catch: bad_request
+      indices.update_aliases:
+        body:
+          actions:
+            - add:
+                index: test_index
+                aliases: test_alias1
+                must_exist: true
+
+  - do:
+      catch: bad_request
+      indices.update_aliases:
+        body:
+          actions:
+            - remove_index:
+                index: test_index
+                must_exist: true
diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java
@@ -177,6 +177,9 @@ private static ObjectParser<AliasActions, Void> parser(String name, Supplier<Ali
         }
 
         private static final ObjectParser<AliasActions, Void> ADD_PARSER = parser(ADD.getPreferredName(), AliasActions::add);
+        private static final ObjectParser<AliasActions, Void> REMOVE_PARSER = parser(REMOVE.getPreferredName(), AliasActions::remove);
+        private static final ObjectParser<AliasActions, Void> REMOVE_INDEX_PARSER = parser(REMOVE_INDEX.getPreferredName(),
+            AliasActions::removeIndex);
         static {
             ADD_PARSER.declareObject(AliasActions::filter, (parser, m) -> {
                 try {
@@ -191,11 +194,8 @@ 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);
+            REMOVE_PARSER.declareField(AliasActions::mustExist, XContentParser::booleanValue, MUST_EXIST, ValueType.BOOLEAN);
         }
-        private static final ObjectParser<AliasActions, Void> REMOVE_PARSER = parser(REMOVE.getPreferredName(), AliasActions::remove);
-        private static final ObjectParser<AliasActions, Void> REMOVE_INDEX_PARSER = parser(REMOVE_INDEX.getPreferredName(),
-                AliasActions::removeIndex);
 
         /**
          * Parser for any one {@link AliasAction}.
@@ -524,6 +524,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;
@@ -544,6 +547,7 @@ public String toString() {
                     + ",indexRouting=" + indexRouting
                     + ",searchRouting=" + searchRouting
                     + ",writeIndex=" + writeIndex
+                    + ",mustExist=" + mustExist
                     + "]";
         }
 
@@ -562,12 +566,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/elasticsearch/action/admin/indices/alias/TransportIndicesAliasesAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/alias/TransportIndicesAliasesAction.java
@@ -171,6 +171,9 @@ private static String[] concreteAliases(AliasActions action, Metadata metadata,
                     finalAliases.add(aliasMeta.alias());
                 }
             }
+            if (finalAliases.isEmpty() && action.mustExist() != null && action.mustExist()) {
+                return action.aliases();
+            }
             return finalAliases.toArray(new String[finalAliases.size()]);
         } else {
             //for ADD and REMOVE_INDEX we just return the current aliases
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasAction.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasAction.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.cluster.metadata;
 
+import org.elasticsearch.ResourceNotFoundException;
 import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Strings;
@@ -179,8 +180,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 ResourceNotFoundException("required alias [" + alias  + "] does not exist");
                 }
                 return false;
             }
diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/alias/AliasActionsTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/alias/AliasActionsTests.java
@@ -216,6 +216,7 @@ public void testParseAddDefaultRouting() throws IOException {
     public void testParseRemove() throws IOException {
         String[] indices = generateRandomStringArray(10, 5, false, false);
         String[] aliases = generateRandomStringArray(10, 5, false, false);
+        Boolean mustExist = null;
         XContentBuilder b = XContentBuilder.builder(randomFrom(XContentType.values()).xContent());
         b.startObject();
         {
@@ -231,6 +232,10 @@ public void testParseRemove() throws IOException {
                 } else {
                     b.field("alias", aliases[0]);
                 }
+                if (randomBoolean()) {
+                    mustExist = randomBoolean();
+                    b.field("must_exist", mustExist);
+                }
             }
             b.endObject();
         }
@@ -241,6 +246,7 @@ public void testParseRemove() throws IOException {
             assertEquals(AliasActions.Type.REMOVE, action.actionType());
             assertThat(action.indices(), equalTo(indices));
             assertThat(action.aliases(), equalTo(aliases));
+            assertThat(action.mustExist(), equalTo(mustExist));
         }
     }
 
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexAliasesServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexAliasesServiceTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.cluster.metadata;
 
+import org.elasticsearch.ResourceNotFoundException;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.ClusterName;
 import org.elasticsearch.cluster.ClusterState;
@@ -138,7 +139,7 @@ public void testMustExist() {
 
         // Show that removing non-existing alias with mustExist == true fails
         final ClusterState finalCS = after;
-        final IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
+        final ResourceNotFoundException iae = expectThrows(ResourceNotFoundException.class,
             () -> service.applyAliasActions(finalCS, singletonList(new AliasAction.Remove(index, "test_2", true))));
         assertThat(iae.getMessage(), containsString("required alias [test_2] does not exist"));
     }
diff --git a/test/framework/src/main/java/org/elasticsearch/index/alias/RandomAliasActionsGenerator.java b/test/framework/src/main/java/org/elasticsearch/index/alias/RandomAliasActionsGenerator.java
@@ -54,6 +54,11 @@ public static AliasActions randomAliasAction(boolean useStringAsFilter) {
             }
             action.indices(indices);
         }
+        if (action.actionType() == AliasActions.Type.REMOVE) {
+            if (randomBoolean()) {
+                action.mustExist(randomBoolean());
+            }
+        }
         if (action.actionType() != AliasActions.Type.REMOVE_INDEX) {
             if (randomBoolean()) {
                 action.alias(randomAlphaOfLength(5));

Original file line number	Diff line number	Diff line change
`@@ -171,6 +171,9 @@ private static String[] concreteAliases(AliasActions action, Metadata metadata,`
`171`	`171`	`finalAliases.add(aliasMeta.alias());`
`172`	`172`	`}`
`173`	`173`	`}`
	`174`	`+ if (finalAliases.isEmpty() && action.mustExist() != null && action.mustExist()) {`
	`175`	`+ return action.aliases();`
	`176`	`+ }`
`174`	`177`	`return finalAliases.toArray(new String[finalAliases.size()]);`
`175`	`178`	`} else {`
`176`	`179`	`//for ADD and REMOVE_INDEX we just return the current aliases`
Original file line number	Diff line number	Diff line change
`@@ -216,6 +216,7 @@ public void testParseAddDefaultRouting() throws IOException {`
`216`	`216`	`public void testParseRemove() throws IOException {`
`217`	`217`	`String[] indices = generateRandomStringArray(10, 5, false, false);`
`218`	`218`	`String[] aliases = generateRandomStringArray(10, 5, false, false);`
	`219`	`+ Boolean mustExist = null;`
`219`	`220`	`XContentBuilder b = XContentBuilder.builder(randomFrom(XContentType.values()).xContent());`
`220`	`221`	`b.startObject();`
`221`	`222`	`{`
`@@ -231,6 +232,10 @@ public void testParseRemove() throws IOException {`
`231`	`232`	`} else {`
`232`	`233`	`b.field("alias", aliases[0]);`
`233`	`234`	`}`
	`235`	`+ if (randomBoolean()) {`
	`236`	`+ mustExist = randomBoolean();`
	`237`	`+ b.field("must_exist", mustExist);`
	`238`	`+ }`
`234`	`239`	`}`
`235`	`240`	`b.endObject();`
`236`	`241`	`}`
`@@ -241,6 +246,7 @@ public void testParseRemove() throws IOException {`
`241`	`246`	`assertEquals(AliasActions.Type.REMOVE, action.actionType());`
`242`	`247`	`assertThat(action.indices(), equalTo(indices));`
`243`	`248`	`assertThat(action.aliases(), equalTo(aliases));`
	`249`	`+ assertThat(action.mustExist(), equalTo(mustExist));`
`244`	`250`	`}`
`245`	`251`	`}`
`246`	`252`
Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,11 @@ public static AliasActions randomAliasAction(boolean useStringAsFilter) {`
`54`	`54`	`}`
`55`	`55`	`action.indices(indices);`
`56`	`56`	`}`
	`57`	`+ if (action.actionType() == AliasActions.Type.REMOVE) {`
	`58`	`+ if (randomBoolean()) {`
	`59`	`+ action.mustExist(randomBoolean());`
	`60`	`+ }`
	`61`	`+ }`
`57`	`62`	`if (action.actionType() != AliasActions.Type.REMOVE_INDEX) {`
`58`	`63`	`if (randomBoolean()) {`
`59`	`64`	`action.alias(randomAlphaOfLength(5));`