This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit 3718c1e19021ced482ff53b4536ea0c00182f4ee Author: Alex Heneveld <[email protected]> AuthorDate: Thu Dec 1 14:09:26 2022 +0000 ensure rebinding does not mangle/coerce config values --- .../brooklyn/CustomTypeConfigYamlRebindTest.java | 176 +++++++++++++++++++++ .../config/internal/AbstractConfigMapImpl.java | 8 + .../core/mgmt/rebind/BasicEntityRebindSupport.java | 17 +- .../objs/AbstractConfigurationSupportInternal.java | 5 + .../brooklyn/core/objs/BrooklynObjectInternal.java | 4 + .../rebind/RebindManagerExceptionHandlerTest.java | 66 ++++++-- ...klynRegisteredTypeJacksonSerializationTest.java | 29 ++-- 7 files changed, 276 insertions(+), 29 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/CustomTypeConfigYamlRebindTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/CustomTypeConfigYamlRebindTest.java new file mode 100644 index 0000000000..ea464d641e --- /dev/null +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/CustomTypeConfigYamlRebindTest.java @@ -0,0 +1,176 @@ +/* + * 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.camp.brooklyn; + +import com.google.common.collect.Iterables; +import org.apache.brooklyn.api.entity.Application; +import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.camp.brooklyn.spi.dsl.DslUtils; +import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.config.MapConfigKey; +import org.apache.brooklyn.core.objs.BrooklynObjectInternal; +import org.apache.brooklyn.core.resolve.jackson.BrooklynRegisteredTypeJacksonSerializationTest.SampleBean; +import org.apache.brooklyn.core.sensor.Sensors; +import org.apache.brooklyn.core.test.entity.TestEntityImpl; +import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.collections.MutableSet; +import org.apache.brooklyn.util.text.Strings; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.annotations.Test; + +import java.util.Map; + +@Test +public class CustomTypeConfigYamlRebindTest extends AbstractYamlRebindTest { + private static final Logger log = LoggerFactory.getLogger(CustomTypeConfigYamlRebindTest.class); + private Entity lastDeployedEntity; + + public static class EntityWithCustomTypeConfig extends TestEntityImpl { + public static final MapConfigKey<SampleBean> CUSTOM_TYPE_KEY = new MapConfigKey.Builder(SampleBean.class, "customTypeKey").build(); + } + + @Test + public void testCustomTypeKeyRebind() throws Exception { + Application app = (Application) createAndStartApplication(Strings.lines( + "services:", + "- type: " + EntityWithCustomTypeConfig.class.getName(), + " brooklyn.config:", + " " + EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.getName() + ":", + " i1: { x: hello }" + )); + makeChildCustomTypeAssertions(app); + + app = rebind(); + // prior to 2022-12-01 rebind would coerce the config when setting it, changing what is actually stored in subtle ways; + // also it could manifest errors if the config was invalid + makeChildCustomTypeAssertions(app); + } + + private void makeChildCustomTypeAssertions(Application app) { + Entity child = Iterables.getOnlyElement(app.getChildren()); + Map<String, SampleBean> vm = child.config().get(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY); + Asserts.assertEquals(vm.get("i1").x, "hello"); + + Object vmu = ((BrooklynObjectInternal.ConfigurationSupportInternal) child.config()).getRaw(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY).get(); + Asserts.assertInstanceOf(vmu, Map.class); + Asserts.assertInstanceOf( ((Map)vmu).get("i1"), Map.class); + } + + @Test + public void testCustomTypeKeyBadlyFormedButUsingAttributeWhenReady() throws Exception { + Application app = (Application) createAndStartApplication(Strings.lines( + "services:", + "- type: " + EntityWithCustomTypeConfig.class.getName(), + " brooklyn.config:", + " " + EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.getName() + ":", + " i1: { x: hello, xx: $brooklyn:attributeWhenReady(\"set-later\") }" + )); + + { + Entity child = Iterables.getOnlyElement(app.getChildren()); + // and we can manually set the map, validation is not deep + child.config().set((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY, + MutableMap.of("i2", MutableMap.of("x", "hi", "xx", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:attributeWhenReady(\"set-later\")")))); + // but cannot set a specific subkey, even untyped + Asserts.assertFailsWith( + () -> + child.config().set((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.subKey("i3"), + MutableMap.of("x", "hi", "xx", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:attributeWhenReady(\"set-later\")"))), + e -> Asserts.expectedFailureContains(e, "Cannot coerce or set", "Unrecognized field", "xx", "SampleBean")); + + // above can be deployed, and will block during startup because unready attribute reference means its type coercion isn't attempted so doesn't fail + // but rebinding it threw an error prior to 2022-12-01 because BasicEntityRebindSupport.addConfig tries to coerce and validate; + // now it is fine + app = rebind(); + } + + { + // but throws when we get, of course (once sensor is set) + Entity child = Iterables.getOnlyElement(app.getChildren()); + child.sensors().set(Sensors.newStringSensor("set-later"), "now-set"); + + Asserts.assertFailsWith( + () -> child.config().get(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY), + e -> Asserts.expectedFailureContains(e, "Cannot resolve", "Unrecognized field", "xx", "SampleBean")); + } + } + + @Test + public void testCustomTypeKeyUsingAttributeWhenReadyForStringValue() throws Exception { + Application app = (Application) createAndStartApplication(Strings.lines( + "services:", + "- type: " + EntityWithCustomTypeConfig.class.getName(), + " brooklyn.config:", + " " + EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.getName() + ":", + " i1: { x: $brooklyn:attributeWhenReady(\"set-later\") }" + )); + + { + Entity child = Iterables.getOnlyElement(app.getChildren()); + child.sensors().set(Sensors.newStringSensor("set-later"), "now-set"); + + // allowed to set as above + + // and can get, as map or bean + Map<String, SampleBean> map = child.config().get(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY); + Asserts.assertEquals(map.keySet(), MutableSet.of("i1")); + Asserts.assertEquals(map.get("i1").x, "now-set"); + + Object bean = child.config().get((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.subKey("i1")); + Asserts.assertEquals(((SampleBean) bean).x, "now-set"); + + // but not permitted to set a DSL expression to a string value at top level + Asserts.assertFailsWith( + () -> + child.config().set((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.subKey("i2"), + MutableMap.of("x", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:attributeWhenReady(\"set-later\")"))), + e -> Asserts.expectedFailureContains(e, "Cannot deserialize value", "String", "from Object")); + + // but is allowed at map level (is additive) + child.config().set((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY, + MutableMap.of("i3", MutableMap.of("x", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:attributeWhenReady(\"set-later\")")))); + + // and again, works for access to child (top-level fields are resolved?) -- not sure why, might not keep + map = child.config().get(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY); + Asserts.assertEquals(map.keySet(), MutableSet.of("i1", "i3")); + Asserts.assertEquals(map.get("i3").x, "now-set"); + + bean = child.config().get((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.subKey("i3")); + Asserts.assertEquals(((SampleBean) bean).x, "now-set"); + } + + app = rebind(); + + { + // and values available after rebind + Entity child = Iterables.getOnlyElement(app.getChildren()); + child.sensors().set(Sensors.newStringSensor("set-later"), "now-set"); + + Map<String, SampleBean> map = child.config().get(EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY); + Asserts.assertEquals(map.keySet(), MutableSet.of("i1", "i3")); + Asserts.assertEquals(map.get("i1").x, "now-set"); + Asserts.assertEquals(map.get("i3").x, "now-set"); + + Object bean = child.config().get((ConfigKey) EntityWithCustomTypeConfig.CUSTOM_TYPE_KEY.subKey("i1")); + Asserts.assertEquals(((SampleBean) bean).x, "now-set"); + } + } +} diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java index 39edafee24..1c0ce763fe 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java @@ -199,6 +199,14 @@ public abstract class AbstractConfigMapImpl<TContainer extends BrooklynObject> i } } + public void setRaw(ConfigKey<?> key, boolean preferContainerKey, Object value) { + synchronized (ownConfig) { + ConfigKey<?> ownKey = preferContainerKey ? getKeyAtContainer(getContainer(), key) : null; + if (ownKey==null) ownKey = key; + ownConfig.put(key, value); + } + } + @SuppressWarnings("unchecked") public void putAll(Map<?,?> vals) { for (Map.Entry<?, ?> entry : vals.entrySet()) { diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java index c4f7ba9a74..580a5ef326 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/BasicEntityRebindSupport.java @@ -42,6 +42,7 @@ import org.apache.brooklyn.core.entity.trait.AsyncStartable; import org.apache.brooklyn.core.feed.AbstractFeed; import org.apache.brooklyn.core.location.Machines; import org.apache.brooklyn.core.objs.AbstractBrooklynObject; +import org.apache.brooklyn.core.objs.BrooklynObjectInternal; import org.apache.brooklyn.core.policy.AbstractPolicy; import org.apache.brooklyn.entity.group.AbstractGroupImpl; import org.apache.brooklyn.util.exceptions.Exceptions; @@ -100,10 +101,18 @@ public class BasicEntityRebindSupport extends AbstractBrooklynObjectRebindSuppor try { key = entry.getKey(); Object value = entry.getValue(); - @SuppressWarnings("unused") // just to ensure we can load the declared type? or maybe not needed - // In what cases key.getType() will be null? - Class<?> type = (key.getType() != null) ? key.getType() : rebindContext.loadClass(key.getTypeName()); - entity.config().set((ConfigKey<Object>)key, value); + + Class<?> type = null; + try { + // just to warn if we cannot find the type; probably type is never null so not a problem here, rebind would have thrown earlier + type = (key.getType() != null) ? key.getType() : rebindContext.loadClass(key.getTypeName()); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + LOG.warn("Unable to find type of key "+key.getName()+" in "+memento+"; proceeding, but errors may occur when using"); + } + + entity.config().setRaw(key, type==null || type.equals(Object.class) ? true : false, value); + } catch (Exception e) { Exceptions.propagateIfFatal(e); rebindContext.getExceptionHandler().onAddConfigFailed(memento, key, e); diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java index 9656be85c7..ff56c293fe 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/AbstractConfigurationSupportInternal.java @@ -215,6 +215,11 @@ public abstract class AbstractConfigurationSupportInternal implements BrooklynOb return getConfigsInternal().getConfigLocalRaw(key); } + @Override + public void setRaw(ConfigKey<?> key, boolean preferContainerKey, Object value) { + getConfigsInternal().setRaw(key, preferContainerKey, value); + } + @Override public void putAll(Map<?, ?> vals) { getConfigsInternal().putAll(vals); diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java index c4f572c7e7..56128442d8 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BrooklynObjectInternal.java @@ -121,6 +121,10 @@ public interface BrooklynObjectInternal extends BrooklynObject, Rebindable { @Beta Maybe<Object> getLocalRaw(ConfigKey<?> key); + /** Sets a value, with no coercion or chedcking and no notifications */ + @Beta + void setRaw(ConfigKey<?> key, boolean preferContainerKey, Object value); + /** * @see {@link #getLocalRaw(ConfigKey)} */ diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindManagerExceptionHandlerTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindManagerExceptionHandlerTest.java index 3eaec9c917..3fba5f06e3 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindManagerExceptionHandlerTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindManagerExceptionHandlerTest.java @@ -18,11 +18,23 @@ */ package org.apache.brooklyn.core.mgmt.rebind; +import com.thoughtworks.xstream.annotations.XStreamConverter; +import com.thoughtworks.xstream.converters.Converter; +import com.thoughtworks.xstream.converters.ConverterLookup; +import com.thoughtworks.xstream.converters.MarshallingContext; +import com.thoughtworks.xstream.converters.UnmarshallingContext; +import com.thoughtworks.xstream.core.ClassLoaderReference; +import com.thoughtworks.xstream.io.HierarchicalStreamReader; +import com.thoughtworks.xstream.io.HierarchicalStreamWriter; +import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.core.config.ConfigKeys; import org.apache.brooklyn.core.entity.EntityAsserts; import org.apache.brooklyn.core.test.entity.TestApplication; +import org.apache.brooklyn.core.test.entity.TestApplicationNoEnrichersImpl; import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.core.task.Tasks.ForTestingAndLegacyCompatibilityOnly.LegacyDeepResolutionMode; import org.apache.brooklyn.util.exceptions.Exceptions; @@ -30,41 +42,67 @@ import org.testng.annotations.Test; import com.google.common.collect.ImmutableMap; +import java.util.Arrays; +import java.util.stream.Collectors; + public class RebindManagerExceptionHandlerTest extends RebindTestFixtureWithApp { + @XStreamConverter(BadConverter.class) + static class NotDeserializable { + } + public static class BadConverter implements Converter { + public BadConverter(ConverterLookup lookup, ClassLoaderReference loader) { + } + + @Override + public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingContext context) { + } + + @Override + public Object unmarshal(HierarchicalStreamReader hierarchicalStreamReader, UnmarshallingContext unmarshallingContext) { + throw new IllegalStateException("Deliberate failure"); + } + + @Override + public boolean canConvert(Class type) { + return NotDeserializable.class.equals(type); + } + } + @Override protected TestApplication createApp() { TestApplication app = super.createApp(); - try { - Tasks.ForTestingAndLegacyCompatibilityOnly.withLegacyDeepResolutionMode(LegacyDeepResolutionMode.ALLOW_LEGACY, () -> - app.createAndManageChild(EntitySpec.create(TestEntity.class) - .configure(TestEntity.CONF_MAP_THING.getName(), - // misconfigured map value, should be a string key, but forced (by using a flag) so failure won't be enforced until persist/rebind - ImmutableMap.of("keyWithMapValue", ImmutableMap.of("minRam", 4))))); - } catch (Exception e) { - throw Exceptions.propagate(e); - } + app.config().set(TestEntity.CONF_OBJECT, new NotDeserializable()); return app; } @Test - public void testAddConfigFailure() throws Throwable { + public void testRebindFailFailure() throws Throwable { try { RebindOptions rebindOptions = RebindOptions.create().defaultExceptionHandler(); rebind(rebindOptions); Asserts.shouldHaveFailedPreviously(); } catch (Throwable e) { - Asserts.expectedFailureContainsIgnoreCase(e, "minRam=4", "keyWithMapValue"); + Asserts.expectedFailureContainsIgnoreCase(e, "Deliberate failure"); } } @Test - public void testAddConfigContinue() throws Throwable { + public void testRebindFailContinue() throws Throwable { + TestApplication app2 = super.createApp(); + Asserts.assertEquals(mgmt().getEntityManager().getEntities().stream().map(Entity::getId).collect(Collectors.toSet()), MutableSet.of(app().getId(), app2.getId())); + RebindOptions rebindOptions = RebindOptions.create() .defaultExceptionHandler() - .additionalProperties(ImmutableMap.of("rebind.failureMode.addConfig", "continue")); + .additionalProperties(ImmutableMap.of( + // prior to 2022-12 we tested ADD_CONFIG handler, but now that never throws (we don't coerce on rebind) + RebindManagerImpl.REBIND_FAILURE_MODE.getName(), "continue")); TestApplication rebindedApp = rebind(rebindOptions); - EntityAsserts.assertConfigEquals(rebindedApp, TestEntity.CONF_MAP_THING, null); + + // app1 not rebinded due to failure + + Asserts.assertEquals(mgmt().getEntityManager().getEntities().stream().map(Entity::getId).collect(Collectors.toSet()), MutableSet.of(app2.getId())); + Asserts.assertEquals(rebindedApp.getId(), app2.getId()); } } diff --git a/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java b/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java index be93620f84..218bc784c4 100644 --- a/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java @@ -160,34 +160,41 @@ public class BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmt Object a2 = deser(deser); Asserts.assertInstanceOf(a2, SampleBeanWithType.class); - Asserts.assertEquals( ((SampleBeanWithType)a2).x, "hello" ); - Asserts.assertEquals( ((SampleBeanWithType)a2).type, "T" ); + Asserts.assertEquals(((SampleBeanWithType) a2).x, "hello"); + Asserts.assertEquals(((SampleBeanWithType) a2).type, "T"); deser = Strings.replaceAllNonRegex(deser, "!", "@"); // @type accepted a2 = deser(deser); Asserts.assertInstanceOf(a2, SampleBeanWithType.class); - Asserts.assertEquals( ((SampleBeanWithType)a2).x, "hello" ); - Asserts.assertEquals( ((SampleBeanWithType)a2).type, "T" ); + Asserts.assertEquals(((SampleBeanWithType) a2).x, "hello"); + Asserts.assertEquals(((SampleBeanWithType) a2).type, "T"); a.type = null; - deser = "{\"(type)\":\""+SampleBeanWithType.class.getName()+"\",\"x\":\"hello\"}"; + deser = "{\"(type)\":\"" + SampleBeanWithType.class.getName() + "\",\"x\":\"hello\"}"; Assert.assertEquals(ser(a), deser); - Asserts.assertEquals( ((SampleBeanWithType) deser(deser)).x, "hello" ); - Asserts.assertNull( ((SampleBeanWithType) deser(deser)).type ); + Asserts.assertEquals(((SampleBeanWithType) deser(deser)).x, "hello"); + Asserts.assertNull(((SampleBeanWithType) deser(deser)).type); // if type (not (type)) is first it treats it as the type, but warning about ambiguity deser = Strings.replaceAllNonRegex(deser, "!", ""); - Asserts.assertEquals( ((SampleBeanWithType) deser(deser)).x, "hello" ); - Asserts.assertNull( ((SampleBeanWithType) deser(deser)).type ); + Asserts.assertEquals(((SampleBeanWithType) deser(deser)).x, "hello"); + Asserts.assertNull(((SampleBeanWithType) deser(deser)).type); // if type is not first and refers to a class with a field 'type`, it treats it as a plain vanilla object (map) - deser = "{\"x\":\"hello\",\"type\":\""+SampleBeanWithType.class.getName()+"\"}"; + deser = "{\"x\":\"hello\",\"type\":\"" + SampleBeanWithType.class.getName() + "\"}"; Asserts.assertInstanceOf(deser(deser), Map.class); // but for a bean without a field 'type', that is _not_ the case (but might change that in future) - deser = "{\"x\":\"hello\",\"type\":\""+SampleBean.class.getName()+"\"}"; + deser = "{\"x\":\"hello\",\"type\":\"" + SampleBean.class.getName() + "\"}"; Asserts.assertInstanceOf(deser(deser), SampleBean.class); } + @Test + public void testDeserializeSampleBeanWithTooMuch() throws Exception { + String deser = "{\"(type)\":\"" + SampleBeanWithType.class.getName() + "\",\"x\":\"hello\",\"xx\":\"not_supported\"}"; + Asserts.assertFailsWith(() -> deser(deser), + e -> Asserts.expectedFailureContainsIgnoreCase(e, "unrecognized field", "xx")); + } + @Test public void testDeserializeEntityInitializerWithTypeField() throws Exception { BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class);
