Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/173#discussion_r65536289
--- Diff:
core/src/main/java/org/apache/brooklyn/core/entity/internal/EntityConfigMap.java
---
@@ -111,47 +110,107 @@ public EntityConfigMap(AbstractEntity entity,
Map<ConfigKey<?>, Object> storage)
// Don't use groovy truth: if the set value is e.g. 0, then would
ignore set value and return default!
if (ownKey instanceof ConfigKeySelfExtracting) {
Object rawval = ownConfig.get(key);
- T result = null;
- boolean complete = false;
- if (((ConfigKeySelfExtracting<T>)ownKey).isSet(ownConfig)) {
- ExecutionContext exec = entity.getExecutionContext();
- result =
((ConfigKeySelfExtracting<T>)ownKey).extractValue(ownConfig, exec);
- complete = true;
- } else if (isInherited(ownKey, inheritance) &&
-
((ConfigKeySelfExtracting<T>)ownKey).isSet(inheritedConfig)) {
- ExecutionContext exec = entity.getExecutionContext();
- result =
((ConfigKeySelfExtracting<T>)ownKey).extractValue(inheritedConfig, exec);
- complete = true;
- } else if (localConfigBag.containsKey(ownKey)) {
- // TODO configBag.get doesn't handle
tasks/attributeWhenReady - it only uses TypeCoercions
- result = localConfigBag.get(ownKey);
- complete = true;
- } else if (isInherited(ownKey, inheritance) &&
- inheritedConfigBag.containsKey(ownKey)) {
- result = inheritedConfigBag.get(ownKey);
- complete = true;
- }
+ Maybe<T> result =
getConfigImpl((ConfigKeySelfExtracting<T>)ownKey, parentInheritanceMode);
if (rawval instanceof Task) {
entity.getManagementSupport().getEntityChangeListener().onConfigChanged(key);
}
- if (complete) {
- return result;
+ if (result.isPresent()) {
+ return result.get();
}
} else {
LOG.warn("Config key {} of {} is not a
ConfigKeySelfExtracting; cannot retrieve value; returning default", ownKey,
this);
}
return TypeCoercions.coerce((defaultValue != null) ? defaultValue
: ownKey.getDefaultValue(), key.getTypeToken());
}
+
+ @SuppressWarnings("unchecked")
+ private <T> Maybe<T> getConfigImpl(ConfigKeySelfExtracting<T> key,
InheritanceMode parentInheritance) {
+ ExecutionContext exec = entity.getExecutionContext();
+ Maybe<T> ownValue;
+ Maybe<T> parentValue;
+
+ // Get own value
+ if (((ConfigKeySelfExtracting<T>)key).isSet(ownConfig)) {
+ ownValue =
Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(ownConfig, exec));
+ } else if (localConfigBag.containsKey(key)) {
+ // TODO configBag.get doesn't handle tasks/attributeWhenReady
- it only uses TypeCoercions
+ // Precedence ordering has changed; previously we'd prefer an
explicit isSet(inheritedConfig)
+ // over the localConfigBag.get(key).
+ ownValue = Maybe.of(localConfigBag.get(key));
+ } else {
+ ownValue = Maybe.<T>absent();
+ }
+
+ // Get the parent-inheritance value (but only if we'll need it)
+ switch (parentInheritance) {
+ case IF_NO_EXPLICIT_VALUE:
+ if (ownValue.isAbsent()) {
+ if
(((ConfigKeySelfExtracting<T>)key).isSet(inheritedConfig)) {
+ parentValue =
Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(inheritedConfig, exec));
+ } else if (inheritedConfigBag.containsKey(key)) {
+ parentValue = Maybe.of(inheritedConfigBag.get(key));
+ } else {
+ parentValue = Maybe.absent();
+ }
+ } else {
+ parentValue = Maybe.absent();
+ }
+ break;
+ case MERGE:
+ if (((ConfigKeySelfExtracting<T>)key).isSet(inheritedConfig)) {
+ parentValue =
Maybe.of(((ConfigKeySelfExtracting<T>)key).extractValue(inheritedConfig, exec));
+ } else if (inheritedConfigBag.containsKey(key)) {
+ parentValue = Maybe.of(inheritedConfigBag.get(key));
+ } else {
+ parentValue = Maybe.absent();
+ }
+ break;
+ case NONE:
+ parentValue = Maybe.absent();
+ break;
+ default:
+ throw new IllegalStateException("Unsupported
parent-inheritance mode for "+key.getName()+": "+parentInheritance);
+ }
+
+ // Merge or override, as appropriate
+ switch (parentInheritance) {
+ case IF_NO_EXPLICIT_VALUE:
+ return ownValue.isPresent() ? ownValue : parentValue;
+ case MERGE:
+ return (Maybe<T>) deepMerge(ownValue, parentValue, key);
+ case NONE:
+ return ownValue;
+ default:
+ throw new IllegalStateException("Unsupported
parent-inheritance mode for "+key.getName()+": "+parentInheritance);
+ }
+ }
+
+ private <T> Maybe<?> deepMerge(Maybe<? extends T> val1, Maybe<?
extends T> val2, ConfigKey<?> keyForLogging) {
+ if (val2.isAbsent() || val2.isNull()) {
+ return val1;
+ } else if (val1.isAbsent()) {
+ return val2;
+ } else if (val1.isNull()) {
+ return val1; // an explicit null means an override; don't merge
+ } else if (val1.get() instanceof Map && val2.get() instanceof Map)
{
+ return
Maybe.of(CollectionMerger.builder().build().merge((Map<?,?>)val1.get(),
(Map<?,?>)val2.get()));
--- End diff --
@neykov I kind of agree, but am not sure. The examples I can think of are
all shallow except for templateOptions. So doing a deep merge will have no
impact (because the values of `shell.env` etc will be shallow). Therefore
keeping the default as deep feels like we have a consistent rule that makes it
work for templateOptions, and hopefully doesn't impact the other uses.
I worry slightly about deep for `templateOptions` (versus an explicit
depth=2). If a value within templateOptions is a map, then it will be
deep-merged rather than overriding. Most of the values are not, but there are
some - e.g. jclouds `TemplateOptions.userMetadata(Map<String, String>
userMetadata)`. What would someone expect? Would they expect to merge
additional userMetadata or to override the value?
There is also potentially surprising behaviour for lists + sets within
templateOptions. These values are merged to concatenate the sets/lists. For
example, `TemplateOptions.networks(Iterable<String> networks)` will be merged.
I think for now we just want a sensible default that can be easily
explained. And we can worry later about the edge cases where the default
doesn't match what someone wants, and it's tricky (or even impossible) for them
to avoid that without changing their super-type location-definition.
Another possibility is that we have a parameterised `MERGE` that says the
depth. But that seems overly elaborate. Same for saying whether lists/sets
should be merged or not.
So in conclusion:
* should we just rename it to DEEP_MERGE?
* should we change the defaults so that it doesn't merge (i.e. concatenate)
lists/sets deep within a map when doing a merge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
