Adjuncts shouldn't mutate passed flags The constructor would mutate the passed in map and even keep reference to it, modifying it on the next .configure call.
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/de3b706e Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/de3b706e Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/de3b706e Branch: refs/heads/master Commit: de3b706ed24d69c31752a97a4188215c537d41b9 Parents: f69ade3 Author: Svetoslav Neykov <[email protected]> Authored: Fri Jul 10 18:14:19 2015 +0300 Committer: Svetoslav Neykov <[email protected]> Committed: Thu Oct 22 16:33:19 2015 +0300 ---------------------------------------------------------------------- .../core/objs/AbstractEntityAdjunct.java | 21 ++++---- .../policy/basic/AbstractEntityAdjunctTest.java | 52 ++++++++++++++++++++ 2 files changed, 61 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/de3b706e/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 3fc2839..55b6b16 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 @@ -148,7 +148,7 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple if (entry.getKey() instanceof ConfigKey) { ConfigKey key = (ConfigKey)entry.getKey(); if (adjunctType.getConfigKeys().contains(key)) { - setConfig(key, entry.getValue()); + config().set(key, entry.getValue()); } else { log.warn("Unknown configuration key {} for policy {}; ignoring", key, this); iter.remove(); @@ -161,25 +161,20 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple FlagUtils.setAllConfigKeys(this, bag, false); leftoverProperties.putAll(bag.getUnusedConfig()); - //replace properties _contents_ with leftovers so subclasses see leftovers only - flags.clear(); - flags.putAll(leftoverProperties); - leftoverProperties = flags; - - if (!truth(name) && flags.containsKey("displayName")) { + if (!truth(name) && leftoverProperties.containsKey("displayName")) { //TODO inconsistent with entity and location, where name is legacy and displayName is encouraged! //'displayName' is a legacy way to refer to a policy's name - Preconditions.checkArgument(flags.get("displayName") instanceof CharSequence, "'displayName' property should be a string"); - setDisplayName(flags.remove("displayName").toString()); + Preconditions.checkArgument(leftoverProperties.get("displayName") instanceof CharSequence, "'displayName' property should be a string"); + setDisplayName(leftoverProperties.remove("displayName").toString()); } - // set leftover flags should as config items; particularly useful when these have come from a brooklyn.config map - for (Object flag: flags.keySet()) { + // set leftover leftoverProperties should as config items; particularly useful when these have come from a brooklyn.config map + for (Object flag: leftoverProperties.keySet()) { ConfigKey<Object> key = ConfigKeys.newConfigKey(Object.class, Strings.toString(flag)); if (config().getRaw(key).isPresent()) { log.warn("Config '"+flag+"' on "+this+" conflicts with key already set; ignoring"); } else { - config().set(key, flags.get(flag)); + config().set(key, leftoverProperties.get(flag)); } } @@ -417,6 +412,7 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple return getClass().getCanonicalName(); } + @Override public void setDisplayName(String name) { this.name = name; } @@ -560,6 +556,7 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple return uniqueTag; } + @Override public TagSupport tags() { return new AdjunctTagSupport(); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/de3b706e/core/src/test/java/brooklyn/policy/basic/AbstractEntityAdjunctTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/policy/basic/AbstractEntityAdjunctTest.java b/core/src/test/java/brooklyn/policy/basic/AbstractEntityAdjunctTest.java new file mode 100644 index 0000000..d6d41de --- /dev/null +++ b/core/src/test/java/brooklyn/policy/basic/AbstractEntityAdjunctTest.java @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 brooklyn.policy.basic; + +import java.util.Map; + +import org.apache.brooklyn.api.mgmt.rebind.RebindSupport; +import org.apache.brooklyn.core.objs.AbstractEntityAdjunct; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableMap; + +public class AbstractEntityAdjunctTest { + private static class TestAdjunct extends AbstractEntityAdjunct { + public TestAdjunct(Map<?, ?> properties) { + super(properties); + } + + @Override + public RebindSupport<?> getRebindSupport() { + return null; + } + + @Override + protected void onChanged() { + } + } + + @Test + public void testImmutableFlags() { + //shouldn't try to mutate passed flags map (or previous references to it) + TestAdjunct testAdjunct = new TestAdjunct(ImmutableMap.of("unusedKey", "unusedValue")); + testAdjunct.configure(ImmutableMap.of("finalKey", "finalValue")); + } + +}
