Repository: incubator-brooklyn Updated Branches: refs/heads/master 206d78256 -> 2a004d939
Validation of config constraints on policies and enrichers. Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/8ff866b0 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/8ff866b0 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/8ff866b0 Branch: refs/heads/master Commit: 8ff866b0fad456f5c856f59738935e9f765d8959 Parents: 87b4b9b Author: Sam Corbett <[email protected]> Authored: Fri Aug 28 11:37:33 2015 +0100 Committer: Sam Corbett <[email protected]> Committed: Thu Oct 8 11:10:31 2015 +0100 ---------------------------------------------------------------------- .../brooklyn/core/config/BasicConfigKey.java | 1 + .../brooklyn/core/config/ConfigConstraints.java | 75 ++++++++++++++++---- .../brooklyn/core/entity/AbstractEntity.java | 2 - .../core/objs/AbstractEntityAdjunct.java | 2 +- .../core/objs/proxy/InternalPolicyFactory.java | 16 +++-- .../core/config/ConfigKeyConstraintTest.java | 67 ++++++++++++++--- 6 files changed, 130 insertions(+), 33 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java index 239e7dd..9056eac 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java @@ -112,6 +112,7 @@ public class BasicConfigKey<T> implements ConfigKeySelfExtracting<T>, Serializab public Builder<T> inheritance(ConfigInheritance val) { this.inheritance = val; return this; } + @Beta public Builder<T> constraint(Predicate<? super T> constraint) { this.constraint = checkNotNull(constraint, "constraint"); return this; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java ---------------------------------------------------------------------- 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 0ac030f..ce6f39b 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 @@ -22,8 +22,11 @@ package org.apache.brooklyn.core.config; import java.util.List; import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.objs.BrooklynObject; +import org.apache.brooklyn.api.objs.EntityAdjunct; import org.apache.brooklyn.config.ConfigKey; -import org.apache.brooklyn.core.entity.EntityInternal; +import org.apache.brooklyn.core.objs.AbstractEntityAdjunct; +import org.apache.brooklyn.core.objs.BrooklynObjectInternal; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,25 +34,35 @@ import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; -public class ConfigConstraints { +public abstract class ConfigConstraints<T extends BrooklynObject> { public static final Logger LOG = LoggerFactory.getLogger(ConfigConstraints.class); - private final Entity entity; - public ConfigConstraints(Entity e) { - this.entity = e; - } + private final T brooklynObject; /** * Checks all constraints of all config keys available to an entity. */ - public static void assertValid(Entity e) { - Iterable<ConfigKey<?>> violations = new ConfigConstraints(e).getViolations(); + public static void assertValid(Entity entity) { + Iterable<ConfigKey<?>> violations = new EntityConfigConstraints(entity).getViolations(); + if (!Iterables.isEmpty(violations)) { + throw new ConstraintViolationException("ConfigKeys violate constraints: " + violations); + } + } + + public static void assertValid(EntityAdjunct adjunct) { + Iterable<ConfigKey<?>> violations = new EntityAdjunctConstraints(adjunct).getViolations(); if (!Iterables.isEmpty(violations)) { - throw new AssertionError("ConfigKeys violate constraints: " + violations); + throw new ConstraintViolationException("ConfigKeys violate constraints: " + violations); } } + public ConfigConstraints(T brooklynObject) { + this.brooklynObject = brooklynObject; + } + + abstract Iterable<ConfigKey<?>> getBrooklynObjectTypeConfigKeys(); + public boolean isValid() { return Iterables.isEmpty(getViolations()); } @@ -60,12 +73,14 @@ public class ConfigConstraints { @SuppressWarnings("unchecked") private Iterable<ConfigKey<?>> validateAll() { - EntityInternal ei = (EntityInternal) entity; List<ConfigKey<?>> violating = Lists.newArrayList(); + BrooklynObjectInternal.ConfigurationSupportInternal configInternal = getConfigurationSupportInternal(); - for (ConfigKey<?> configKey : getEntityConfigKeys(entity)) { + Iterable<ConfigKey<?>> configKeys = getBrooklynObjectTypeConfigKeys(); + LOG.trace("Checking config keys on {}: {}", getBrooklynObject(), configKeys); + for (ConfigKey<?> configKey : configKeys) { // getRaw returns null if explicitly set and absent if config key was unset. - Object value = ei.config().getRaw(configKey).or(configKey.getDefaultValue()); + Object value = configInternal.getRaw(configKey).or(configKey.getDefaultValue()); if (value == null || value.getClass().isAssignableFrom(configKey.getType())) { // Cast should be safe because the author of the constraint on the config key had to @@ -83,8 +98,40 @@ public class ConfigConstraints { return violating; } - private static Iterable<ConfigKey<?>> getEntityConfigKeys(Entity entity) { - return entity.getEntityType().getConfigKeys(); + private BrooklynObjectInternal.ConfigurationSupportInternal getConfigurationSupportInternal() { + return ((BrooklynObjectInternal) brooklynObject).config(); + } + + protected T getBrooklynObject() { + return brooklynObject; + } + + private static class EntityConfigConstraints extends ConfigConstraints<Entity> { + public EntityConfigConstraints(Entity brooklynObject) { + super(brooklynObject); + } + + @Override + Iterable<ConfigKey<?>> getBrooklynObjectTypeConfigKeys() { + return getBrooklynObject().getEntityType().getConfigKeys(); + } + } + + private static class EntityAdjunctConstraints extends ConfigConstraints<EntityAdjunct> { + public EntityAdjunctConstraints(EntityAdjunct brooklynObject) { + super(brooklynObject); + } + + @Override + Iterable<ConfigKey<?>> getBrooklynObjectTypeConfigKeys() { + return ((AbstractEntityAdjunct) getBrooklynObject()).getAdjunctType().getConfigKeys(); + } + } + + public static class ConstraintViolationException extends RuntimeException { + public ConstraintViolationException(String message) { + super(message); + } } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java index d91b28f..eb919a7 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java @@ -1551,8 +1551,6 @@ public abstract class AbstractEntity extends AbstractBrooklynObject implements E */ protected ToStringHelper toStringHelper() { return Objects.toStringHelper(this).omitNullValues().add("id", getId()); -// make output more concise by suppressing display name -// .add("name", getDisplayName()); } // -------- INITIALIZATION -------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java index 8200ea8..3434471 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractEntityAdjunct.java @@ -426,7 +426,7 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple protected abstract void onChanged(); - protected AdjunctType getAdjunctType() { + public AdjunctType getAdjunctType() { return adjunctType; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java index aaee778..370d6fc 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java @@ -20,13 +20,17 @@ package org.apache.brooklyn.core.objs.proxy; import java.util.Map; +import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; import org.apache.brooklyn.api.mgmt.ManagementContext; +import org.apache.brooklyn.api.objs.BrooklynObject; +import org.apache.brooklyn.api.objs.EntityAdjunct; import org.apache.brooklyn.api.policy.Policy; import org.apache.brooklyn.api.policy.PolicySpec; import org.apache.brooklyn.api.sensor.Enricher; import org.apache.brooklyn.api.sensor.EnricherSpec; import org.apache.brooklyn.api.sensor.Feed; import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.config.ConfigConstraints; import org.apache.brooklyn.core.enricher.AbstractEnricher; import org.apache.brooklyn.core.entity.AbstractEntity; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; @@ -98,15 +102,15 @@ public class InternalPolicyFactory extends InternalFactory { if (spec.getFlags().containsKey("parent")) { throw new IllegalArgumentException("Spec's flags must not contain parent; use spec.parent() instead for "+spec); } - + try { Class<? extends T> clazz = spec.getType(); - + T pol = construct(clazz, spec.getFlags()); - if (spec.getDisplayName()!=null) + if (spec.getDisplayName()!=null) { ((AbstractPolicy)pol).setDisplayName(spec.getDisplayName()); - + } if (spec.getCatalogItemId()!=null) { ((AbstractPolicy)pol).setCatalogItemId(spec.getCatalogItemId()); } @@ -125,6 +129,7 @@ public class InternalPolicyFactory extends InternalFactory { for (Map.Entry<ConfigKey<?>, Object> entry : spec.getConfig().entrySet()) { pol.config().set((ConfigKey)entry.getKey(), entry.getValue()); } + ConfigConstraints.assertValid(pol); ((AbstractPolicy)pol).init(); return pol; @@ -166,6 +171,7 @@ public class InternalPolicyFactory extends InternalFactory { for (Map.Entry<ConfigKey<?>, Object> entry : spec.getConfig().entrySet()) { enricher.config().set((ConfigKey)entry.getKey(), entry.getValue()); } + ConfigConstraints.assertValid(enricher); ((AbstractEnricher)enricher).init(); return enricher; @@ -174,7 +180,7 @@ public class InternalPolicyFactory extends InternalFactory { throw Exceptions.propagate(e); } } - + /** * Constructs a new-style policy (fails if no no-arg constructor). */ http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/8ff866b0/core/src/test/java/org/apache/brooklyn/core/config/ConfigKeyConstraintTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/config/ConfigKeyConstraintTest.java b/core/src/test/java/org/apache/brooklyn/core/config/ConfigKeyConstraintTest.java index 130e3a7..6636944 100644 --- a/core/src/test/java/org/apache/brooklyn/core/config/ConfigKeyConstraintTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/config/ConfigKeyConstraintTest.java @@ -24,17 +24,22 @@ import static org.testng.Assert.fail; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.entity.ImplementedBy; +import org.apache.brooklyn.api.policy.Policy; import org.apache.brooklyn.api.policy.PolicySpec; +import org.apache.brooklyn.api.sensor.EnricherSpec; import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.enricher.AbstractEnricher; +import org.apache.brooklyn.core.policy.AbstractPolicy; import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.core.test.policy.TestPolicy; import org.apache.brooklyn.util.exceptions.Exceptions; +import org.apache.brooklyn.util.net.Networking; import org.testng.annotations.Test; +import org.apache.brooklyn.core.config.ConfigConstraints.ConstraintViolationException; import com.google.common.base.Predicates; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; public class ConfigKeyConstraintTest extends BrooklynAppUnitTestSupport { @@ -46,7 +51,6 @@ public class ConfigKeyConstraintTest extends BrooklynAppUnitTestSupport { .description("Configuration key that must not be null") .constraint(Predicates.notNull()) .build(); - } @ImplementedBy(EntityWithNonNullConstraintWithNonNullDefaultImpl.class) @@ -86,13 +90,30 @@ public class ConfigKeyConstraintTest extends BrooklynAppUnitTestSupport { public static class EntityProvidingDefaultValueForConfigKeyInRangeImpl extends TestEntityImpl implements EntityProvidingDefaultValueForConfigKeyInRange { } + public static class PolicyWithConfigConstraint extends AbstractPolicy{ + public static final ConfigKey<Object> NON_NULL_CONFIG = ConfigKeys.builder(Object.class) + .name("test.policy.non-null") + .description("Configuration key that must not be null") + .constraint(Predicates.notNull()) + .build(); + } + + public static class EnricherWithConfigConstraint extends AbstractEnricher { + public static final ConfigKey<String> PATTERN = ConfigKeys.builder(String.class) + .name("test.enricher.regex") + .description("Must match a valid IPv4 address") + .constraint(Predicates.containsPattern(Networking.VALID_IP_ADDRESS_REGEX)) + .build(); + } + + @Test public void testExceptionWhenEntityHasNullConfig() { try { app.createAndManageChild(EntitySpec.create(EntityWithNonNullConstraint.class)); fail("Expected exception when managing entity with missing config"); } catch (Exception e) { - Throwable t = Exceptions.getFirstThrowableOfType(e, AssertionError.class); + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); assertNotNull(t); } } @@ -114,7 +135,7 @@ public class ConfigKeyConstraintTest extends BrooklynAppUnitTestSupport { app.createAndManageChild(EntitySpec.create(EntityProvidingDefaultValueForConfigKeyInRange.class)); fail("Expected exception when managing entity setting invalid default value"); } catch (Exception e) { - Throwable t = Exceptions.getFirstThrowableOfType(e, AssertionError.class); + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); assertNotNull(t); } } @@ -126,23 +147,47 @@ public class ConfigKeyConstraintTest extends BrooklynAppUnitTestSupport { .configure(EntityWithNonNullConstraintWithNonNullDefault.NON_NULL_WITH_DEFAULT, (Object) null)); fail("Expected exception when config key set to null"); } catch (Exception e) { - Throwable t = Exceptions.getFirstThrowableOfType(e, AssertionError.class); + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); assertNotNull(t); } } + // Test fails because config keys that are not on an object's interfaces cannot be checked automatically. @Test(enabled = false) + public void testExceptionWhenPolicyHasNullForeignConfig() { + Policy p = mgmt.getEntityManager().createPolicy(PolicySpec.create(TestPolicy.class) + .configure(EntityWithNonNullConstraint.NON_NULL_CONFIG, (Object) null)); + try { + ConfigConstraints.assertValid(p); + fail("Expected exception when validating policy with missing config"); + } catch (Exception e) { + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); + assertNotNull(t); + } + } + + @Test public void testExceptionWhenPolicyHasNullConfig() { - app.createAndManageChild(EntitySpec.create(TestEntity.class) - .policy(PolicySpec.create(TestPolicy.class) - .configure(EntityWithNonNullConstraint.NON_NULL_CONFIG, (Object) null))); try { - app.start(ImmutableList.of(app.newSimulatedLocation())); - fail("Expected exception when starting entity with policy with missing config"); + mgmt.getEntityManager().createPolicy(PolicySpec.create(PolicyWithConfigConstraint.class) + .configure(PolicyWithConfigConstraint.NON_NULL_CONFIG, (Object) null)); + fail("Expected exception when creating policy with missing config"); } catch (Exception e) { - Throwable t = Exceptions.getFirstThrowableOfType(e, AssertionError.class); + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); assertNotNull(t); } } + @Test + public void testExceptionWhenEnricherHasInvalidConfig() { + try { + mgmt.getEntityManager().createEnricher(EnricherSpec.create(EnricherWithConfigConstraint.class) + .configure(EnricherWithConfigConstraint.PATTERN, "123.123.256.10")); + fail("Expected exception when creating enricher with invalid config"); + } catch (Exception e) { + Throwable t = Exceptions.getFirstThrowableOfType(e, ConstraintViolationException.class); + assertNotNull(t, "Exception was: " + Exceptions.collapseText(e)); + } + } + }
