[GitHub] brooklyn-server pull request #462: Inherit config default values

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/brooklyn-server/pull/462


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r101306815
  
--- Diff: 
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
 Maybe other = (Maybe)obj;
 if (!isPresent()) 
 return !other.isPresent();
+if (!other.isPresent())
+return false;
--- End diff --

Don't mind me, somehow missed the previous check where two absents will be 
equal.
My impression was exactly the opposite, that with the change two absents 
are never equal, even for `this.equals(this)`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r101298270
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BrooklynDynamicType.java ---
@@ -175,20 +179,29 @@ protected static void buildConfigKeys(Class clazz, Abs
 LOG.warn("cannot access config key (skipping): "+f);
 }
 }
+Collection interfaces = 
MutableSet.copyOf(Arrays.asList(clazz.getInterfaces()));
 LinkedHashSet keys = new 
LinkedHashSet(configKeysAll.keys());
 for (String kn: keys) {
 List> kk = 
Lists.newArrayList(configKeysAll.get(kn));
-if (kk.size()>1) {
-// remove anything which extends another value in the list
-for (FieldAndValue k: kk) {
-ConfigKey key = value(k);
-if (key instanceof BasicConfigKeyOverwriting) {

-ConfigKey parent = 
((BasicConfigKeyOverwriting)key).getParentKey();
-// find and remove the parent from consideration
-for (FieldAndValue k2: kk) {
-if (value(k2) == parent)
-configKeysAll.remove(kn, k2);
-}
+// remove anything which isn't reinheritable, or which is 
overridden
+for (FieldAndValue k: kk) {
+ConfigKey key = value(k);
+if (!ConfigInheritances.isKeyReinheritable(key, 
InheritanceContext.TYPE_DEFINITION)) {
+if (k.field.getDeclaringClass().equals(clazz)) {
+// proceed if key is declared on this class
+} else if 
(interfaces.contains(k.field.getDeclaringClass())) {
+// proceed if key is declared on an exlicitly 
declared interface
+} else {
+// key not re-inheritable from parent so not 
visible here
+configKeysAll.remove(k.value.getName(), k);
+}
+}
+if (key instanceof BasicConfigKeyOverwriting) {

+ConfigKey parent = 
((BasicConfigKeyOverwriting)key).getParentKey();
+// find and remove the parent from consideration
+for (FieldAndValue k2: kk) {
+if (value(k2) == parent)
+configKeysAll.remove(kn, k2);
 }
 }
 kk = Lists.newArrayList(configKeysAll.get(kn));
--- End diff --

You've moved this update of `kk` inside the `for( ... k : kk)` loop, which 
doesn't look right. Updating the list variable each time round an iterator 
initialised from that variable?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r101001789
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
+}
+
+/** as {@link #resolveWithAncestor(SpecParameter)} but where the 
param redefines/extends a config key coming from a java supertype, rather than 
a parameter */
+// TODO not used yet; see calls to the other resolveWithAncestor 
method,
+// and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+// also note whilst it is easiest to do this here, logically it is 
messy,
+// and arguably it should be done when converting the spec to an 
instance
+

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100937270
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -309,7 +298,7 @@ public void testCatalogConfigOverridesParameters() {
 CatalogItem item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-assertEquals(params.size(), 3);
+assertEquals(params.size(), NUM_ENTITY_DEFAULT_CONFIG_KEYS + 2);
--- End diff --

if we deployed i think it would be; here we're just doing `peekSpec` so it 
doesn't have the app config keys, it just has `NUM_ENTITY_DEFAULT_CONFIG_KEYS + 
ConfigEntityForTest.NUM_CONFIG_KEYS_DEFINED_HERE + 
NUM_CONFIG_KEYS_FROM_TEST_BLUEPRINT` = `1+1+1`


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100936874
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
 CatalogItem item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, 
"params="+params);
 assertTrue(Iterables.tryFind(params, 
nameEqualTo("simple")).isPresent());
+assertTrue(Iterables.tryFind(params, 
nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, 
"params="+params);
--- End diff --

nice idea to use constants for all the counts.  have also added comment 
about unwrapping, agree it's a bit wasteful, but doesn't seem to do any harm.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100846590
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
--- End diff --

yes, should always be top-down, and the `SpecParameterForInheritance` items 
replaced immediately.  i'll have another look.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100846344
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
--- End diff --

the separate class is to know when something still needs to be inherited 
and when inheritance has been completed.  would welcome a second opinion here 
@neykov because it is ugly, but i think it should never leak ie the ugliness is 
entirely hidden.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100844827
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java
 ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable 
tagsToReplace) {
 // it is a CatalogConfig or merely a config key, maybe introducing 
displayable, or even priority 
 // (but note part of the reason for CatalogConfig.priority is that 
java reflection doesn't preserve field order) .
 // see also comments on the camp SpecParameterResolver.
+
+// probably the thing to do is deprecate the ambiguous method in 
favour of an explicit
 @Beta
 public SpecT parameters(List> parameters) {
 return parametersReplace(parameters);
 }
-/** adds the given parameters */
+/** adds the given parameters, new ones first so they dominate 
subsequent ones */
 @Beta
 public SpecT parametersAdd(List> 
parameters) {
 // parameters follows immutable pattern, unlike the other fields
 Set params = 
MutableSet.copyOf(parameters);
 Set current = 
MutableSet.copyOf(this.parameters);
 current.removeAll(params);
 
-this.parameters = ImmutableList.builder()
+return parametersReplace(ImmutableList.builder()
 .addAll(params)
 .addAll(current)
-.build();
-return self();
+.build());
 }
 /** replaces parameters with the given */
 @Beta
-public SpecT parametersReplace(List> 
parameters) {
+public SpecT parametersReplace(Collection> 
parameters) {
--- End diff --

going with `Iterable`


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100810977
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---
@@ -152,8 +152,8 @@
 /** This is currently the only way to get some rolled up 
collections and raw,
  * and also to test for the presence of a value (without any 
default).
  * As more accessors are added callers may be asked to migrate. 
- * Callers may also consider using {@link 
#findKeys(com.google.common.base.Predicate)}
- * if that isn't too inefficient. */
+ * Callers may also consider using {@link 
#findKeysDeprecated(com.google.common.base.Predicate)}
--- End diff --

yes - TY


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100810871
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
+}
+
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
+
+} catch (Exception e) {
+throw Exceptions.propagate(e);
+}
+
+return returnEquivalentConstant(this);
 }
 
 @Override
+public boolean equals(Object obj) {
--- End diff --

good spot, also added for `DelegatingCI`


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100810442
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100810106
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
+}
+
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
+
+} catch (Exception e) {
+throw Exceptions.propagate(e);
+}
+
+return returnEquivalentConstant(this);
 }
 
 @Override
+public boolean equals(Object obj) {
+if (obj==null) return false;
+if (obj instanceof DelegatingConfigInheritance) return equals( 
((DelegatingConfigInheritance)obj).getDelegate() );
+if (obj.getClass().equals(BasicConfigInheritance.class)) {
--- End diff --

we don't have any subclasses so it doesn't matter; have switched to 
instance, assuming if it is subclasses then the author will extend the equals 
if needed.  also refactored so easier to read and if subclassed easier to 
extend the check.  (you're right, `instanceof` is better as it permits the 
caller to reuse some of the logic.)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100808284
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
+}
+
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
--- End diff --

reflection because fields are final; added comment


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100808041
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
--- End diff --

i think this should never happen; logging is safety first in case it is


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100807828
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -82,9 +223,17 @@ public InheritanceMode isInherited(ConfigKey key, 
Object from, Object to) {
 
 protected  void 
checkInheritanceContext(ConfigValueAtContainer local, 
ConfigInheritanceContext context) {
 ConfigInheritance rightInheritance = 
ConfigInheritances.findInheritance(local, context, this);
-if (!equals(rightInheritance)) 
+if (!isSameRootInstanceAs(rightInheritance)) {
 throw new IllegalStateException("Low level inheritance 
computation error: caller should invoke on "+rightInheritance+" "
 + "(the inheritance at "+local+"), not "+this);
+}
+}
+
+private boolean isSameRootInstanceAs(ConfigInheritance other) {
+if (other==null) return false;
+if (this==other) return true;
+if (other instanceof DelegatingConfigInheritance) return 
isSameRootInstanceAs( ((DelegatingConfigInheritance)other).getDelegate() );
--- End diff --

`equals` would work but it wouldn't catch quite as many errors; it should 
be invoked on the same _instance_; have added comment to explain.  you're right 
dropping the delegate pattern would allow this line to be removed but i think 
losing the internal fields in persisted state is worth it.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100806157
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100805561
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
--- End diff --

agree


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100802788
  
--- Diff: 
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
 Maybe other = (Maybe)obj;
 if (!isPresent()) 
 return !other.isPresent();
+if (!other.isPresent())
+return false;
--- End diff --

I suppose it's useful when you put it inside containers (i.e. 
`remove(Object)` works for sets)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100798508
  
--- Diff: 
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
 Maybe other = (Maybe)obj;
 if (!isPresent()) 
 return !other.isPresent();
+if (!other.isPresent())
+return false;
--- End diff --

that seems harsh to me; could cause problems if we serialized a `Maybe`?

could compare the exceptions but for now I will just add javadoc


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100796966
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
 String label = (String)inputDef.get("label");
 String description = (String)inputDef.get("description");
 String type = (String)inputDef.get("type");
-Object defaultValue = inputDef.get("default");
 Boolean pinned = (Boolean)inputDef.get("pinned");
+boolean hasDefaultValue = inputDef.containsKey("default");
+Object defaultValue = inputDef.get("default");
 if (specialFlagTransformer != null) {
 defaultValue = specialFlagTransformer.apply(defaultValue);
 }
+boolean hasConstraints = inputDef.containsKey("constraints");
 Predicate constraint = 
parseConstraints(inputDef.get("constraints"), loader);
-ConfigInheritance parentInheritance = 
parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ConfigInheritance runtimeInheritance;
+boolean hasRuntimeInheritance;
+if (inputDef.containsKey("inheritance.runtime")) {
+hasRuntimeInheritance = true;
+runtimeInheritance = 
parseInheritance(inputDef.get("inheritance.runtime"), loader);
+} else if (inputDef.containsKey("inheritance.parent")) {
+// this alias will be deprecated
+hasRuntimeInheritance = true;
+runtimeInheritance = 
parseInheritance(inputDef.get("inheritance.parent"), loader);
+} else {
+hasRuntimeInheritance = false;
+runtimeInheritance = parseInheritance(null, loader);
--- End diff --

done, thx


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100796950
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
 String label = (String)inputDef.get("label");
 String description = (String)inputDef.get("description");
 String type = (String)inputDef.get("type");
-Object defaultValue = inputDef.get("default");
 Boolean pinned = (Boolean)inputDef.get("pinned");
+boolean hasDefaultValue = inputDef.containsKey("default");
+Object defaultValue = inputDef.get("default");
 if (specialFlagTransformer != null) {
 defaultValue = specialFlagTransformer.apply(defaultValue);
 }
+boolean hasConstraints = inputDef.containsKey("constraints");
 Predicate constraint = 
parseConstraints(inputDef.get("constraints"), loader);
-ConfigInheritance parentInheritance = 
parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ConfigInheritance runtimeInheritance;
+boolean hasRuntimeInheritance;
+if (inputDef.containsKey("inheritance.runtime")) {
+hasRuntimeInheritance = true;
+runtimeInheritance = 
parseInheritance(inputDef.get("inheritance.runtime"), loader);
+} else if (inputDef.containsKey("inheritance.parent")) {
+// this alias will be deprecated
--- End diff --

done


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100796338
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---
@@ -255,8 +255,9 @@ private static void 
mergeWrapperParentSpecToChildEntity(EntitySpec

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100795903
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java
 ---
@@ -403,8 +408,10 @@ protected ConfigInheritance 
getDefaultConfigInheritance() {
 // name "B" the "val2" then applies to both!
 //
 // I plan to propose a change on dev@brooklyn, to replace 
`@SetFromFlag`!
-
-Set allKeys = MutableSet.of();
+
+// need to de-dupe? (can't use Set bc FCKVR doesn't impl 
equals/hashcode)
--- End diff --

good job we didn't call it `FlagUnifiedConfigKeyValueRecord` :)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100795410
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100789705
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
--- End diff --

that would work but i put the extra branch in to show the logic that should 
be applied; have used your cleanup to extract `existingP` just once, and 
expanded the placeholder code so it's clearer


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100786370
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
 ---
@@ -397,48 +397,83 @@ protected ConfigInheritance 
getDefaultRuntimeInheritance() {
 
 final InheritanceContext context = 
InheritanceContext.RUNTIME_MANAGEMENT;
 ConfigInheritance currentInheritance = 
ConfigInheritances.findInheritance(queryKey, context, 
getDefaultRuntimeInheritance());
-
+
 BasicConfigValueAtContainer last = null;
-
+
 while (c!=null) {
 Maybe v = getRawValueAtContainer(c, queryKey);
 BasicConfigValueAtContainer next = new 
BasicConfigValueAtContainer(c, getKeyAtContainer(c, 
queryKey), v);
-
+
 if (last!=null && !currentInheritance.considerParent(last, 
next, context)) break;
-
+
 ConfigInheritance currentInheritanceExplicit = 
ConfigInheritances.findInheritance(next.getKey(), 
InheritanceContext.RUNTIME_MANAGEMENT, null);
 if (currentInheritanceExplicit!=null) {
 if (count>0 && 
!currentInheritanceExplicit.isReinheritable(next, context)) break;
 currentInheritance = currentInheritanceExplicit;
 }
-
+
 if (next.isValueExplicitlySet()) result.add(0, next);
 
 last = next;
 c = getParentOfContainer(c);
 count++;
 }
-
+
 return result;
 }
-
-@Override
+
+@Override @Deprecated
 public Set findKeys(Predicate> 
filter) {
+return findKeys(filter, KeyFindingMode.PRESENT_NOT_RESOLVED);
+}
+
+@Override
+public Set findKeysDeclared(Predicate> filter) {
+return findKeys(filter, KeyFindingMode.DECLARED_OR_PRESENT);
+}
+
+@Override
+public Set findKeysPresent(Predicate> filter) {
+return findKeys(filter, KeyFindingMode.PRESENT_BUT_RESOLVED);
+}
+
+protected enum KeyFindingMode { DECLARED_OR_PRESENT, 
PRESENT_BUT_RESOLVED, PRESENT_NOT_RESOLVED }
--- End diff --

agree


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100526488
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100509692
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100509157
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100507371
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
+}
+
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
+
+} catch (Exception e) {
+throw Exceptions.propagate(e);
+}
+
+return returnEquivalentConstant(this);
 }
 
 @Override
+public boolean equals(Object obj) {
--- End diff --

Implements `equals` but not `hashCode`, which is wrong.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100370235
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java
 ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable 
tagsToReplace) {
 // it is a CatalogConfig or merely a config key, maybe introducing 
displayable, or even priority 
 // (but note part of the reason for CatalogConfig.priority is that 
java reflection doesn't preserve field order) .
 // see also comments on the camp SpecParameterResolver.
+
+// probably the thing to do is deprecate the ambiguous method in 
favour of an explicit
 @Beta
 public SpecT parameters(List> parameters) {
 return parametersReplace(parameters);
 }
-/** adds the given parameters */
+/** adds the given parameters, new ones first so they dominate 
subsequent ones */
 @Beta
 public SpecT parametersAdd(List> 
parameters) {
 // parameters follows immutable pattern, unlike the other fields
 Set params = 
MutableSet.copyOf(parameters);
 Set current = 
MutableSet.copyOf(this.parameters);
 current.removeAll(params);
 
-this.parameters = ImmutableList.builder()
+return parametersReplace(ImmutableList.builder()
 .addAll(params)
 .addAll(current)
-.build();
-return self();
+.build());
 }
 /** replaces parameters with the given */
 @Beta
-public SpecT parametersReplace(List> 
parameters) {
+public SpecT parametersReplace(Collection> 
parameters) {
--- End diff --

I'd be fine with this being the even more general 
`parametersReplace(Iterable> parameters)` (which 
implies ordered, even though it's a super-type of collection!)

Agree with the comment about consistency - all three methods should take 
the same type for their parameter.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r100523726
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java ---
@@ -152,8 +152,8 @@
 /** This is currently the only way to get some rolled up 
collections and raw,
  * and also to test for the presence of a value (without any 
default).
  * As more accessors are added callers may be asked to migrate. 
- * Callers may also consider using {@link 
#findKeys(com.google.common.base.Predicate)}
- * if that isn't too inefficient. */
+ * Callers may also consider using {@link 
#findKeysDeprecated(com.google.common.base.Predicate)}
--- End diff --

Do you mean `findKeysDeclared`, rather than `findKeysDeprecated`?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94975515
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
--- End diff --

Could make this a constant.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94960067
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
--- End diff --

Suggest we introduce support for both cases at the same time to avoid 
confusion of when the inheritance works and when it doesn't.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94962954
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
+}
+
+/** as {@link #resolveWithAncestor(SpecParameter)} but where the 
param redefines/extends a config key coming from a java supertype, rather than 
a parameter */
+// TODO not used yet; see calls to the other resolveWithAncestor 
method,
+// and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+// also note whilst it is easiest to do this here, logically it is 
messy,
+// and arguably it should be done when converting the spec to an 
instance
+

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94962175
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
--- End diff --

Can you guarantee that resolving is always top-down? What I mean, is it 
possible to call `resolveWithAncestor` with another 
`SpecParameterForInheritance`?
If so then should update `hasXXX` fields according to the `ancestor` state.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94961965
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
--- End diff --

Wrong indentation - all should start at the same column.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94980522
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94968550
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
+}
+
+/** as {@link #resolveWithAncestor(SpecParameter)} but where the 
param redefines/extends a config key coming from a java supertype, rather than 
a parameter */
+// TODO not used yet; see calls to the other resolveWithAncestor 
method,
+// and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+// also note whilst it is easiest to do this here, logically it is 
messy,
+// and arguably it should be done when converting the spec to an 
instance
+

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94785113
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
 CatalogItem item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, 
"params="+params);
 assertTrue(Iterables.tryFind(params, 
nameEqualTo("simple")).isPresent());
+assertTrue(Iterables.tryFind(params, 
nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, 
"params="+params);
--- End diff --

Unrelated, but in the case of unwrapping the spec because we are expecting 
a non-app spec it's surprising to inherit the wrapper app config (and anything 
else from it).



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94787693
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -309,7 +298,7 @@ public void testCatalogConfigOverridesParameters() {
 CatalogItem item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-assertEquals(params.size(), 3);
+assertEquals(params.size(), NUM_ENTITY_DEFAULT_CONFIG_KEYS + 2);
--- End diff --

Hmm now that we are merging parameters with config keys I'd expect this 
should behave the same as `testRootParametersUnwrapped` (again unrelated to the 
PR).



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94927466
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---
@@ -255,8 +255,9 @@ private static void 
mergeWrapperParentSpecToChildEntity(EntitySpechttps://github.com/apache/brooklyn-server/blob/master/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java#L320).
 Or is it still TBD?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94777251
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java
 ---
@@ -147,27 +147,28 @@ public SpecT tagsReplace(Iterable 
tagsToReplace) {
 // it is a CatalogConfig or merely a config key, maybe introducing 
displayable, or even priority 
 // (but note part of the reason for CatalogConfig.priority is that 
java reflection doesn't preserve field order) .
 // see also comments on the camp SpecParameterResolver.
+
+// probably the thing to do is deprecate the ambiguous method in 
favour of an explicit
 @Beta
 public SpecT parameters(List> parameters) {
 return parametersReplace(parameters);
 }
-/** adds the given parameters */
+/** adds the given parameters, new ones first so they dominate 
subsequent ones */
 @Beta
 public SpecT parametersAdd(List> 
parameters) {
 // parameters follows immutable pattern, unlike the other fields
 Set params = 
MutableSet.copyOf(parameters);
 Set current = 
MutableSet.copyOf(this.parameters);
 current.removeAll(params);
 
-this.parameters = ImmutableList.builder()
+return parametersReplace(ImmutableList.builder()
 .addAll(params)
 .addAll(current)
-.build();
-return self();
+.build());
 }
 /** replaces parameters with the given */
 @Beta
-public SpecT parametersReplace(List> 
parameters) {
+public SpecT parametersReplace(Collection> 
parameters) {
--- End diff --

`parameters` are ordered - that's why the signature requires a `List`. It's 
also consistent with the other two parameters related methods.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94981371
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94973110
  
--- Diff: 
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -457,6 +457,8 @@ public boolean equals(Object obj) {
 Maybe other = (Maybe)obj;
 if (!isPresent()) 
 return !other.isPresent();
+if (!other.isPresent())
+return false;
--- End diff --

`Optional.absent()` returns `true` if `other==this`. Could be useful to 
keep the same convention.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94961073
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
--- End diff --

What's the case of having this as a separate class vs updating 
`BasicSpecParameter`?
Personal preference to sticking with just `BasicSpecParameter`  ( + `Maybe` 
for the fields) to avoid `instanceof` checks. For persistence compat - could 
use the `readObject` trick.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94984001
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -82,9 +223,17 @@ public InheritanceMode isInherited(ConfigKey key, 
Object from, Object to) {
 
 protected  void 
checkInheritanceContext(ConfigValueAtContainer local, 
ConfigInheritanceContext context) {
 ConfigInheritance rightInheritance = 
ConfigInheritances.findInheritance(local, context, this);
-if (!equals(rightInheritance)) 
+if (!isSameRootInstanceAs(rightInheritance)) {
 throw new IllegalStateException("Low level inheritance 
computation error: caller should invoke on "+rightInheritance+" "
 + "(the inheritance at "+local+"), not "+this);
+}
+}
+
+private boolean isSameRootInstanceAs(ConfigInheritance other) {
+if (other==null) return false;
+if (this==other) return true;
+if (other instanceof DelegatingConfigInheritance) return 
isSameRootInstanceAs( ((DelegatingConfigInheritance)other).getDelegate() );
--- End diff --

Another reason to avoid the delegate. Why is `equals` a bad idea?
Note that you could still end up with an anonymous `BasicConfigInheritance` 
coming from persistence - they are not resolved to the singletons.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94924294
  
--- Diff: api/src/main/java/org/apache/brooklyn/api/objs/Configurable.java 
---
@@ -98,6 +99,14 @@
  */
  T set(HasConfigKey key, Task val);
 
-Set findKeys(Predicate> 
predicate);
+/** @deprecated since 0.11.0 see {@link 
ConfigMap#findKeys(Predicate)} */
+@Deprecated
+Set findKeys(Predicate> filter);
+
+/** see {@link ConfigMap#findKeysDelcared(Predicate)}  */
--- End diff --

Typo `Delcared`


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94985326
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
--- End diff --

Are you suspecting the object could be coming from a non-xstream based 
deserialization? If not, then this is too much on the defensive side.



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94952205
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -227,13 +234,30 @@ public String toString() {
 String label = (String)inputDef.get("label");
 String description = (String)inputDef.get("description");
 String type = (String)inputDef.get("type");
-Object defaultValue = inputDef.get("default");
 Boolean pinned = (Boolean)inputDef.get("pinned");
+boolean hasDefaultValue = inputDef.containsKey("default");
+Object defaultValue = inputDef.get("default");
 if (specialFlagTransformer != null) {
 defaultValue = specialFlagTransformer.apply(defaultValue);
 }
+boolean hasConstraints = inputDef.containsKey("constraints");
 Predicate constraint = 
parseConstraints(inputDef.get("constraints"), loader);
-ConfigInheritance parentInheritance = 
parseInheritance(inputDef.get("inheritance.parent"), loader);
+
+ConfigInheritance runtimeInheritance;
+boolean hasRuntimeInheritance;
+if (inputDef.containsKey("inheritance.runtime")) {
+hasRuntimeInheritance = true;
+runtimeInheritance = 
parseInheritance(inputDef.get("inheritance.runtime"), loader);
+} else if (inputDef.containsKey("inheritance.parent")) {
+// this alias will be deprecated
--- End diff --

+1

Log a warning with a deprecation message.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94785585
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -169,19 +175,23 @@ public void 
testDependantCatalogsInheritParameters(Class item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-switch (type.getSimpleName()) {
-case "ConfigEntityForTest":
-assertEquals(params.size(), 3);
-break;
-case "ConfigPolicyForTest":
-assertEquals(params.size(), 2);
-break;
-case "ConfigLocationForTest":
-assertEquals(params.size(), 7);
-break;
-}
+// should have simple in parent yaml type and sample from parent 
java type
+Asserts.assertSize(params, getDefaultsFor(type.getSimpleName()) + 
2);
 assertTrue(Iterables.tryFind(params, 
nameEqualTo("simple")).isPresent());
 assertTrue(Iterables.tryFind(params, 
labelEqualTo("simple")).isPresent());
+assertTrue(Iterables.tryFind(params, 
nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+}
+
+private int getDefaultsFor(String simpleName) {
--- End diff --

Good cleanup, could include it as a second parameter in the provider, this 
works as well.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94962666
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
+}
+
+/** as {@link #resolveWithAncestor(SpecParameter)} but where the 
param redefines/extends a config key coming from a java supertype, rather than 
a parameter */
+// TODO not used yet; see calls to the other resolveWithAncestor 
method,
+// and see BrooklynComponentTemplateResolver.findAllConfigKeys;
+// also note whilst it is easiest to do this here, logically it is 
messy,
+// and arguably it should be done when converting the spec to an 
instance
+

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94985394
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
+}
+
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
--- End diff --

Could you add a comment describing why the use of reflection - can't figure 
it out.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94782620
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java
 ---
@@ -146,8 +151,9 @@ public void testRootParametersUnwrapped() {
 CatalogItem item = catalog.getCatalogItem(SYMBOLIC_NAME, 
TEST_VERSION);
 AbstractBrooklynObjectSpec spec = catalog.peekSpec(item);
 List params = spec.getParameters();
-assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 1, 
"params="+params);
 assertTrue(Iterables.tryFind(params, 
nameEqualTo("simple")).isPresent());
+assertTrue(Iterables.tryFind(params, 
nameEqualTo(SHARED_CONFIG.getName())).isPresent());
+assertEquals(params.size(), NUM_APP_DEFAULT_CONFIG_KEYS + 2, 
"params="+params);
--- End diff --

Could introduce `NUM_CONFIG_ENTITY_FOR_TEST_CONFIG_KEYS` and change this to 
 `NUM_APP_DEFAULT_CONFIG_KEYS + NUM_CONFIG_ENTITY_FOR_TEST_CONFIG_KEYS + 1`, 
not clear why the `+2` otherwise (same for following changes).



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94780176
  
--- Diff: 
camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java
 ---
@@ -403,8 +408,10 @@ protected ConfigInheritance 
getDefaultConfigInheritance() {
 // name "B" the "val2" then applies to both!
 //
 // I plan to propose a change on dev@brooklyn, to replace 
`@SetFromFlag`!
-
-Set allKeys = MutableSet.of();
+
+// need to de-dupe? (can't use Set bc FCKVR doesn't impl 
equals/hashcode)
--- End diff --

It's `FCKAVR`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94440253
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94396058
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializerTest.java
 ---
@@ -792,4 +794,13 @@ private Bundle installBundle(ManagementContext mgmt, 
String bundleUrl) throws Ex
 Framework framework = osgiManager.getFramework();
 return Osgis.install(framework, bundleUrl);
 }
+
+@Test
+public void testConfigInheritanceVals() throws Exception {
+ConfigInheritance val = BasicConfigInheritance.NEVER_INHERITED;
+
+ConfigInheritance newVal = assertSerializeAndDeserialize(val); // 
TODO this line fails
--- End diff --

the comment can be removed now


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94440837
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java
 ---
@@ -355,40 +355,40 @@ protected ConfigInheritance 
getDefaultRuntimeInheritance() {
 // prefer default and type of ownKey
 Maybe defaultValue = raw ? Maybe.absent() :
 ownKey.hasDefaultValue() ? 
coerceFn.apply(Maybe.of((Object)ownKey.getDefaultValue())) : 
-queryKey.hasDefaultValue() ? 
coerceFn.apply(Maybe.of((Object)queryKey.getDefaultValue())) :
-Maybe.absent();
-
-if (ownKey instanceof ConfigKeySelfExtracting) {
-
-Function lookupFn = new 
Function() {
-@Override public Maybe apply(TContainer input) {
-// lookup against ownKey as it may do extra resolution 
(eg grab *.* subkeys if a map)
-Maybe result = getRawValueAtContainer(input, 
ownKey);
-if (!raw) result = resolveRawValueFromContainer(input, 
ownKey, result);
-return result;
-}
-};
-Function parentFn = new 
Function() {
-@Override public TContainer apply(TContainer input) {
-return getParentOfContainer(input);
+queryKey.hasDefaultValue() ? 
coerceFn.apply(Maybe.of((Object)queryKey.getDefaultValue())) :
+Maybe.absent();
+
+if (ownKey instanceof ConfigKeySelfExtracting) {
--- End diff --

Indentation is wrong


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94418145
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94415792
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -28,51 +32,188 @@
 import org.apache.brooklyn.config.ConfigKey;
 import org.apache.brooklyn.config.ConfigValueAtContainer;
 import org.apache.brooklyn.util.collections.CollectionMerger;
+import org.apache.brooklyn.util.exceptions.Exceptions;
 import org.apache.brooklyn.util.exceptions.ReferenceWithError;
 import org.apache.brooklyn.util.guava.Maybe;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+@SuppressWarnings("serial")
 public class BasicConfigInheritance implements ConfigInheritance {
 
+private static final Logger log = 
LoggerFactory.getLogger(BasicConfigInheritance.class);
+
 private static final long serialVersionUID = -5916548049057961051L;
 
 public static final String CONFLICT_RESOLUTION_STRATEGY_DEEP_MERGE = 
"deep_merge";
 public static final String CONFLICT_RESOLUTION_STRATEGY_OVERWRITE = 
"overwrite";
 
+public static abstract class DelegatingConfigInheritance implements 
ConfigInheritance {
+protected abstract ConfigInheritance getDelegate();
+
+@Override @Deprecated public InheritanceMode 
isInherited(ConfigKey key, Object from, Object to) {
+return getDelegate().isInherited(key, from, to);
+}
+@Override public  boolean 
isReinheritable(ConfigValueAtContainer parent, 
ConfigInheritanceContext context) {
+return getDelegate().isReinheritable(parent, context);
+}
+@Override public  boolean 
considerParent(ConfigValueAtContainer local, 
ConfigValueAtContainer parent, ConfigInheritanceContext 
context) {
+return getDelegate().considerParent(local, parent, context);
+}
+@Override
+public  
ReferenceWithError> 
resolveWithParent(ConfigValueAtContainer local, 
ConfigValueAtContainer resolvedParent, 
ConfigInheritanceContext context) {
+return getDelegate().resolveWithParent(local, resolvedParent, 
context);
+}
+@Override
+public boolean equals(Object obj) {
+return super.equals(obj) || getDelegate().equals(obj);
+}
+
+// standard deserialization method
+protected ConfigInheritance readResolve() {
+return returnEquivalentConstant(this);
+}
+}
+
+private static ConfigInheritance 
returnEquivalentConstant(ConfigInheritance candidate) {
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, BasicConfigInheritance.DEEP_MERGE)) {
+if (candidate.equals(knownMode)) return knownMode;
+}
+if (candidate.equals(new BasicConfigInheritance(false, 
CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, true, true))) {
+// ignore the ancestor flag for this mode
+return NEVER_INHERITED;
+}
+return candidate;
+}
+
+/*
+ * use of delegate is so that stateless classes can be defined to make 
the serialization nice,
+ * both the name and hiding the implementation detail (also making it 
easier for that detail to change);
+ * with aliased type names the field names here could even be aliases 
for the type names.
+ * (we could alternatively have an "alias-for-final-instance" mode in 
serialization,
+ * to optimize where we know that a java instance is final.)   
+ */
+
+private static class NotReinherited extends 
DelegatingConfigInheritance {
+final static BasicConfigInheritance DELEGATE = new 
BasicConfigInheritance(false, CONFLICT_RESOLUTION_STRATEGY_OVERWRITE, false, 
true); 
+@Override protected ConfigInheritance getDelegate() { return 
DELEGATE; }
+}
 /** Indicates that a config key value should not be passed down from a 
container where it is defined.
  * Unlike {@link #NEVER_INHERITED} these values can be passed down if 
set as anonymous keys at a container
  * (ie the container does not expect it) to a container which does 
expect it, but it will not be passed down further. 
  * If the inheritor also defines a value the parent's value is ignored 
irrespective 
  * (as in {@link #OVERWRITE}; see {@link 

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94416378
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +324,103 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
--- End diff --

since 0.11.0 now, and in a number of other places in the PR


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r94390638
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
--- End diff --

ignore that; have looked at it with a fresh eye, the `type` _is_ the sensor 
type of the parameter.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r93286450
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
--- End diff --

The argument `spec` is never used in this method, should it be?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r93292616
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -404,7 +431,126 @@ public static void 
addParameters(AbstractBrooklynObjectSpec spec, List 0) {
-spec.parametersAdd(explicitParams);
+spec.parametersReplace(resolveParameters(explicitParams, 
spec.getParameters(), spec));
+}
+}
+
+/** merge parameters against other parameters and known and 
type-inherited config keys */
+static Collection resolveParameters(Collection> newParams, Collection> 
existingReferenceParamsToKeep, AbstractBrooklynObjectSpec spec) {
+Map existingToKeep = MutableMap.of();
+if (existingReferenceParamsToKeep!=null) {
+for (SpecParameter p: existingReferenceParamsToKeep) { 
+existingToKeep.put(p.getConfigKey().getName(), p);
+}
+}
+
+List result = MutableList.of();
+for (SpecParameter p: newParams) {
+if (p instanceof SpecParameterForInheritance) {
+SpecParameter existingP = 
existingToKeep.remove(p.getConfigKey().getName());
+if (existingP!=null) {
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor(existingP);
+} else {
+// TODO find config keys on the type (not available as 
parameters)
+/* we don't currently do this due to low priority; all 
it means if there is a config key in java,
+ * and a user wishes to expose it as a parameter, they 
have to redeclare everything;
+ * nothing from the config key in java will be 
inherited */
+p = 
((SpecParameterForInheritance)p).resolveWithAncestor((ConfigKey)null);
+}
+result.add(p);
+} else {
+existingToKeep.remove(p.getConfigKey().getName());
+result.add(p);
+}
+}
+result.addAll(existingToKeep.values());
+return result;
+}
+
+/** instance of {@link SpecParameter} which includes information about 
what fields are explicitly set,
+ * for use with a subsequent merge */
+@SuppressWarnings("serial")
+@Beta
+static class SpecParameterForInheritance extends 
BasicSpecParameter {
+
+private final boolean hasType, hasLabelSet, hasPinnedSet, 
hasDefaultValue, hasConstraints, hasRuntimeInheritance, hasTypeInheritance;
+
+private  SpecParameterForInheritance(String label, 
Boolean pinned, ConfigKey config, AttributeSensor sensor,
+boolean hasType, boolean hasDefaultValue, boolean 
hasConstraints, boolean hasRuntimeInheritance, boolean hasTypeInheritance) {
+super(Preconditions.checkNotNull(label!=null ? label : 
config.getName(), "label or config name must be set"), 
+pinned==null ? true : pinned, config, sensor);
+this.hasType = hasType;
+this.hasLabelSet = label!=null;
+this.hasPinnedSet = pinned!=null;
+this.hasDefaultValue = hasDefaultValue;
+this.hasConstraints = hasConstraints;
+this.hasRuntimeInheritance = hasRuntimeInheritance;
+this.hasTypeInheritance = hasTypeInheritance;
+}
+
+/** used if yaml specifies a parameter, but possibly incomplete, 
and a spec supertype has a parameter */
+SpecParameter resolveWithAncestor(SpecParameter ancestor) {
+if (ancestor==null) return new 
BasicSpecParameter<>(getLabel(), isPinned(), getConfigKey(), getSensor());
+
+return new BasicSpecParameter<>(
+hasLabelSet ? getLabel() : ancestor.getLabel(), 
+hasPinnedSet ? isPinned() : 
ancestor.isPinned(), 
+
resolveWithAncestorConfigKey(ancestor.getConfigKey()), 
+hasType ? getSensor() : 
ancestor.getSensor());
--- End diff --

why is the decision between `getSensor()` and `ancestor.getSensor()` 
predicated upon `hasType` particularly? I don't see how they are related.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r90756006
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
 ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map nameToType) {
 
 @Override
 public Class realClass(String elementName) {
-Optional elementNameOpt = 
Reflections.tryFindMappedName(nameToType, elementName);
+Maybe elementNameOpt = 
Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --

My question about renaming wasn't so much about why change to return a 
`Maybe`. It's why change the method name? I prefer `tryFindMappedName` because 
it follows the guava naming convention. We use that naming convention in quite 
a few other places in Brooklyn as well.

I see now why - the previous `tryFindMappedName` is deprecated in favour of 
`findMappedNameMaybe` so that it can have a different return type. That's 
really annoying (that we're creating a different method naming convention so as 
to change the return type). But I'm  not sure how better to tackle that while 
still preserving backwards compatibility!

As for null, you've changed the contract. Maybe it's fine that the super 
throws the `NullPointerException` rather than us checking so not a big deal. 
But on balance, I'd prefer to report a null present value here, rather than 
calling `super.realClass(null)`, which could get into completely different code 
before an NPE is thrown. No strong feelings though.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r90568817
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java
 ---
@@ -0,0 +1,145 @@
+/*
+ * 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 org.apache.brooklyn.core.mgmt.rebind;
+
+import java.io.File;
+import java.io.FileReader;
+
+import org.apache.brooklyn.api.entity.Application;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.config.ConfigInheritance;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.BasicConfigInheritance;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext;
+import org.apache.brooklyn.core.config.ConfigPredicates;
+import org.apache.brooklyn.core.entity.EntityAsserts;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.stream.Streams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+import com.google.common.io.Files;
+
+public class RebindConfigInheritanceTest extends RebindTestFixtureWithApp {
+
+private static final Logger log = 
LoggerFactory.getLogger(RebindConfigInheritanceTest.class);
+
+ConfigKey key1 = ConfigKeys.builder(String.class, 
"key1").runtimeInheritance(BasicConfigInheritance.NEVER_INHERITED).build();
+String origMemento, newMemento;
+Application rebindedApp;
+
+@Override
+@BeforeMethod(alwaysRun=true)
+public void setUp() throws Exception {
+super.setUp();
+}
+
+protected void doReadConfigInheritance(String label, String entityId) 
throws Exception {
+String mementoFilename = "config-inheritance-"+label+"-"+entityId;
+origMemento = 
Streams.readFullyString(getClass().getResourceAsStream(mementoFilename));
+
+File persistedEntityFile = new File(mementoDir, 
Os.mergePaths("entities", entityId));
+Files.write(origMemento.getBytes(), persistedEntityFile);
+
+// we'll make assertions on what we've loaded
+rebind();
+rebindedApp = (Application) newManagementContext.lookup(entityId);
+
+// we'll also make assertions on the contents written out
+RebindTestUtils.waitForPersisted(mgmt());
+newMemento = 
Joiner.on("\n").join(Files.readLines(persistedEntityFile, Charsets.UTF_8));
+}
+
+/** also see {@link RebindWithDeserializingClassRenamesTest} */
+@Test
+public void testPreBasicConfigInheritance_2016_07() throws Exception {
+doReadConfigInheritance("prebasic-2016-07", "toruf2wxg4");
+
+ConfigKey k = Iterables.getOnlyElement( 
rebindedApp.config().findKeys(ConfigPredicates.nameEqualTo("my.config.inheritanceMerged"))
 );
+
+Asserts.assertStringContains(origMemento, "");
+Asserts.assertStringDoesNotContain(origMemento, 
BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+// should now convert it to BasicConfigInheritance.DEEP_MERGE
+Asserts.assertStringDoesNotContain(newMemento, 
"ConfigInheritance$Merged");
+Asserts.assertStringDoesNotContain(newMemento, 
"ConfigInheritance$Legacy$Merged");
+Asserts.assertStringContains(newMemento, 
BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ConfigInheritance inh = 
k.getInheritanceByContext(InheritanceContext.RUNTIME_MANAGEMENT);
+   

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r90568746
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
 ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map nameToType) {
 
 @Override
 public Class realClass(String elementName) {
-Optional elementNameOpt = 
Reflections.tryFindMappedName(nameToType, elementName);
+Maybe elementNameOpt = 
Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --

the Maybe.absent() contains a useful error message. the Optional.absent() 
was completely unhelpful.

re null i don't know whether null is valid or not ... and nor do i need to, 
i don't think -- it's not our responsibility here (delegating the rename to the 
super) to check whether the rename method gave us the right thing.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r90567752
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/config/BasicConfigInheritance.java 
---
@@ -170,12 +306,88 @@ public String getConflictResolutionStrategy() {
 return conflictResolutionStrategy;
 }
 
+@Deprecated /** @deprecated since 0.10.0 when it was introduced, 
prefer {@link #getLocalDefaultResolvesWithAncestorValue()} */
 public boolean getUseLocalDefaultValue() {
-return useLocalDefaultValue;
+return getLocalDefaultResolvesWithAncestorValue();
+}
+
+/** see {@link #localDefaultResolvesWithAncestorValue} */
+public boolean getLocalDefaultResolvesWithAncestorValue() {
+if (localDefaultResolvesWithAncestorValue==null) {
+// in case some legacy path is using an improperly 
deserialized object
+log.warn("Encountered legacy "+this+" with null 
localDefaultResolvesWithAncestorValue; transforming", new Throwable("stack 
trace for legacy "+this));
+readResolve();
+}
+return localDefaultResolvesWithAncestorValue;
+}
+
+public boolean getAncestorDefaultInheritable() {
+if (ancestorDefaultInheritable==null) {
+log.warn("Encountered legacy "+this+" with null 
ancestorDefaultInheritable; transforming", new Throwable("stack trace for 
legacy "+this));
+readResolve();
+}
+return ancestorDefaultInheritable;
 }
 
+// standard deserialization method
+private ConfigInheritance readResolve() {
+try {
+if (useLocalDefaultValue!=null) {
+// move away from useLocalDefaultValue to 
localDefaultResolvesWithAncestorValue
+
+Field fNew = 
getClass().getDeclaredField("localDefaultResolvesWithAncestorValue");
+fNew.setAccessible(true);
+Field fOld = 
getClass().getDeclaredField("useLocalDefaultValue");
+fOld.setAccessible(true);
+
+if (fNew.get(this)==null) {
+fNew.set(this, useLocalDefaultValue);
+} else {
+if (!fNew.get(this).equals(useLocalDefaultValue)) {
+throw new IllegalStateException("Incompatible 
values detected for "+fOld+" ("+fOld.get(this)+") and "+fNew+" 
("+fNew.get(this)+")");
+}
+}
+fOld.set(this, null);
+}
+
+if (ancestorDefaultInheritable==null) {
+Field f = 
getClass().getDeclaredField("ancestorDefaultInheritable");
+f.setAccessible(true);
+f.set(this, true);
+}
+
+} catch (Exception e) {
+throw Exceptions.propagate(e);
+}
+
+for (ConfigInheritance knownMode: Arrays.asList(
+NOT_REINHERITED, NOT_REINHERITED_ELSE_DEEP_MERGE, 
NEVER_INHERITED, OVERWRITE, DEEP_MERGE)) {
+if (equals(knownMode)) return knownMode;
--- End diff --

you're right, adding a `protected readResolve` on the 
`DelegatingConfigInheritance`

(i'm re-reviewing this after #440)


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r89772773
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java
 ---
@@ -0,0 +1,145 @@
+/*
+ * 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 org.apache.brooklyn.core.mgmt.rebind;
+
+import java.io.File;
+import java.io.FileReader;
+
+import org.apache.brooklyn.api.entity.Application;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.config.ConfigInheritance;
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.BasicConfigInheritance;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext;
+import org.apache.brooklyn.core.config.ConfigPredicates;
+import org.apache.brooklyn.core.entity.EntityAsserts;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.stream.Streams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
+import com.google.common.collect.Iterables;
+import com.google.common.io.Files;
+
+public class RebindConfigInheritanceTest extends RebindTestFixtureWithApp {
+
+private static final Logger log = 
LoggerFactory.getLogger(RebindConfigInheritanceTest.class);
+
+ConfigKey key1 = ConfigKeys.builder(String.class, 
"key1").runtimeInheritance(BasicConfigInheritance.NEVER_INHERITED).build();
+String origMemento, newMemento;
+Application rebindedApp;
+
+@Override
+@BeforeMethod(alwaysRun=true)
+public void setUp() throws Exception {
+super.setUp();
+}
+
+protected void doReadConfigInheritance(String label, String entityId) 
throws Exception {
+String mementoFilename = "config-inheritance-"+label+"-"+entityId;
+origMemento = 
Streams.readFullyString(getClass().getResourceAsStream(mementoFilename));
+
+File persistedEntityFile = new File(mementoDir, 
Os.mergePaths("entities", entityId));
+Files.write(origMemento.getBytes(), persistedEntityFile);
+
+// we'll make assertions on what we've loaded
+rebind();
+rebindedApp = (Application) newManagementContext.lookup(entityId);
+
+// we'll also make assertions on the contents written out
+RebindTestUtils.waitForPersisted(mgmt());
+newMemento = 
Joiner.on("\n").join(Files.readLines(persistedEntityFile, Charsets.UTF_8));
+}
+
+/** also see {@link RebindWithDeserializingClassRenamesTest} */
+@Test
+public void testPreBasicConfigInheritance_2016_07() throws Exception {
+doReadConfigInheritance("prebasic-2016-07", "toruf2wxg4");
+
+ConfigKey k = Iterables.getOnlyElement( 
rebindedApp.config().findKeys(ConfigPredicates.nameEqualTo("my.config.inheritanceMerged"))
 );
+
+Asserts.assertStringContains(origMemento, "");
+Asserts.assertStringDoesNotContain(origMemento, 
BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+// should now convert it to BasicConfigInheritance.DEEP_MERGE
+Asserts.assertStringDoesNotContain(newMemento, 
"ConfigInheritance$Merged");
+Asserts.assertStringDoesNotContain(newMemento, 
"ConfigInheritance$Legacy$Merged");
+Asserts.assertStringContains(newMemento, 
BasicConfigInheritance.DEEP_MERGE.getClass().getName());
+
+ConfigInheritance inh = 
k.getInheritanceByContext(InheritanceContext.RUNTIME_MANAGEMENT);
+   

[GitHub] brooklyn-server pull request #462: Inherit config default values

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/462#discussion_r89772102
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
 ---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map nameToType) {
 
 @Override
 public Class realClass(String elementName) {
-Optional elementNameOpt = 
Reflections.tryFindMappedName(nameToType, elementName);
+Maybe elementNameOpt = 
Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --

Why rename this? It was previously following the same naming conventions as 
guava's `Iterables` (e.g. `tryFind` versus `find`).

Also, if changing it to a `Maybe` then should we explicitly check for it 
returning `Maybe.of(null)`, rather than risking a NPE later?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

Github user ahgittin closed the pull request at:

https://github.com/apache/brooklyn-server/pull/462


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

GitHub user ahgittin reopened a pull request:

https://github.com/apache/brooklyn-server/pull/462

Inherit config default values

resolve https://issues.apache.org/jira/browse/BROOKLYN-267

also during deserializing migrate to non-deprecated classes

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ahgittin/brooklyn-server 
inherit-config-default-values

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/brooklyn-server/pull/462.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #462


commit 9fb6f35e8b2c694168c06a39b7fcc922b0ece8fc
Author: Alex Heneveld 
Date:   2016-11-24T10:56:46Z

migrate config inheritance to new classes

pioneers use of `readResolve` that actually works brilliantly out of the 
box due to xstream

also tidying `BasicConfigInheritance` and adding a placeholder (not used 
yet)
for resolving ancestor defaults

includes tests for config inheritance serialization

commit 1d5cafca0195275758518e7bba3541ff29298c89
Author: Alex Heneveld 
Date:   2016-11-24T16:29:21Z

document better config inheritance semantics

commit 79bb11957be95e525f126d9c7c301635a9fa6404
Author: Alex Heneveld 
Date:   2016-11-17T15:12:09Z

implement inheritance of config default values

test and code changes to respect the additional inheritance argument added 
in the previous request:
resolves https://issues.apache.org/jira/browse/BROOKLYN-267

commit 2bb1346ee59d0b1c3bb8ec638f38af1cf5a62e9e
Author: Alex Heneveld 
Date:   2016-11-24T17:34:10Z

uncomment another assertion about inherited config




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #462: Inherit config default values

[ahgittin]

GitHub user ahgittin opened a pull request:

https://github.com/apache/brooklyn-server/pull/462

Inherit config default values

resolve https://issues.apache.org/jira/browse/BROOKLYN-267

also during deserializing migrate to non-deprecated classes

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ahgittin/brooklyn-server 
inherit-config-default-values

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/brooklyn-server/pull/462.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #462


commit 9fb6f35e8b2c694168c06a39b7fcc922b0ece8fc
Author: Alex Heneveld 
Date:   2016-11-24T10:56:46Z

migrate config inheritance to new classes

pioneers use of `readResolve` that actually works brilliantly out of the 
box due to xstream

also tidying `BasicConfigInheritance` and adding a placeholder (not used 
yet)
for resolving ancestor defaults

includes tests for config inheritance serialization

commit 1d5cafca0195275758518e7bba3541ff29298c89
Author: Alex Heneveld 
Date:   2016-11-24T16:29:21Z

document better config inheritance semantics

commit 79bb11957be95e525f126d9c7c301635a9fa6404
Author: Alex Heneveld 
Date:   2016-11-17T15:12:09Z

implement inheritance of config default values

test and code changes to respect the additional inheritance argument added 
in the previous request:
resolves https://issues.apache.org/jira/browse/BROOKLYN-267




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---