Move dev-validation-override to validator, and keep all failures and changes

8 files changed, 118 insertions, 63 deletions

diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
index ae2753d47bc..a8aaf0f57ef 100644
--- a/config-model-api/abi-spec.json
+++ b/config-model-api/abi-spec.json
@@ -771,7 +771,9 @@
     "attributes" : [
       "public"
     ],
-    "methods" : [ ],
+    "methods" : [
+      "public java.util.Map messagesById()"
+    ],
     "fields" : [ ]
   },
   "com.yahoo.config.application.api.ValidationOverrides" : {
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
index 1edddb63e52..7b52d825473 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationOverrides.java
@@ -15,6 +15,7 @@ import java.time.format.DateTimeFormatter;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -36,7 +37,7 @@ public class ValidationOverrides {
 
     private final String xmlForm;
 
-    /** Creates a validation overrides which does not have an xml form */
+    /** Creates a validation overrides which does not have an XML form */
     public ValidationOverrides(List<Allow> overrides) {
         this(overrides, null);
     }
@@ -163,10 +164,13 @@ public class ValidationOverrides {
      */
     public static class ValidationException extends IllegalArgumentException {
 
+        private final Map<ValidationId, Collection<String>> messagesById = new LinkedHashMap<>();
+
         static final long serialVersionUID = 789984668;
 
         private ValidationException(ValidationId validationId, String message) {
             super(validationId + ": " + message + ". " + toAllowMessage(validationId));
+            messagesById.put(validationId, List.of(message));
         }
 
         private ValidationException(Map<ValidationId, Collection<String>> messagesById) {
@@ -175,8 +179,11 @@ public class ValidationOverrides {
                                                String.join("\n\t", messages.getValue()) + "\n" +
                                                toAllowMessage(messages.getKey()))
                               .collect(Collectors.joining("\n")));
+            messagesById.forEach((id, messages) -> this.messagesById.put(id, List.copyOf(messages)));
         }
 
+        public Map<ValidationId, Collection<String>> messagesById() { return Map.copyOf(messagesById); }
+
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
index 33dfee58d1a..f19341098f4 100644
--- a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
+++ b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployState.java
@@ -172,7 +172,7 @@ public class DeployState implements ConfigDefinitionStore {
     /** Get the global rank profile registry for this application. */
     public final RankProfileRegistry rankProfileRegistry() { return rankProfileRegistry; }
 
-    /** Returns the validation overrides of this. This is never null */
+    /** Returns the validation overrides of this. This is never null. */
     public ValidationOverrides validationOverrides() { return validationOverrides; }
 
     @Override
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
index 903f1c06024..65572d07cc2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java
@@ -217,13 +217,6 @@ public class VespaModelFactory implements ModelFactory {
     private List<ConfigChangeAction> validateModel(VespaModel model, DeployState deployState, ValidationParameters validationParameters) {
         try {
             return new Validation(additionalValidators).validate(model, validationParameters, deployState);
-        } catch (ValidationOverrides.ValidationException e) {
-            if (deployState.isHosted() && zone.environment().isManuallyDeployed())
-                deployState.getDeployLogger().logApplicationPackage(Level.WARNING,
-                                                                    "Auto-overriding validation which would be disallowed in production: " +
-                                                                    Exceptions.toMessageString(e));
-            else
-                rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors());
         } catch (IllegalArgumentException | TransientException | QuotaExceededException e) {
             rethrowUnlessIgnoreErrors(e, validationParameters.ignoreValidationErrors());
         } catch (Exception e) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
index 56277345515..b9995d290cc 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.model.application.validation;
 
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
+import com.yahoo.config.application.api.ValidationOverrides.ValidationException;
 import com.yahoo.config.model.api.ConfigChangeAction;
 import com.yahoo.config.model.api.Model;
 import com.yahoo.config.model.api.ValidationParameters;
@@ -25,15 +26,19 @@ import com.yahoo.vespa.model.application.validation.change.RestartOnDeployForOnn
 import com.yahoo.vespa.model.application.validation.change.StartupCommandChangeValidator;
 import com.yahoo.vespa.model.application.validation.change.StreamingSearchClusterChangeValidator;
 import com.yahoo.vespa.model.application.validation.first.RedundancyValidator;
+import com.yahoo.yolean.Exceptions;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.stream.Collectors;
 
 import static java.util.stream.Collectors.groupingBy;
@@ -51,7 +56,7 @@ public class Validation {
 
     public Validation() { this(List.of()); }
 
-    /** Create instance taking additional validators (e.g for cloud applications) */
+    /** Create instance taking additional validators (e.g., for cloud applications) */
     public Validation(List<Validator> additionalValidators) { this.additionalValidators = additionalValidators; }
 
     /**
@@ -62,52 +67,53 @@ public class Validation {
      * @throws ValidationOverrides.ValidationException if the change fails validation
      */
     public List<ConfigChangeAction> validate(VespaModel model, ValidationParameters validationParameters, DeployState deployState) {
+        Execution execution = new Execution(model, deployState);
         if (validationParameters.checkRouting()) {
-            new RoutingValidator().validate(model, deployState);
-            new RoutingSelectorValidator().validate(model, deployState);
+            execution.run(new RoutingValidator());
+            execution.run(new RoutingSelectorValidator());
         }
-        new SchemasDirValidator().validate(model, deployState);
-        new BundleValidator().validate(model, deployState);
-        new PublicApiBundleValidator().validate(model, deployState);
-        new SearchDataTypeValidator().validate(model, deployState);
-        new ComplexFieldsWithStructFieldAttributesValidator().validate(model, deployState);
-        new ComplexFieldsWithStructFieldIndexesValidator().validate(model, deployState);
-        new StreamingValidator().validate(model, deployState);
-        new RankSetupValidator(validationParameters.ignoreValidationErrors()).validate(model, deployState);
-        new NoPrefixForIndexes().validate(model, deployState);
-        new ContainerInCloudValidator().validate(model, deployState);
-        new DeploymentSpecValidator().validate(model, deployState);
-        new ValidationOverridesValidator().validate(model, deployState);
-        new ConstantValidator().validate(model, deployState);
-        new SecretStoreValidator().validate(model, deployState);
-        new EndpointCertificateSecretsValidator().validate(model, deployState);
-        new AccessControlFilterValidator().validate(model, deployState);
-        new QuotaValidator().validate(model, deployState);
-        new UriBindingsValidator().validate(model, deployState);
-        new CloudDataPlaneFilterValidator().validate(model, deployState);
-        new AccessControlFilterExcludeValidator().validate(model, deployState);
-        new CloudUserFilterValidator().validate(model, deployState);
-        new CloudHttpConnectorValidator().validate(model, deployState);
-        new UrlConfigValidator().validate(model, deployState);
-        new JvmHeapSizeValidator().validate(model, deployState);
-
-        additionalValidators.forEach(v -> v.validate(model, deployState));
+        execution.run(new SchemasDirValidator());
+        execution.run(new BundleValidator());
+        execution.run(new PublicApiBundleValidator());
+        execution.run(new SearchDataTypeValidator());
+        execution.run(new ComplexFieldsWithStructFieldAttributesValidator());
+        execution.run(new ComplexFieldsWithStructFieldIndexesValidator());
+        execution.run(new StreamingValidator());
+        execution.run(new RankSetupValidator(validationParameters.ignoreValidationErrors()));
+        execution.run(new NoPrefixForIndexes());
+        execution.run(new ContainerInCloudValidator());
+        execution.run(new DeploymentSpecValidator());
+        execution.run(new ValidationOverridesValidator());
+        execution.run(new ConstantValidator());
+        execution.run(new SecretStoreValidator());
+        execution.run(new EndpointCertificateSecretsValidator());
+        execution.run(new AccessControlFilterValidator());
+        execution.run(new QuotaValidator());
+        execution.run(new UriBindingsValidator());
+        execution.run(new CloudDataPlaneFilterValidator());
+        execution.run(new AccessControlFilterExcludeValidator());
+        execution.run(new CloudUserFilterValidator());
+        execution.run(new CloudHttpConnectorValidator());
+        execution.run(new UrlConfigValidator());
+        execution.run(new JvmHeapSizeValidator());
+
+        additionalValidators.forEach(execution::run);
 
         List<ConfigChangeAction> result = Collections.emptyList();
         if (deployState.getProperties().isFirstTimeDeployment()) {
-            validateFirstTimeDeployment(model, deployState);
+            validateFirstTimeDeployment(execution);
         } else {
             Optional<Model> currentActiveModel = deployState.getPreviousModel();
             if (currentActiveModel.isPresent() && (currentActiveModel.get() instanceof VespaModel)) {
-                result = validateChanges((VespaModel) currentActiveModel.get(), model, deployState);
+                result = validateChanges((VespaModel) currentActiveModel.get(), execution);
                 deferConfigChangesForClustersToBeRestarted(result, model);
             }
         }
+        execution.throwIfFailed();
         return result;
     }
 
-    private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, VespaModel nextModel,
-                                                            DeployState deployState) {
+    private static List<ConfigChangeAction> validateChanges(VespaModel currentModel, Execution execution) {
         ChangeValidator[] validators = new ChangeValidator[] {
                 new IndexingModeChangeValidator(),
                 new GlobalDocumentChangeValidator(),
@@ -127,20 +133,15 @@ public class Validation {
                 new RestartOnDeployForOnnxModelChangesValidator(),
         };
         List<ConfigChangeAction> actions = Arrays.stream(validators)
-                                                 .flatMap(v -> v.validate(currentModel, nextModel, deployState).stream())
+                                                 .flatMap(v -> v.validate(currentModel, execution.model, execution.deployState).stream())
                                                  .toList();
 
-        Map<ValidationId, Collection<String>> disallowableActions = actions.stream()
-                                                                           .filter(action -> action.validationId().isPresent())
-                                                                           .collect(groupingBy(action -> action.validationId().orElseThrow(),
-                                                                                               mapping(ConfigChangeAction::getMessage,
-                                                                                                       toCollection(LinkedHashSet::new))));
-        deployState.validationOverrides().invalid(disallowableActions, deployState.now());
+        execution.runChanges(actions);
         return actions;
     }
 
-    private static void validateFirstTimeDeployment(VespaModel model, DeployState deployState) {
-        new RedundancyValidator().validate(model, deployState);
+    private static void validateFirstTimeDeployment(Execution execution) {
+        execution.run(new RedundancyValidator());
     }
 
     private static void deferConfigChangesForClustersToBeRestarted(List<ConfigChangeAction> actions, VespaModel model) {
@@ -159,4 +160,53 @@ public class Validation {
         }
     }
 
+
+    private static class Execution {
+
+        private final Map<ValidationId, List<String>> failures = new LinkedHashMap<>();
+        private final VespaModel model;
+        private final DeployState deployState;
+
+        private Execution(VespaModel model, DeployState deployState) {
+            this.model = model;
+            this.deployState = deployState;
+        }
+
+        private void run(Validator validator) {
+            try {
+                validator.validate(model, deployState);
+            }
+            catch (ValidationException e) {
+                e.messagesById().forEach((id, messages) -> failures.computeIfAbsent(id, __ -> new ArrayList<>()).addAll(messages));
+            }
+        }
+
+        private void runChanges(List<ConfigChangeAction> actions) {
+            for (ConfigChangeAction action : actions) {
+                if (action.validationId().isPresent()) run(new Validator() { // Changes without a validation ID are always allowed.
+                    @Override public void validate(VespaModel model, DeployState deployState) {
+                        deployState.validationOverrides().invalid(action.validationId().get(), action.getMessage(), deployState.now());
+                    }
+                });
+            }
+        }
+
+        private void throwIfFailed() {
+            try {
+                if (failures.size() == 1 && failures.values().iterator().next().size() == 1) // Retain single-form exception message when possible.
+                    deployState.validationOverrides().invalid(failures.keySet().iterator().next(), failures.values().iterator().next().get(0), deployState.now());
+                else
+                    deployState.validationOverrides().invalid(failures, deployState.now());
+            }
+            catch (ValidationException e) {
+                if (deployState.isHosted() && deployState.zone().environment().isManuallyDeployed())
+                    deployState.getDeployLogger().logApplicationPackage(Level.WARNING,
+                                                                        "Auto-overriding validation which would be disallowed in production: " +
+                                                                        Exceptions.toMessageString(e));
+                else throw e;
+            }
+        }
+
+    }
+
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
index fb07f65b5f4..e2dd3aca0b9 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeValidator.java
@@ -40,7 +40,7 @@ public class IndexingScriptChangeValidator {
             String fieldName = nextField.getName();
             ImmutableSDField currentField = currentSchema.getConcreteField(fieldName);
             if (currentField != null) {
-                validateScripts(currentField, nextField).ifPresent(r -> result.add(r));
+                validateScripts(currentField, nextField).ifPresent(result::add);
             }
             else if (nextField.isExtraField()) {
                 result.add(VespaReindexAction.of(id,
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
index 6b7df8871aa..bc36b800bfb 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java
@@ -23,9 +23,11 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 public class CertificateRemovalChangeValidatorTest {
 
     private static final String validationOverrides =
-            "<validation-overrides>\n" +
-            "    <allow until='2000-01-14' comment='test override'>certificate-removal</allow>\n" +
-            "</validation-overrides>\n";
+            """
+            <validation-overrides>
+                <allow until='2000-01-14' comment='test override'>certificate-removal</allow>
+            </validation-overrides>
+            """;
 
     @Test
     void validate() {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
index 1e1406b38c2..9e0eab9aba7 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/IndexingModeChangeValidatorTest.java
@@ -38,13 +38,12 @@ public class IndexingModeChangeValidatorTest {
         VespaModel oldModel =
                 tester.deploy(null, getServices("index"), Environment.prod, "<validation-overrides />").getFirst();
         try {
-            List<ConfigChangeAction> changeActions =
-                    tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond();
+            tester.deploy(oldModel, getServices("streaming"), Environment.prod, "<calidation-overrides />").getSecond();
             fail("Should throw on disallowed config change action");
         }
         catch (ValidationException e) {
-            assertEquals("indexing-mode-change:\n" +
-                    "\tDocument type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'\n" +
+            assertEquals("indexing-mode-change: " +
+                    "Document type 'music' in cluster 'default-content' changed indexing mode from 'indexed' to 'streaming'. " +
                     "To allow this add <allow until='yyyy-mm-dd'>indexing-mode-change</allow> to validation-overrides.xml, see https://docs.vespa.ai/en/reference/validation-overrides.html",
                     e.getMessage());
         }
@@ -104,8 +103,10 @@ public class IndexingModeChangeValidatorTest {
     }
 
     private static final String validationOverrides =
-            "<validation-overrides>\n" +
-            "    <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>\n" +
-            "</validation-overrides>\n";
+            """
+            <validation-overrides>
+                <allow until='2000-01-14' comment='test override'>indexing-mode-change</allow>
+            </validation-overrides>
+            """;
 
 }