This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit ed825602a11b792d9f4363d166f35bf496c40c64 Author: Alex Heneveld <[email protected]> AuthorDate: Tue Mar 23 13:21:35 2021 +0000 better detection of config constraint violations --- .../brooklyn/core/config/ConfigConstraints.java | 29 +++++++++++++--------- .../core/config/ConstraintViolationException.java | 2 +- .../brooklyn/core/mgmt/DeployFailureTest.java | 5 ++-- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java b/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java index e4d353f..2d92c45 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java @@ -158,19 +158,24 @@ public abstract class ConfigConstraints<T> { Iterable<ConfigKey<?>> configKeys = getConfigKeys(); LOG.trace("Checking config keys on {}: {}", getSource(), configKeys); for (ConfigKey<?> configKey : configKeys) { - Maybe<?> maybeValue = getValue(configKey); - if (maybeValue.isPresent()) { - // Cast is safe because the author of the constraint on the config key had to - // keep its type to Predicate<? super T>, where T is ConfigKey<T>. - ReferenceWithError<?> validation = validateValue((ConfigKey<Object>) configKey, maybeValue.get()); - if (validation.hasError()) { - violating.put(configKey, validation.getError()); + try { + Maybe<?> maybeValue = getValue(configKey); + if (maybeValue.isPresent()) { + // Cast is safe because the author of the constraint on the config key had to + // keep its type to Predicate<? super T>, where T is ConfigKey<T>. + ReferenceWithError<?> validation = validateValue((ConfigKey<Object>) configKey, maybeValue.get()); + if (validation.hasError()) { + violating.put(configKey, validation.getError()); + } + } else { + // absent means did not resolve in time or not coercible; + // code will return `Maybe.of(null)` if it is unset, + // and coercion errors are handled when the value is _set_ or _needed_ + // (this allows us to deal with edge cases where we can't *immediately* coerce) } - } else { - // absent means did not resolve in time or not coercible; - // code will return `Maybe.of(null)` if it is unset, - // and coercion errors are handled when the value is _set_ or _needed_ - // (this allows us to deal with edge cases where we can't *immediately* coerce) + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + violating.put(configKey, e); } } return violating; diff --git a/core/src/main/java/org/apache/brooklyn/core/config/ConstraintViolationException.java b/core/src/main/java/org/apache/brooklyn/core/config/ConstraintViolationException.java index 5ad490a..d99acc5 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/ConstraintViolationException.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/ConstraintViolationException.java @@ -168,7 +168,7 @@ public class ConstraintViolationException extends UserFacingException { Set<ConfigKey<?>> keys = MutableSet.copyOf(getConfigKeys(cves)).putAll(violations.keySet()); if (keys.size() > 1) { - message.append("Invalid values for " + keys.stream().map(ConfigKey::getName) + ": "); + message.append("Invalid values for " + keys.stream().map(ConfigKey::getName).collect(Collectors.toSet()) + ": "); } if (!violations.isEmpty()) { message.append(violations.values().stream().map(Throwable::toString).collect(Collectors.toSet())); diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java index b9b5bb0..281b148 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/DeployFailureTest.java @@ -106,9 +106,8 @@ public class DeployFailureTest extends BrooklynAppUnitTestSupport { .child(EntitySpec.create(TestEntity.class) .impl(TestEntityFailingGetParentImpl.class))); Asserts.shouldHaveFailedPreviously(); - } catch (ClassCastException e) { - // TODO Should give nicer exception - Asserts.expectedFailureContains(e, "cannot be cast", "WrongParentEntity"); + } catch (ConstraintViolationException e) { + Asserts.expectedFailureContains(e, "cannot be cast", "WrongParentEntity", TestEntityFailingGetParentImpl.class.getSimpleName()); } // This should continue to work, after the failed entity-deploy above
