Accumulate validation errors when validating index templates

This commit changes the behavior when validating index templates to
accumulate all validation errors before reporting failure to the user.
This addresses a usability issue when creating index templates.

Closes #12900

core/src/main/java/org/elasticsearch/action/ActionRequestValidationException.java

@@ -19,45 +19,7 @@
 
 package org.elasticsearch.action;
 
-import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.ValidationException;
 
-
+public class ActionRequestValidationException extends ValidationException {
-import java.util.ArrayList;
-import java.util.List;
-
-/**
- *
- */
-public class ActionRequestValidationException extends IllegalArgumentException {
-
-    private final List<String> validationErrors = new ArrayList<>();
-
-    public ActionRequestValidationException() {
-        super("validation failed");
-    }
-
-    public void addValidationError(String error) {
-        validationErrors.add(error);
-    }
-
-    public void addValidationErrors(Iterable<String> errors) {
-        for (String error : errors) {
-            validationErrors.add(error);
-        }
-    }
-
-    public List<String> validationErrors() {
-        return validationErrors;
-    }
-
-    @Override
-    public String getMessage() {
-        StringBuilder sb = new StringBuilder();
-        sb.append("Validation Failed: ");
-        int index = 0;
-        for (String error : validationErrors) {
-            sb.append(++index).append(": ").append(error).append(";");
-        }
-        return sb.toString();
-    }
 }

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

@@ -24,7 +24,6 @@ import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
 import com.google.common.base.Charsets;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
-
 import org.apache.lucene.util.CollectionUtil;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
@@ -46,6 +45,7 @@ import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.inject.Inject;
@@ -60,12 +60,15 @@ import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.index.Index;
+import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MapperParsingException;
 import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.query.IndexQueryParserService;
-import org.elasticsearch.index.IndexService;
+import org.elasticsearch.indices.IndexAlreadyExistsException;
-import org.elasticsearch.indices.*;
+import org.elasticsearch.indices.IndexCreationException;
+import org.elasticsearch.indices.IndicesService;
+import org.elasticsearch.indices.InvalidIndexNameException;
 import org.elasticsearch.script.ScriptService;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.joda.time.DateTime;
@@ -514,6 +517,15 @@ public class MetaDataCreateIndexService extends AbstractComponent {
     }
 
     public void validateIndexSettings(String indexName, Settings settings) throws IndexCreationException {
+        List<String> validationErrors = getIndexSettingsValidationErrors(settings);
+        if (validationErrors.isEmpty() == false) {
+            ValidationException validationException = new ValidationException();
+            validationException.addValidationErrors(validationErrors);
+            throw new IndexCreationException(new Index(indexName), validationException);
+        }
+    }
+
+    List<String> getIndexSettingsValidationErrors(Settings settings) {
         String customPath = settings.get(IndexMetaData.SETTING_DATA_PATH, null);
         List<String> validationErrors = Lists.newArrayList();
         if (customPath != null && env.sharedDataFile() == null) {
@@ -530,22 +542,9 @@ public class MetaDataCreateIndexService extends AbstractComponent {
             validationErrors.add("index must have 1 or more primary shards");
         }
         if (number_of_replicas != null && number_of_replicas < 0) {
-           validationErrors.add("index must have 0 or more replica shards");
+            validationErrors.add("index must have 0 or more replica shards");
         }
-        if (validationErrors.isEmpty() == false) {
+        return validationErrors;
-            throw new IndexCreationException(new Index(indexName),
-                new IllegalArgumentException(getMessage(validationErrors)));
-        }
-    }
-
-    private String getMessage(List<String> validationErrors) {
-        StringBuilder sb = new StringBuilder();
-        sb.append("Validation Failed: ");
-        int index = 0;
-        for (String error : validationErrors) {
-            sb.append(++index).append(": ").append(error).append(";");
-        }
-        return sb.toString();
     }
 
     private static class DefaultIndexTemplateFilter implements IndexTemplateFilter {

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java

@@ -29,12 +29,12 @@ import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.TimeoutClusterStateUpdateTask;
 import org.elasticsearch.common.Priority;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.indices.IndexCreationException;
 import org.elasticsearch.indices.IndexTemplateAlreadyExistsException;
 import org.elasticsearch.indices.IndexTemplateMissingException;
 import org.elasticsearch.indices.InvalidIndexTemplateException;
@@ -179,41 +179,44 @@ public class MetaDataIndexTemplateService extends AbstractComponent {
     }
 
     private void validate(PutRequest request) {
+        List<String> validationErrors = Lists.newArrayList();
         if (request.name.contains(" ")) {
-            throw new InvalidIndexTemplateException(request.name, "name must not contain a space");
+            validationErrors.add("name must not contain a space");
         }
         if (request.name.contains(",")) {
-            throw new InvalidIndexTemplateException(request.name, "name must not contain a ','");
+            validationErrors.add("name must not contain a ','");
         }
         if (request.name.contains("#")) {
-            throw new InvalidIndexTemplateException(request.name, "name must not contain a '#'");
+            validationErrors.add("name must not contain a '#'");
         }
         if (request.name.startsWith("_")) {
-            throw new InvalidIndexTemplateException(request.name, "name must not start with '_'");
+            validationErrors.add("name must not start with '_'");
         }
         if (!request.name.toLowerCase(Locale.ROOT).equals(request.name)) {
-            throw new InvalidIndexTemplateException(request.name, "name must be lower cased");
+            validationErrors.add("name must be lower cased");
         }
         if (request.template.contains(" ")) {
-            throw new InvalidIndexTemplateException(request.name, "template must not contain a space");
+            validationErrors.add("template must not contain a space");
         }
         if (request.template.contains(",")) {
-            throw new InvalidIndexTemplateException(request.name, "template must not contain a ','");
+            validationErrors.add("template must not contain a ','");
         }
         if (request.template.contains("#")) {
-            throw new InvalidIndexTemplateException(request.name, "template must not contain a '#'");
+            validationErrors.add("template must not contain a '#'");
         }
         if (request.template.startsWith("_")) {
-            throw new InvalidIndexTemplateException(request.name, "template must not start with '_'");
+            validationErrors.add("template must not start with '_'");
         }
         if (!Strings.validFileNameExcludingAstrix(request.template)) {
-            throw new InvalidIndexTemplateException(request.name, "template must not container the following characters " + Strings.INVALID_FILENAME_CHARS);
+            validationErrors.add("template must not container the following characters " + Strings.INVALID_FILENAME_CHARS);
         }
 
-        try {
+        List<String> indexSettingsValidation = metaDataCreateIndexService.getIndexSettingsValidationErrors(request.settings);
-            metaDataCreateIndexService.validateIndexSettings(request.name, request.settings);
+        validationErrors.addAll(indexSettingsValidation);
-        } catch (IndexCreationException exception) {
+        if (!validationErrors.isEmpty()) {
-            throw new InvalidIndexTemplateException(request.name, exception.getDetailedMessage());
+            ValidationException validationException = new ValidationException();
+            validationException.addValidationErrors(validationErrors);
+            throw new InvalidIndexTemplateException(request.name, validationException.getMessage());
         }
 
         for (Alias alias : request.aliases) {
@@ -271,7 +274,7 @@ public class MetaDataIndexTemplateService extends AbstractComponent {
             this.mappings.putAll(mappings);
             return this;
         }
-        
+
         public PutRequest aliases(Set<Alias> aliases) {
             this.aliases.addAll(aliases);
             return this;

core/src/main/java/org/elasticsearch/common/ValidationException.java

@@ -0,0 +1,71 @@
+/*
+ * 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.common;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Encapsulates an accumulation of validation errors
+ */
+public class ValidationException extends IllegalArgumentException {
+    private final List<String> validationErrors = new ArrayList<>();
+
+    public ValidationException() {
+        super("validation failed");
+    }
+
+    /**
+     * Add a new validation error to the accumulating validation errors
+     * @param error the error to add
+     */
+    public void addValidationError(String error) {
+        validationErrors.add(error);
+    }
+
+    /**
+     * Add a sequence of validation errors to the accumulating validation errors
+     * @param errors the errors to add
+     */
+    public void addValidationErrors(Iterable<String> errors) {
+        for (String error : errors) {
+            validationErrors.add(error);
+        }
+    }
+
+    /**
+     * Returns the validation errors accumulated
+     * @return
+     */
+    public List<String> validationErrors() {
+        return validationErrors;
+    }
+
+    @Override
+    public String getMessage() {
+        StringBuilder sb = new StringBuilder();
+        sb.append("Validation Failed: ");
+        int index = 0;
+        for (String error : validationErrors) {
+            sb.append(++index).append(": ").append(error).append(";");
+        }
+        return sb.toString();
+    }
+}

core/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java

@@ -33,13 +33,47 @@ import org.elasticsearch.indices.InvalidIndexTemplateException;
 import org.elasticsearch.test.ESTestCase;
 import org.junit.Test;
 
-import java.io.IOException;
 import java.util.List;
 import java.util.Map;
 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.Matchers.contains;
+
 public class MetaDataIndexTemplateServiceTests extends ESTestCase {
     @Test
-    public void testIndexTemplateInvalidNumberOfShards() throws IOException {
+    public void testIndexTemplateInvalidNumberOfShards() {
+        PutRequest request = new PutRequest("test", "test_shards");
+        request.template("test_shards*");
+
+        Map<String, Object> map = Maps.newHashMap();
+        map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0");
+        request.settings(Settings.settingsBuilder().put(map).build());
+
+        List<Throwable> throwables = putTemplate(request);
+        assertEquals(throwables.size(), 1);
+        assertThat(throwables.get(0), instanceOf(InvalidIndexTemplateException.class));
+        assertThat(throwables.get(0).getMessage(), containsString("index must have 1 or more primary shards"));
+    }
+
+    @Test
+    public void testIndexTemplateValidationAccumulatesValidationErrors() {
+        PutRequest request = new PutRequest("test", "putTemplate shards");
+        request.template("_test_shards*");
+
+        Map<String, Object> map = Maps.newHashMap();
+        map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0");
+        request.settings(Settings.settingsBuilder().put(map).build());
+
+        List<Throwable> throwables = putTemplate(request);
+        assertEquals(throwables.size(), 1);
+        assertThat(throwables.get(0), instanceOf(InvalidIndexTemplateException.class));
+        assertThat(throwables.get(0).getMessage(), containsString("name must not contain a space"));
+        assertThat(throwables.get(0).getMessage(), containsString("template must not start with '_'"));
+        assertThat(throwables.get(0).getMessage(), containsString("index must have 1 or more primary shards"));
+    }
+
+    private static List<Throwable> putTemplate(PutRequest request) {
         MetaDataCreateIndexService createIndexService = new MetaDataCreateIndexService(
                 Settings.EMPTY,
                 null,
@@ -55,13 +89,6 @@ public class MetaDataIndexTemplateServiceTests extends ESTestCase {
         );
         MetaDataIndexTemplateService service = new MetaDataIndexTemplateService(Settings.EMPTY, null, createIndexService, null);
 
-        PutRequest request = new PutRequest("test", "test_shards");
-        request.template("test_shards*");
-
-        Map<String, Object> map = Maps.newHashMap();
-        map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0");
-        request.settings(Settings.settingsBuilder().put(map).build());
-
         final List<Throwable> throwables = Lists.newArrayList();
         service.putTemplate(request, new MetaDataIndexTemplateService.PutListener() {
             @Override
@@ -74,8 +101,7 @@ public class MetaDataIndexTemplateServiceTests extends ESTestCase {
                 throwables.add(t);
             }
         });
-        assertEquals(throwables.size(), 1);
+
-        assertTrue(throwables.get(0) instanceof InvalidIndexTemplateException);
+        return throwables;
-        assertTrue(throwables.get(0).getMessage().contains("index must have 1 or more primary shards"));
     }
 }