From 70d005c93acaf225e969552ffb8239200266a402 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Mon, 23 May 2016 21:08:08 +0100 Subject: [PATCH] BROOKLYN-278: fix Propagator validation/defaults --- .../brooklyn/enricher/stock/Propagator.java | 124 ++++++++++++------ .../stock/SensorPropagatingEnricherTest.java | 99 +++++++++++++- .../stock/TransformingEnricherTest.java | 15 +++ 3 files changed, 196 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java b/core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java index d77b19966b..5e0bc8ac75 100644 --- a/core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java +++ b/core/src/main/java/org/apache/brooklyn/enricher/stock/Propagator.java @@ -19,6 +19,7 @@ package org.apache.brooklyn.enricher.stock; import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.Set; @@ -32,6 +33,7 @@ import org.apache.brooklyn.core.config.ConfigKeys; import org.apache.brooklyn.core.enricher.AbstractEnricher; import org.apache.brooklyn.core.entity.Attributes; +import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.flags.SetFromFlag; import org.apache.brooklyn.util.core.sensor.SensorPredicates; @@ -40,6 +42,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Objects; import com.google.common.base.Optional; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; @@ -47,6 +50,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import com.google.common.reflect.TypeToken; @SuppressWarnings("serial") @@ -63,7 +67,9 @@ public class Propagator extends AbstractEnricher implements SensorEventListener< public static ConfigKey PRODUCER = ConfigKeys.newConfigKey(Entity.class, "enricher.producer"); @SetFromFlag("propagatingAllBut") - public static ConfigKey>> PROPAGATING_ALL_BUT = ConfigKeys.newConfigKey(new TypeToken>>() {}, "enricher.propagating.propagatingAllBut"); + public static ConfigKey>> PROPAGATING_ALL_BUT = ConfigKeys.newConfigKey( + new TypeToken>>() {}, + "enricher.propagating.propagatingAllBut"); @SetFromFlag("propagatingAll") public static ConfigKey PROPAGATING_ALL = ConfigKeys.newBooleanConfigKey("enricher.propagating.propagatingAll"); @@ -75,10 +81,10 @@ public class Propagator extends AbstractEnricher implements SensorEventListener< public static ConfigKey, ? extends Sensor>> SENSOR_MAPPING = ConfigKeys.newConfigKey(new TypeToken, ? extends Sensor>>() {}, "enricher.propagating.sensorMapping"); protected Entity producer; - protected Map, ? extends Sensor> sensorMapping; + protected Map, Sensor> sensorMapping; protected boolean propagatingAll; protected Collection> propagatingAllBut; - protected Predicate> sensorFilter; + protected Predicate> sensorFilter; public Propagator() { } @@ -87,24 +93,41 @@ public Propagator() { public void setEntity(EntityLocal entity) { super.setEntity(entity); - this.producer = getConfig(PRODUCER) == null ? entity : getConfig(PRODUCER); - boolean sensorMappingSet = getConfig(SENSOR_MAPPING)!=null; - MutableMap,Sensor> sensorMappingTemp = MutableMap.copyOf(getConfig(SENSOR_MAPPING)); - this.propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || getConfig(PROPAGATING_ALL_BUT)!=null; + producer = getConfig(PRODUCER); + sensorMapping = resolveSensorMappings(getConfig(SENSOR_MAPPING)); + propagatingAllBut = resolveSensorCollection(getConfig(PROPAGATING_ALL_BUT)); + propagatingAll = Boolean.TRUE.equals(getConfig(PROPAGATING_ALL)) || propagatingAllBut.size() > 0; + Collection> propagating = resolveSensorCollection(getConfig(PROPAGATING)); - if (getConfig(PROPAGATING) != null) { + if (producer == null) { + throw new IllegalStateException("Propagator enricher "+this+" missing config '"+PRODUCER.getName()); + } + if (propagating.isEmpty() && sensorMapping.isEmpty() && !propagatingAll) { + throw new IllegalStateException("Propagator enricher "+this+" must have 'propagating' and/or 'sensorMapping', or 'propagatingAll' or 'propagatingAllBut' set"); + } + if (entity.equals(producer)) { if (propagatingAll) { - throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' set at same time as either 'propagatingAll' or 'propagatingAllBut'"); + throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING_ALL.getName()+" or "+PROPAGATING_ALL_BUT.getName()+", when publishing to own entity (to avoid infinite loop)"); + } else if (propagating.size() > 0) { + throw new IllegalStateException("Propagator enricher "+this+" must not have "+PROPAGATING.getName()+", when publishing to own entity (to avoid infinite loop)"); + } else if (filterForKeyEqualsValue(sensorMapping).size() > 0) { + Map, ? extends Sensor> selfPublishingSensors = filterForKeyEqualsValue(sensorMapping); + throw new IllegalStateException("Propagator enricher "+this+" must not publish to same sensor in config "+SENSOR_MAPPING.getName()+" ("+selfPublishingSensors.keySet()+"), when publishing to own entity (to avoid infinite loop)"); } - - for (Object sensorO : getConfig(PROPAGATING)) { - Sensor sensor = Tasks.resolving(sensorO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get(); - if (!sensorMappingTemp.containsKey(sensor)) { - sensorMappingTemp.put(sensor, sensor); + } + if ((propagating.size() > 0 || sensorMapping.size() > 0) && propagatingAll) { + throw new IllegalStateException("Propagator enricher "+this+" must not have 'propagating' or 'sensorMapping' set at same time as either 'propagatingAll' or 'propagatingAllBut'"); + } + + if (propagating.size() > 0) { + for (Sensor sensor : propagating) { + if (!sensorMapping.containsKey(sensor)) { + sensorMapping.put(sensor, sensor); } } - this.sensorMapping = ImmutableMap.copyOf(sensorMappingTemp); - this.sensorFilter = new Predicate>() { + sensorMapping = ImmutableMap.copyOf(sensorMapping); + sensorFilter = Predicates.alwaysTrue(); + new Predicate>() { @Override public boolean apply(Sensor input) { // TODO kept for deserialization of inner classes, but shouldn't be necessary, as with other inner classes (qv); // NB: previously this did this check: @@ -113,33 +136,16 @@ public void setEntity(EntityLocal entity) { return true; } }; - } else if (sensorMappingSet) { - if (propagatingAll) { - throw new IllegalStateException("Propagator enricher "+this+" must not have 'sensorMapping' set at same time as either 'propagatingAll' or 'propagatingAllBut'"); - } - this.sensorMapping = ImmutableMap.copyOf(sensorMappingTemp); - this.sensorFilter = Predicates.alwaysTrue(); + } else if (sensorMapping.size() > 0) { + sensorMapping = ImmutableMap.copyOf(sensorMapping); + sensorFilter = Predicates.alwaysTrue(); } else { - this.sensorMapping = ImmutableMap., Sensor>of(); - if (!propagatingAll) { - // default if nothing specified is to do all but the ones not usually propagated - propagatingAll = true; - // user specified nothing, so *set* the all_but to the default set - // if desired, we could allow this to be dynamically reconfigurable, remove this field and always look up; - // slight performance hit (always looking up), and might need to recompute subscriptions, so not supported currently - // TODO this default is @Beta behaviour! -- maybe better to throw? - propagatingAllBut = SENSORS_NOT_USUALLY_PROPAGATED; - } else { - propagatingAllBut = getConfig(PROPAGATING_ALL_BUT); - } - this.sensorFilter = new Predicate>() { + Preconditions.checkState(propagatingAll, "Impossible case: propagatingAll=%s; propagating=%s; " + + "sensorMapping=%s", propagatingAll, propagating, sensorMapping); + sensorMapping = ImmutableMap.of(); + sensorFilter = new Predicate>() { @Override public boolean apply(Sensor input) { - Collection> exclusions = propagatingAllBut; - // TODO this anonymous inner class and getConfig check kept should be removed / confirmed for rebind compatibility. - // we *should* be regenerating these fields on each rebind (calling to this method), - // so serialization of this class shouldn't be needed (and should be skipped), but that needs to be checked. - if (propagatingAllBut==null) exclusions = getConfig(PROPAGATING_ALL_BUT); - return input != null && (exclusions==null || !exclusions.contains(input)); + return input != null && !propagatingAllBut.contains(input); } }; } @@ -205,4 +211,40 @@ private Sensor getDestinationSensor(final Sensor sourceSensor) { return mappingSensor.isPresent() ? sensorMapping.get(mappingSensor.get()) : sourceSensor; } + private Map, Sensor> resolveSensorMappings(Map mapping) { + if (mapping == null) { + return MutableMap.of(); + } + Map, Sensor> result = MutableMap.of(); + for (Map.Entry entry : mapping.entrySet()) { + Object keyO = entry.getKey(); + Object valueO = entry.getValue(); + Sensor key = Tasks.resolving(keyO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get(); + Sensor value = Tasks.resolving(valueO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get(); + result.put(key, value); + } + return result; + } + + private List> resolveSensorCollection(Iterable sensors) { + if (sensors == null) { + return MutableList.of(); + } + List> result = MutableList.of(); + for (Object sensorO : sensors) { + Sensor sensor = Tasks.resolving(sensorO).as(Sensor.class).timeout(ValueResolver.REAL_QUICK_WAIT).context(producer).get(); + result.add(sensor); + } + return result; + } + + private Map filterForKeyEqualsValue(Map map) { + Map result = Maps.newLinkedHashMap(); + for (Map.Entry entry : map.entrySet()) { + if (Objects.equal(entry.getKey(), entry.getValue())) { + result.put(entry.getKey(), entry.getValue()); + } + } + return result; + } } diff --git a/core/src/test/java/org/apache/brooklyn/enricher/stock/SensorPropagatingEnricherTest.java b/core/src/test/java/org/apache/brooklyn/enricher/stock/SensorPropagatingEnricherTest.java index 42eeda797c..fdbba94d7c 100644 --- a/core/src/test/java/org/apache/brooklyn/enricher/stock/SensorPropagatingEnricherTest.java +++ b/core/src/test/java/org/apache/brooklyn/enricher/stock/SensorPropagatingEnricherTest.java @@ -18,8 +18,12 @@ */ package org.apache.brooklyn.enricher.stock; +import static org.testng.Assert.assertEquals; + +import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.sensor.AttributeSensor; import org.apache.brooklyn.api.sensor.EnricherSpec; @@ -34,12 +38,14 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.javalang.AtomicReferences; +import org.apache.brooklyn.util.time.Duration; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; public class SensorPropagatingEnricherTest extends BrooklynAppUnitTestSupport { @@ -108,7 +114,7 @@ public void testPropagatesAllStaticSensors() { @Test public void testPropagatesAllSensorsIncludesDynamicallyAdded() { AttributeSensor dynamicAttribute = Sensors.newStringSensor("test.dynamicsensor.strattrib"); - BasicNotificationSensor dynamicNotificationSensor = new BasicNotificationSensor(String.class, "test.dynamicsensor.strnotif"); + BasicNotificationSensor dynamicNotificationSensor = new BasicNotificationSensor(String.class, "test.dynamicsensor.strnotif"); app.enrichers().add(Enrichers.builder() .propagatingAll() @@ -265,4 +271,95 @@ public void testPropagateToDynamicSensor() { entity.sensors().set(TestEntity.NAME, "newName"); EntityAsserts.assertAttributeEqualsEventually(app, targetSensor, "newName"); } + + @Test + public void testPropagatorAvoidsInfiniteLoopInPropagateAll() throws Exception { + AttributeSensor mySensor = Sensors.newSensor(String.class, "mySensor"); + + EnricherSpec spec = EnricherSpec.create(Propagator.class) + .configure(Propagator.PRODUCER, app) + .configure(Propagator.PROPAGATING_ALL, true); + + assertAddEnricherThrowsIllegalStateException(spec, "when publishing to own entity"); + assertAttributeNotRepublished(app, mySensor); + } + + @Test + public void testPropagatorAvoidsInfiniteLoopInPropagateAllBut() throws Exception { + AttributeSensor mySensor = Sensors.newSensor(String.class, "mySensor"); + AttributeSensor mySensor2 = Sensors.newSensor(String.class, "mySensor2"); + + EnricherSpec spec = EnricherSpec.create(Propagator.class) + .configure(Propagator.PRODUCER, app) + .configure(Propagator.PROPAGATING_ALL_BUT, ImmutableList.of(mySensor2)); + + assertAddEnricherThrowsIllegalStateException(spec, "when publishing to own entity"); + assertAttributeNotRepublished(app, mySensor); + } + + @Test + public void testPropagatorAvoidsInfiniteLoopInPropagate() throws Exception { + AttributeSensor mySensor = Sensors.newSensor(String.class, "mySensor"); + + EnricherSpec spec = EnricherSpec.create(Propagator.class) + .configure(Propagator.PRODUCER, app) + .configure(Propagator.PROPAGATING, ImmutableList.of(mySensor)); + + assertAddEnricherThrowsIllegalStateException(spec, "when publishing to own entity"); + assertAttributeNotRepublished(app, mySensor); + } + + @Test + public void testPropagatorAvoidsInfiniteLoopInSameSensorMapping() throws Exception { + AttributeSensor mySensor = Sensors.newSensor(String.class, "mySensor"); + + EnricherSpec spec = EnricherSpec.create(Propagator.class) + .configure(Propagator.PRODUCER, app) + .configure(Propagator.SENSOR_MAPPING, ImmutableMap.of(mySensor, mySensor)); + + assertAddEnricherThrowsIllegalStateException(spec, "when publishing to own entity"); + assertAttributeNotRepublished(app, mySensor); + } + + @Test + public void testPropagatorFailsWithEmptyConfig() throws Exception { + EnricherSpec spec = EnricherSpec.create(Propagator.class); + + assertAddEnricherThrowsIllegalStateException(spec, "missing config"); + } + + protected void assertAttributeNotRepublished(Entity entity, AttributeSensor sensor) { + final List> events = Lists.newCopyOnWriteArrayList(); + app.subscriptions().subscribe(entity, sensor, new SensorEventListener() { + @Override public void onEvent(SensorEvent event) { + events.add(event); + }}); + + app.sensors().set(sensor, "myval"); + assertSizeEventually(events, 1); + assertSizeContinually(events, 1, Duration.millis(100)); + } + + protected void assertSizeEventually(final List actual, final int expected) { + Asserts.succeedsEventually(new Runnable() { + @Override public void run() { + assertEquals(actual.size(), expected, "actual="+actual); + }}); + } + + protected void assertSizeContinually(final List actual, final int expected, Duration duration) { + Asserts.succeedsContinually(ImmutableMap.of("timeout", duration), new Runnable() { + @Override public void run() { + assertEquals(actual.size(), expected, "actual="+actual); + }}); + } + + private void assertAddEnricherThrowsIllegalStateException(EnricherSpec spec, String expectedPhrase) { + try { + app.enrichers().add(spec); + Asserts.shouldHaveFailedPreviously(); + } catch (IllegalStateException e) { + Asserts.expectedFailureContains(e, expectedPhrase); + } + } } diff --git a/core/src/test/java/org/apache/brooklyn/enricher/stock/TransformingEnricherTest.java b/core/src/test/java/org/apache/brooklyn/enricher/stock/TransformingEnricherTest.java index 7481fa8bf3..e47d268877 100644 --- a/core/src/test/java/org/apache/brooklyn/enricher/stock/TransformingEnricherTest.java +++ b/core/src/test/java/org/apache/brooklyn/enricher/stock/TransformingEnricherTest.java @@ -224,4 +224,19 @@ public void testInfersTargetSensorSameAsSource() throws Exception { EntityAsserts.assertAttributeEqualsEventually(app, intSensorA, 3); } + + public void testTransformerFailsWithEmptyConfig() throws Exception { + EnricherSpec spec = EnricherSpec.create(Transformer.class); + + assertAddEnricherThrowsNullPointerException(spec, "Value required"); + } + + private void assertAddEnricherThrowsNullPointerException(EnricherSpec spec, String expectedPhrase) { + try { + app.enrichers().add(spec); + Asserts.shouldHaveFailedPreviously(); + } catch (NullPointerException e) { + Asserts.expectedFailureContains(e, expectedPhrase); + } + } }