You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2017/11/07 12:09:16 UTC
[07/10] brooklyn-server git commit: make deep resolution types and
defaults consistent
make deep resolution types and defaults consistent
(from release notes):
Deep config resolution of nested items has changed to be consistent with when deep config applies.
Deep config applies to maps and collections, but previously any Iterable contained therein
would have a recursive deep config evaluation. Now this is limited to nested Collection types
(Lists, Sets, etc) and Maps; nested Iterable instances that are not also Collections are
no longer traversed and resolved recursively. This makes their nested resolution consistent
with when such instances are non-nested config value, as deep resolution was not applied there.
This mainly affects PortRange, where previously if set directly on a config key it would return
the PortRange value but if accessed in a map such as `shell.env` any non-default value would
be expanded as a list `[1, 2]` (but default values would not be expanded, as per previous point,
but now they are).
Also now returns immutable collections for default values, as it did previously but stopped when we did deep resolution
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/64a8806a
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/64a8806a
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/64a8806a
Branch: refs/heads/master
Commit: 64a8806a607b0523659268f8e180be64cb32d397
Parents: ab13220
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Thu Nov 2 09:53:01 2017 +0000
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Thu Nov 2 09:53:01 2017 +0000
----------------------------------------------------------------------
.../brooklyn/core/config/BasicConfigKey.java | 3 +-
.../config/internal/AbstractConfigMapImpl.java | 20 +++-
.../apache/brooklyn/util/core/task/Tasks.java | 6 +-
.../brooklyn/util/core/task/ValueResolver.java | 98 +++++++++++---------
.../MapListAndOtherStructuredConfigKeyTest.java | 19 ++++
.../brooklyn/util/core/task/TasksTest.java | 7 +-
6 files changed, 98 insertions(+), 55 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/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 61f4874..b2ec95d 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
@@ -36,6 +36,7 @@ import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext;
import org.apache.brooklyn.util.collections.MutableMap;
import org.apache.brooklyn.util.core.internal.ConfigKeySelfExtracting;
import org.apache.brooklyn.util.core.task.Tasks;
+import org.apache.brooklyn.util.core.task.ValueResolver;
import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.guava.TypeTokens;
import org.slf4j.Logger;
@@ -440,7 +441,7 @@ public class BasicConfigKey<T> implements ConfigKeySelfExtracting<T>, Serializab
}
protected Object resolveValue(Object v, ExecutionContext exec) throws ExecutionException, InterruptedException {
- if (v instanceof Collection || v instanceof Map) {
+ if (ValueResolver.supportsDeepResolution(v)) {
return Tasks.resolveDeepValue(v, Object.class, exec, "Resolving deep config "+name);
} else {
return Tasks.resolveValue(v, getType(), exec, "Resolving config "+name);
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
index f044e42..c196f9c 100644
--- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
+++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
@@ -53,6 +53,7 @@ import org.apache.brooklyn.util.core.flags.TypeCoercions;
import org.apache.brooklyn.util.core.internal.ConfigKeySelfExtracting;
import org.apache.brooklyn.util.core.task.DeferredSupplier;
import org.apache.brooklyn.util.core.task.Tasks;
+import org.apache.brooklyn.util.core.task.ValueResolver;
import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.ReferenceWithError;
import org.apache.brooklyn.util.guava.Maybe;
@@ -362,15 +363,24 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i
}
@SuppressWarnings("unchecked")
- protected <T> T coerce(TContainer container, String name, Object value, Class<T> type) {
+ protected <T> T coerceDefaultValue(TContainer container, String name, Object value, Class<T> type) {
if (type==null || value==null) return (T) value;
ExecutionContext exec = getExecutionContext(container);
try {
- if (value instanceof Collection || value instanceof Map) {
- return (T) Tasks.resolveDeepValue(value, Object.class, exec, "Resolving deep default config "+name);
+ T result;
+ if (ValueResolver.supportsDeepResolution(value)) {
+ result = (T) Tasks.resolveDeepValue(value, Object.class, exec, "Resolving deep default config "+name);
} else {
- return Tasks.resolveValue(value, type, exec, "Resolving default config "+name);
+ result = Tasks.resolveValue(value, type, exec, "Resolving default config "+name);
}
+
+ // best effort to preserve/enforce immutability for defaults
+ if (result instanceof Map) return (T) Collections.unmodifiableMap((Map<?,?>)result);
+ if (result instanceof List) return (T) Collections.unmodifiableList((List<?>)result);
+ if (result instanceof Set) return (T) Collections.unmodifiableSet((Set<?>)result);
+ if (result instanceof Collection) return (T) Collections.unmodifiableCollection((Collection<?>)result);
+
+ return result;
} catch (Exception e) {
throw Exceptions.propagate(e);
}
@@ -404,7 +414,7 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i
if (raw || input==null || input.isAbsent()) return (Maybe<T>)input;
// use lambda to defer execution if default value not needed.
// this coercion should never be persisted so this is safe.
- return new MaybeSupplier<T>(() -> (coerce(getContainer(), ownKey.getName(), input.get(), type)));
+ return new MaybeSupplier<T>(() -> (coerceDefaultValue(getContainer(), ownKey.getName(), input.get(), type)));
}
};
// prefer default and type of ownKey
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/core/src/main/java/org/apache/brooklyn/util/core/task/Tasks.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/Tasks.java b/core/src/main/java/org/apache/brooklyn/util/core/task/Tasks.java
index a31c663..790150f 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/task/Tasks.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/task/Tasks.java
@@ -21,8 +21,10 @@ package org.apache.brooklyn.util.core.task;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.Writer;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.CancellationException;
import java.util.concurrent.ExecutionException;
@@ -171,9 +173,9 @@ public class Tasks {
*
* {@code Object result = resolveDeepValue(ImmutableList.of(ImmutableMap.of(1, true)), String.class, exec)}
*
- * To perform a deep conversion of futures contained within Iterables or Maps without coercion of each element,
+ * To perform a deep conversion of futures contained within a {@link Collection} or {@link Map} without coercion of each element,
* the type should normally be Object, not the type of the collection. This differs from
- * {@link #resolveValue(Object, Class, ExecutionContext, String)} which will accept Map and Iterable
+ * {@link #resolveValue(Object, Class, ExecutionContext, String)} which will accept {@link Map} and {@link Collection}
* as the required type.
*/
public static <T> T resolveDeepValue(Object v, Class<T> type, ExecutionContext exec, String contextMessage) throws ExecutionException, InterruptedException {
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java
index 5f7621b..e91693c 100644
--- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java
+++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java
@@ -18,6 +18,7 @@
*/
package org.apache.brooklyn.util.core.task;
+import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -509,49 +510,55 @@ public class ValueResolver<T> implements DeferredSupplier<T>, Iterable<Maybe<Obj
}
}
- } else if (v instanceof Map) {
- //and if a map or list we look inside
- Map result = Maps.newLinkedHashMap();
- for (Map.Entry<?,?> entry : ((Map<?,?>)v).entrySet()) {
- Maybe<?> kk = new ValueResolver(entry.getKey(), type, this)
- .description( (description!=null ? description+", " : "") + "map key "+entry.getKey() )
- .getMaybe();
- if (kk.isAbsent()) return (Maybe<T>)kk;
- Maybe<?> vv = new ValueResolver(entry.getValue(), type, this)
- .description( (description!=null ? description+", " : "") + "map value for key "+kk.get() )
- .getMaybe();
- if (vv.isAbsent()) return (Maybe<T>)vv;
- result.put(kk.get(), vv.get());
- }
- return Maybe.of((T) result);
-
- } else if (v instanceof Set) {
- Set result = Sets.newLinkedHashSet();
- int count = 0;
- for (Object it : (Set)v) {
- Maybe<?> vv = new ValueResolver(it, type, this)
- .description( (description!=null ? description+", " : "") + "entry "+count )
- .getMaybe();
- if (vv.isAbsent()) return (Maybe<T>)vv;
- result.add(vv.get());
- count++;
- }
- return Maybe.of((T) result);
-
- } else if (v instanceof Iterable) {
- List result = Lists.newArrayList();
- int count = 0;
- for (Object it : (Iterable)v) {
- Maybe<?> vv = new ValueResolver(it, type, this)
- .description( (description!=null ? description+", " : "") + "entry "+count )
- .getMaybe();
- if (vv.isAbsent()) return (Maybe<T>)vv;
- result.add(vv.get());
- count++;
- }
- return Maybe.of((T) result);
-
} else {
+ if (supportsDeepResolution(v)) {
+ // restrict deep resolution to the same set of types as calling code;
+ // in particular need to avoid for "interesting iterables" such as PortRange
+
+ if (v instanceof Map) {
+ //and if a map or list we look inside
+ Map result = Maps.newLinkedHashMap();
+ for (Map.Entry<?,?> entry : ((Map<?,?>)v).entrySet()) {
+ Maybe<?> kk = new ValueResolver(entry.getKey(), type, this)
+ .description( (description!=null ? description+", " : "") + "map key "+entry.getKey() )
+ .getMaybe();
+ if (kk.isAbsent()) return (Maybe<T>)kk;
+ Maybe<?> vv = new ValueResolver(entry.getValue(), type, this)
+ .description( (description!=null ? description+", " : "") + "map value for key "+kk.get() )
+ .getMaybe();
+ if (vv.isAbsent()) return (Maybe<T>)vv;
+ result.put(kk.get(), vv.get());
+ }
+ return Maybe.of((T) result);
+
+ } else if (v instanceof Set) {
+ Set result = Sets.newLinkedHashSet();
+ int count = 0;
+ for (Object it : (Set)v) {
+ Maybe<?> vv = new ValueResolver(it, type, this)
+ .description( (description!=null ? description+", " : "") + "entry "+count )
+ .getMaybe();
+ if (vv.isAbsent()) return (Maybe<T>)vv;
+ result.add(vv.get());
+ count++;
+ }
+ return Maybe.of((T) result);
+
+ } else if (v instanceof Iterable) {
+ List result = Lists.newArrayList();
+ int count = 0;
+ for (Object it : (Iterable)v) {
+ Maybe<?> vv = new ValueResolver(it, type, this)
+ .description( (description!=null ? description+", " : "") + "entry "+count )
+ .getMaybe();
+ if (vv.isAbsent()) return (Maybe<T>)vv;
+ result.add(vv.get());
+ count++;
+ }
+ return Maybe.of((T) result);
+ }
+ }
+
return TypeCoercions.tryCoerce(v, TypeToken.of(type));
}
@@ -574,11 +581,16 @@ public class ValueResolver<T> implements DeferredSupplier<T>, Iterable<Maybe<Obj
if (recursive) {
return new ValueResolver(v, type, this).getMaybe();
} else {
- // T expected to be Object.class
return (Maybe<T>) Maybe.of(v);
}
}
+ // whether value resolution supports deep resolution
+ @Beta
+ public static boolean supportsDeepResolution(Object v) {
+ return (v instanceof Map || v instanceof Collection);
+ }
+
protected String getDescription() {
return description!=null ? description : ""+value;
}
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/core/src/test/java/org/apache/brooklyn/core/config/MapListAndOtherStructuredConfigKeyTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/core/config/MapListAndOtherStructuredConfigKeyTest.java b/core/src/test/java/org/apache/brooklyn/core/config/MapListAndOtherStructuredConfigKeyTest.java
index ee6cb51..58a03a2 100644
--- a/core/src/test/java/org/apache/brooklyn/core/config/MapListAndOtherStructuredConfigKeyTest.java
+++ b/core/src/test/java/org/apache/brooklyn/core/config/MapListAndOtherStructuredConfigKeyTest.java
@@ -29,11 +29,13 @@ 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.location.PortRange;
import org.apache.brooklyn.config.ConfigKey;
import org.apache.brooklyn.core.config.ListConfigKey.ListModifications;
import org.apache.brooklyn.core.config.MapConfigKey.MapModifications;
import org.apache.brooklyn.core.config.SetConfigKey.SetModifications;
import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.location.PortRanges;
import org.apache.brooklyn.core.location.SimulatedLocation;
import org.apache.brooklyn.core.sensor.DependentConfiguration;
import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport;
@@ -42,7 +44,9 @@ import org.apache.brooklyn.core.test.entity.TestEntity;
import org.apache.brooklyn.util.collections.MutableList;
import org.apache.brooklyn.util.collections.MutableMap;
import org.apache.brooklyn.util.core.task.DeferredSupplier;
+import org.apache.brooklyn.util.core.task.ValueResolver;
import org.apache.brooklyn.util.exceptions.Exceptions;
+import org.testng.Assert;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
@@ -390,4 +394,19 @@ public class MapListAndOtherStructuredConfigKeyTest extends BrooklynAppUnitTestS
ImmutableMap.of("0key", 0, "akey", ImmutableMap.of("aa","AA","a2","A2","a3",3), "bkey", ImmutableList.of("b","b2"), "ckey", "cc"));
assertEquals(entity.getConfig(TestEntity.CONF_MAP_OBJ_THING.subKey("akey")), ImmutableMap.of("aa", "AA", "a2", "A2", "a3", 3));
}
+
+ @Test
+ public void testInterestingIterableConfig() throws Exception {
+ PortRange r12 = PortRanges.fromString("1-2");
+ // previously VR would act on iterables such as PRs inside maps and lists, but not on those at top level, so this test failed
+ Assert.assertFalse(ValueResolver.supportsDeepResolution(r12));
+
+ entity.config().set(TestEntity.CONF_OBJECT, r12);
+ entity.config().set(TestEntity.CONF_MAP_OBJ_THING.subKey("r"), r12);
+ app.start(locs);
+ assertEquals(entity.getConfig(TestEntity.CONF_OBJECT), r12);
+ assertEquals(entity.getConfig(TestEntity.CONF_MAP_OBJ_THING), MutableMap.of("r", r12));
+ }
+
+
}
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/64a8806a/core/src/test/java/org/apache/brooklyn/util/core/task/TasksTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/brooklyn/util/core/task/TasksTest.java b/core/src/test/java/org/apache/brooklyn/util/core/task/TasksTest.java
index 8c656a3..6b4a795 100644
--- a/core/src/test/java/org/apache/brooklyn/util/core/task/TasksTest.java
+++ b/core/src/test/java/org/apache/brooklyn/util/core/task/TasksTest.java
@@ -24,6 +24,7 @@ import static org.testng.Assert.assertTrue;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
@@ -48,7 +49,6 @@ import org.testng.annotations.Test;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
import com.google.common.util.concurrent.Callables;
@@ -103,12 +103,11 @@ public class TasksTest extends BrooklynAppUnitTestSupport {
assertResolvesValue(orig, String.class, expected);
}
- @SuppressWarnings("unchecked")
@Test
- public void testResolvesIterableOfMapsWithAttributeWhenReady() throws Exception {
+ public void testResolvesListOfMapsWithAttributeWhenReady() throws Exception {
app.sensors().set(TestApplication.MY_ATTRIBUTE, "myval");
// using Iterables.concat so that orig is of type FluentIterable rather than List etc
- Iterable<?> orig = Iterables.concat(ImmutableList.of(ImmutableMap.of("mykey", attributeWhenReady(app, TestApplication.MY_ATTRIBUTE))));
+ List<?> orig = ImmutableList.copyOf(ImmutableList.of(ImmutableMap.of("mykey", attributeWhenReady(app, TestApplication.MY_ATTRIBUTE))));
Iterable<Map<?,?>> expected = ImmutableList.<Map<?,?>>of(ImmutableMap.of("mykey", "myval"));
assertResolvesValue(orig, String.class, expected);
}