Aliases: Validate alias filter before adding it, closes #993.

modules/elasticsearch/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java

@@ -19,16 +19,28 @@
 
 package org.elasticsearch.cluster.metadata;
 
+import org.elasticsearch.ElasticSearchIllegalArgumentException;
 import org.elasticsearch.cluster.ClusterService;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ProcessedClusterStateUpdateTask;
+import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.collect.Lists;
+import org.elasticsearch.common.collect.Maps;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.Index;
+import org.elasticsearch.index.query.xcontent.XContentIndexQueryParser;
+import org.elasticsearch.index.service.IndexService;
 import org.elasticsearch.indices.IndexMissingException;
+import org.elasticsearch.indices.IndicesService;
 import org.elasticsearch.indices.InvalidAliasNameException;
 
+import java.util.List;
+import java.util.Map;
+
 import static org.elasticsearch.cluster.ClusterState.*;
 import static org.elasticsearch.cluster.metadata.IndexMetaData.*;
 import static org.elasticsearch.cluster.metadata.MetaData.*;
@@ -40,9 +52,12 @@ public class MetaDataIndexAliasesService extends AbstractComponent {
 
     private final ClusterService clusterService;
 
-    @Inject public MetaDataIndexAliasesService(Settings settings, ClusterService clusterService) {
+    private final IndicesService indicesService;
+
+    @Inject public MetaDataIndexAliasesService(Settings settings, ClusterService clusterService, IndicesService indicesService) {
         super(settings);
         this.clusterService = clusterService;
+        this.indicesService = indicesService;
     }
 
     public void indicesAliases(final Request request, final Listener listener) {
@@ -60,6 +75,9 @@ public class MetaDataIndexAliasesService extends AbstractComponent {
                     }
                 }
 
+                List<String> indicesToClose = Lists.newArrayList();
+                Map<String, IndexService> indices = Maps.newHashMap();
+                try {
                     MetaData.Builder builder = newMetaDataBuilder().metaData(currentState.metaData());
                     for (AliasAction aliasAction : request.actions) {
                         IndexMetaData indexMetaData = builder.get(aliasAction.index());
@@ -68,7 +86,35 @@ public class MetaDataIndexAliasesService extends AbstractComponent {
                         }
                         IndexMetaData.Builder indexMetaDataBuilder = newIndexMetaDataBuilder(indexMetaData);
                         if (aliasAction.actionType() == AliasAction.Type.ADD) {
-                        indexMetaDataBuilder.putAlias(AliasMetaData.newAliasMetaDataBuilder(aliasAction.alias()).filter(aliasAction.filter()).build());
+                            String filter = aliasAction.filter();
+                            if (Strings.hasLength(filter)) {
+                                // parse the filter, in order to validate it
+                                IndexService indexService = indices.get(indexMetaData.index());
+                                if (indexService == null) {
+                                    indexService = indicesService.indexService(indexMetaData.index());
+                                    if (indexService == null) {
+                                        // temporarily create the index so we have can parse the filter
+                                        indexService = indicesService.createIndex(indexMetaData.index(), indexMetaData.settings(), currentState.nodes().localNode().id());
+                                        indicesToClose.add(indexMetaData.index());
+                                    }
+                                    indices.put(indexMetaData.index(), indexService);
+                                }
+
+                                // now, parse the filter
+                                XContentIndexQueryParser indexQueryParser = (XContentIndexQueryParser) indexService.queryParserService().defaultIndexQueryParser();
+                                try {
+                                    XContentParser parser = XContentFactory.xContent(filter).createParser(filter);
+                                    try {
+                                        indexQueryParser.parseInnerFilter(parser);
+                                    } finally {
+                                        parser.close();
+                                    }
+                                } catch (Exception e) {
+                                    listener.onFailure(new ElasticSearchIllegalArgumentException("failed to parse filter for [" + aliasAction.alias() + "]", e));
+                                    return currentState;
+                                }
+                            }
+                            indexMetaDataBuilder.putAlias(AliasMetaData.newAliasMetaDataBuilder(aliasAction.alias()).filter(filter).build());
                         } else if (aliasAction.actionType() == AliasAction.Type.REMOVE) {
                             indexMetaDataBuilder.removerAlias(aliasAction.alias());
                         }
@@ -76,6 +122,11 @@ public class MetaDataIndexAliasesService extends AbstractComponent {
                         builder.put(indexMetaDataBuilder);
                     }
                     return newClusterStateBuilder().state(currentState).metaData(builder).build();
+                } finally {
+                    for (String index : indicesToClose) {
+                        indicesService.cleanIndex(index, "created for mapping processing");
+                    }
+                }
             }
 
             @Override public void clusterStateProcessed(ClusterState clusterState) {

modules/test/integration/src/test/java/org/elasticsearch/test/integration/aliases/IndexAliasesTests.java

@@ -32,14 +32,14 @@ import org.elasticsearch.indices.IndexMissingException;
 import org.elasticsearch.search.SearchHit;
 import org.elasticsearch.search.SearchHits;
 import org.elasticsearch.test.integration.AbstractNodesTests;
-import org.testng.annotations.AfterMethod;
+import org.testng.annotations.AfterClass;
-import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
 import java.util.Set;
 
 import static org.elasticsearch.client.Requests.*;
-import static org.elasticsearch.common.collect.Sets.newHashSet;
+import static org.elasticsearch.common.collect.Sets.*;
 import static org.elasticsearch.index.query.xcontent.FilterBuilders.*;
 import static org.hamcrest.MatcherAssert.*;
 import static org.hamcrest.Matchers.*;
@@ -53,14 +53,14 @@ public class IndexAliasesTests extends AbstractNodesTests {
     protected Client client1;
     protected Client client2;
 
-    @BeforeMethod public void startNodes() {
+    @BeforeClass public void startNodes() {
         startNode("server1");
         startNode("server2");
         client1 = getClient1();
         client2 = getClient2();
     }
 
-    @AfterMethod public void closeNodes() {
+    @AfterClass public void closeNodes() {
         client1.close();
         client2.close();
         closeAllNodes();
@@ -74,8 +74,10 @@ public class IndexAliasesTests extends AbstractNodesTests {
         return client("server2");
     }
 
-
     @Test public void testAliases() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating index [test]");
         client1.admin().indices().create(createIndexRequest("test")).actionGet();
 
@@ -119,7 +121,32 @@ public class IndexAliasesTests extends AbstractNodesTests {
         assertThat(indexResponse.index(), equalTo("test_x"));
     }
 
+    @Test public void testFailedFilter() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
+        logger.info("--> creating index [test]");
+        client1.admin().indices().create(createIndexRequest("test")).actionGet();
+
+        logger.info("--> running cluster_health");
+        ClusterHealthResponse clusterHealth = client1.admin().cluster().health(clusterHealthRequest().waitForGreenStatus()).actionGet();
+        logger.info("--> done cluster_health, status " + clusterHealth.status());
+        assertThat(clusterHealth.timedOut(), equalTo(false));
+        assertThat(clusterHealth.status(), equalTo(ClusterHealthStatus.GREEN));
+
+        try {
+            logger.info("--> aliasing index [test] with [alias1] and filter [t]");
+            client1.admin().indices().prepareAliases().addAlias("test", "alias1", "{ t }").execute().actionGet();
+            assert false;
+        } catch (Exception e) {
+            // all is well
+        }
+    }
+
     @Test public void testFilteringAliases() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating index [test]");
         client1.admin().indices().create(createIndexRequest("test")).actionGet();
 
@@ -143,6 +170,9 @@ public class IndexAliasesTests extends AbstractNodesTests {
     }
 
     @Test public void testSearchingFilteringAliasesSingleIndex() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating index [test]");
         client1.admin().indices().create(createIndexRequest("test")).actionGet();
 
@@ -190,6 +220,9 @@ public class IndexAliasesTests extends AbstractNodesTests {
     }
 
     @Test public void testSearchingFilteringAliasesTwoIndices() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating index [test1]");
         client1.admin().indices().create(createIndexRequest("test1")).actionGet();
 
@@ -258,6 +291,9 @@ public class IndexAliasesTests extends AbstractNodesTests {
     }
 
     @Test public void testSearchingFilteringAliasesMultipleIndices() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating indices");
         client1.admin().indices().create(createIndexRequest("test1")).actionGet();
         client1.admin().indices().create(createIndexRequest("test2")).actionGet();
@@ -325,6 +361,9 @@ public class IndexAliasesTests extends AbstractNodesTests {
     }
 
     @Test public void testDeletingByQueryFilteringAliases() throws Exception {
+        // delete all indices
+        client1.admin().indices().prepareDelete().execute().actionGet();
+
         logger.info("--> creating index [test1]");
         client1.admin().indices().create(createIndexRequest("test1")).actionGet();