From 4a5040bb32355519b3c310f9a6bb5ec5ebcb0016 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Fri, 14 Aug 2015 15:44:10 -0600 Subject: [PATCH] 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 --- .../ActionRequestValidationException.java | 42 +---------- .../metadata/MetaDataCreateIndexService.java | 35 +++++---- .../MetaDataIndexTemplateService.java | 35 ++++----- .../common/ValidationException.java | 71 +++++++++++++++++++ .../MetaDataIndexTemplateServiceTests.java | 50 +++++++++---- 5 files changed, 147 insertions(+), 86 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/common/ValidationException.java diff --git a/core/src/main/java/org/elasticsearch/action/ActionRequestValidationException.java b/core/src/main/java/org/elasticsearch/action/ActionRequestValidationException.java index d41900c27af..a1d0869e59d 100644 --- a/core/src/main/java/org/elasticsearch/action/ActionRequestValidationException.java +++ b/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; - -import java.util.ArrayList; -import java.util.List; - -/** - * - */ -public class ActionRequestValidationException extends IllegalArgumentException { - - private final List validationErrors = new ArrayList<>(); - - public ActionRequestValidationException() { - super("validation failed"); - } - - public void addValidationError(String error) { - validationErrors.add(error); - } - - public void addValidationErrors(Iterable errors) { - for (String error : errors) { - validationErrors.add(error); - } - } - - public List 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(); - } +public class ActionRequestValidationException extends ValidationException { } diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 2571e29d8af..ff878a4f721 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/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.*; +import org.elasticsearch.indices.IndexAlreadyExistsException; +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 validationErrors = getIndexSettingsValidationErrors(settings); + if (validationErrors.isEmpty() == false) { + ValidationException validationException = new ValidationException(); + validationException.addValidationErrors(validationErrors); + throw new IndexCreationException(new Index(indexName), validationException); + } + } + + List getIndexSettingsValidationErrors(Settings settings) { String customPath = settings.get(IndexMetaData.SETTING_DATA_PATH, null); List 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) { - throw new IndexCreationException(new Index(indexName), - new IllegalArgumentException(getMessage(validationErrors))); - } - } - - private String getMessage(List 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(); + return validationErrors; } private static class DefaultIndexTemplateFilter implements IndexTemplateFilter { diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java index 96f79155955..66ad39f9225 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexTemplateService.java +++ b/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 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 { - metaDataCreateIndexService.validateIndexSettings(request.name, request.settings); - } catch (IndexCreationException exception) { - throw new InvalidIndexTemplateException(request.name, exception.getDetailedMessage()); + List indexSettingsValidation = metaDataCreateIndexService.getIndexSettingsValidationErrors(request.settings); + validationErrors.addAll(indexSettingsValidation); + if (!validationErrors.isEmpty()) { + 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 aliases) { this.aliases.addAll(aliases); return this; diff --git a/core/src/main/java/org/elasticsearch/common/ValidationException.java b/core/src/main/java/org/elasticsearch/common/ValidationException.java new file mode 100644 index 00000000000..13288765396 --- /dev/null +++ b/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 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 errors) { + for (String error : errors) { + validationErrors.add(error); + } + } + + /** + * Returns the validation errors accumulated + * @return + */ + public List 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(); + } +} diff --git a/core/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java b/core/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java index 197a6b3b351..0966212fe3a 100644 --- a/core/src/test/java/org/elasticsearch/action/admin/indices/template/put/MetaDataIndexTemplateServiceTests.java +++ b/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 map = Maps.newHashMap(); + map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0"); + request.settings(Settings.settingsBuilder().put(map).build()); + + List 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 map = Maps.newHashMap(); + map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0"); + request.settings(Settings.settingsBuilder().put(map).build()); + + List 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 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 map = Maps.newHashMap(); - map.put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, "0"); - request.settings(Settings.settingsBuilder().put(map).build()); - final List 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); - assertTrue(throwables.get(0).getMessage().contains("index must have 1 or more primary shards")); + + return throwables; } }