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 dd89910600c41b65cc17050bcd27a621da02f9ca Author: Alex Heneveld <[email protected]> AuthorDate: Thu May 25 15:27:00 2023 +0100 stricter coercion to maps previously allows "x" to be the {x: null} map; this has been removed unless there is at least one colon, e.g. 'key: value, key2' still allowed also tidies where we thought 'key = value' would be parsed as a map but it wasn't due to test artifacts --- .../workflow/steps/variables/TransformMerge.java | 2 + .../util/core/internal/TypeCoercionsTest.java | 98 +++++++++++++--------- .../coerce/CommonAdaptorTypeCoercions.java | 15 +++- .../util/javalang/coerce/TypeCoercionsTest.java | 35 ++------ 4 files changed, 80 insertions(+), 70 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/steps/variables/TransformMerge.java b/core/src/main/java/org/apache/brooklyn/core/workflow/steps/variables/TransformMerge.java index aabc425179..ea15cc6f90 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/steps/variables/TransformMerge.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/steps/variables/TransformMerge.java @@ -29,6 +29,7 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.text.QuotedStringTokenizer; +import org.apache.brooklyn.util.text.Strings; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -104,6 +105,7 @@ public class TransformMerge extends WorkflowTransformDefault { }; for (Object vi : (Iterable) v) { + if (vi instanceof String && Strings.isBlank((String)vi)) continue; try { vi = wait ? context.resolveWaiting(WorkflowExpressionResolution.WorkflowExpressionStage.STEP_RUNNING, vi, TypeToken.of(type)) : context.resolve(WorkflowExpressionResolution.WorkflowExpressionStage.STEP_RUNNING, vi, TypeToken.of(type)); diff --git a/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java index 32167362c1..38e40ae234 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java @@ -20,9 +20,6 @@ package org.apache.brooklyn.util.core.internal; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableList; -import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertNull; -import static org.testng.Assert.assertTrue; import java.math.BigDecimal; import java.math.BigInteger; @@ -41,7 +38,9 @@ import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.ClassLoaderUtils; import org.apache.brooklyn.util.core.flags.TypeCoercions; +import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.javalang.coerce.ClassCoercionException; +import org.apache.brooklyn.util.javalang.coerce.CommonAdaptorTypeCoercions; import org.apache.brooklyn.util.text.StringPredicates; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -56,10 +55,25 @@ import com.google.common.reflect.TypeToken; import groovy.lang.GString; +import static org.testng.Assert.*; + public class TypeCoercionsTest { private static final Logger log = LoggerFactory.getLogger(TypeCoercionsTest.class); - + + // largely a duplicate of upstream common coerce/TypeCoercionsTest + // but with a few extras; ideally would be reconciled + + private static void assertMapsEqual(Map actual, Map expected) { + Assert.assertEquals(actual, expected); + + // workaround for bug in testng assert when values are null + // PR https://github.com/testng-team/testng/pull/2914 opened, hopefully included in testng 7.9 then this can be removed + if (actual instanceof Map && expected instanceof Map) { + assertEquals( ((Map)actual).keySet(), ((Map)expected).keySet(), "Keys in map differ: "+actual.keySet()+" / "+expected.keySet() ); + } + } + @Test public void testCoerceCharSequenceToString() { assertEquals(TypeCoercions.coerce(new StringBuilder("abc"), String.class), "abc"); @@ -156,7 +170,7 @@ public class TypeCoercionsTest { assertEquals(TypeCoercions.coerce(BigInteger.ONE, int.class), (Integer)1); assertEquals(TypeCoercions.coerce(BigInteger.valueOf(Long.MAX_VALUE), Long.class), (Long)Long.MAX_VALUE); assertEquals(TypeCoercions.coerce(BigInteger.valueOf(Long.MAX_VALUE), long.class), (Long)Long.MAX_VALUE); - + assertEquals(TypeCoercions.coerce(BigDecimal.valueOf(0.5), Double.class), 0.5d, 0.00001d); assertEquals(TypeCoercions.coerce(BigDecimal.valueOf(0.5), double.class), 0.5d, 0.00001d); } @@ -195,13 +209,13 @@ public class TypeCoercionsTest { @Test public void testListToSetCoercion() { Set<?> s = TypeCoercions.coerce(ImmutableList.of(1), Set.class); - Assert.assertEquals(s, ImmutableSet.of(1)); + assertEquals(s, ImmutableSet.of(1)); } @Test public void testSetToListCoercion() { List<?> s = TypeCoercions.coerce(ImmutableSet.of(1), List.class); - Assert.assertEquals(s, ImmutableList.of(1)); + assertEquals(s, ImmutableList.of(1)); } @Test @@ -223,41 +237,41 @@ public class TypeCoercionsTest { public void testListEntryCoercion() { @SuppressWarnings("serial") List<?> s = TypeCoercions.coerce(ImmutableList.of("java.lang.Integer", "java.lang.Double"), new TypeToken<List<Class<?>>>() { }); - Assert.assertEquals(s, ImmutableList.of(Integer.class, Double.class)); + assertEquals(s, ImmutableList.of(Integer.class, Double.class)); } @Test public void testListEntryToSetCoercion() { @SuppressWarnings("serial") Set<?> s = TypeCoercions.coerce(ImmutableList.of("java.lang.Integer", "java.lang.Double"), new TypeToken<Set<Class<?>>>() { }); - Assert.assertEquals(s, ImmutableSet.of(Integer.class, Double.class)); + assertEquals(s, ImmutableSet.of(Integer.class, Double.class)); } @Test public void testListEntryToCollectionCoercion() { @SuppressWarnings("serial") Collection<?> s = TypeCoercions.coerce(ImmutableList.of("java.lang.Integer", "java.lang.Double"), new TypeToken<Collection<Class<?>>>() { }); - Assert.assertEquals(s, ImmutableList.of(Integer.class, Double.class)); + assertEquals(s, ImmutableList.of(Integer.class, Double.class)); } @Test public void testMapValueCoercion() { @SuppressWarnings("serial") Map<?,?> s = TypeCoercions.coerce(ImmutableMap.of("int", "java.lang.Integer", "double", "java.lang.Double"), new TypeToken<Map<String, Class<?>>>() { }); - Assert.assertEquals(s, ImmutableMap.of("int", Integer.class, "double", Double.class)); + assertMapsEqual(s, ImmutableMap.of("int", Integer.class, "double", Double.class)); } @Test public void testMapKeyCoercion() { @SuppressWarnings("serial") Map<?,?> s = TypeCoercions.coerce(ImmutableMap.of("java.lang.Integer", "int", "java.lang.Double", "double"), new TypeToken<Map<Class<?>, String>>() { }); - Assert.assertEquals(s, ImmutableMap.of(Integer.class, "int", Double.class, "double")); + assertMapsEqual(s, ImmutableMap.of(Integer.class, "int", Double.class, "double")); } @Test public void testStringToListCoercion() { List<?> s = TypeCoercions.coerce("a,b,c", List.class); - Assert.assertEquals(s, ImmutableList.of("a", "b", "c")); + assertEquals(s, ImmutableList.of("a", "b", "c")); } @Test @@ -269,97 +283,105 @@ public class TypeCoercionsTest { @Test public void testJsonStringToMapCoercion() { Map<?,?> s = TypeCoercions.coerce("{ \"a\" : \"1\", b : 2 }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); + assertMapsEqual(s, ImmutableMap.of("a", "1", "b", 2)); } @Test public void testJsonStringWithoutQuotesToMapCoercion() { Map<?,?> s = TypeCoercions.coerce("{ a : 1 }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", 1)); + assertMapsEqual(s, ImmutableMap.of("a", 1)); } @Test public void testJsonComplexTypesToMapCoercion() { Map<?,?> s = TypeCoercions.coerce("{ a : [1, \"2\", '\"3\"'], b: { c: d, 'e': \"f\" } }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", ImmutableList.<Object>of(1, "2", "\"3\""), + assertMapsEqual(s, ImmutableMap.of("a", ImmutableList.<Object>of(1, "2", "\"3\""), "b", ImmutableMap.of("c", "d", "e", "f"))); } @Test public void testJsonStringWithoutBracesToMapCoercion() { Map<?,?> s = TypeCoercions.coerce("a : 1", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", 1)); + assertMapsEqual(s, ImmutableMap.of("a", 1)); } @Test public void testJsonStringWithoutBracesWithMultipleToMapCoercion() { Map<?,?> s = TypeCoercions.coerce("a : 1, b : 2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", 1, "b", 2)); + assertMapsEqual(s, ImmutableMap.of("a", 1, "b", 2)); } - @Test + @Test(enabled = false) //never actually worked, only seemed to because of bug in assertEquals(Map,Map) public void testKeyEqualsValueStringToMapCoercion() { + if (!CommonAdaptorTypeCoercions.PARSE_MAPS_WITH_EQUALS_SYMBOL) Assert.fail("Known to be unsupported"); Map<?,?> s = TypeCoercions.coerce("a=1,b=2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", "2")); + assertMapsEqual(s, ImmutableMap.of("a", "1", "b", "2")); } @Test public void testJsonStringWithoutBracesOrSpaceDisallowedAsMapCoercion() { - Map<?,?> s = TypeCoercions.coerce("a:1,b:2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", "2")); - // NB: snakeyaml 1.17 required spaces after the colon, but 1.21 accepts the above + Maybe<Map> s1 = TypeCoercions.tryCoerce("a:1,b:2", Map.class); + // NB: snakeyaml 1.17 required spaces after the colon, 1.21 accepts the above, but we explicitly disallow it + // (mileage may vary if you do something like a:1,b=2) + if (s1.isPresent()) Asserts.fail("Shouldn't allow colon without space when processing map; instead got: "+s1.get()); + + Map<?,?> s = TypeCoercions.coerce("a: 1,b: 2", Map.class); + assertMapsEqual(s, ImmutableMap.of("a", 1, "b", 2)); } - - @Test - public void testEqualsInBracesMapCoercion() { + + @Test(enabled = false) //never actually worked, only seemed to because of bug in assertEquals(Map,Map) + public void testEqualsInBracesMapCoercionLax() { + if (!CommonAdaptorTypeCoercions.PARSE_MAPS_WITH_EQUALS_SYMBOL) Assert.fail("Known to be unsupported"); Map<?,?> s = TypeCoercions.coerce("{ a = 1, b = '2' }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", 1, "b", "2")); + assertMapsEqual(s, ImmutableMap.of("a", 1, "b", "2")); } - @Test + @Test(enabled = false) //never actually worked, only seemed to because of bug in assertEquals(Map,Map) public void testKeyEqualsOrColonValueWithBracesStringToMapCoercion() { + if (!CommonAdaptorTypeCoercions.PARSE_MAPS_WITH_EQUALS_SYMBOL) Assert.fail("Known to be unsupported"); Map<?,?> s = TypeCoercions.coerce("{ a=1, b: 2 }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); + assertMapsEqual(s, ImmutableMap.of("a", "1", "b", 2)); } - @Test + @Test(enabled = false) //never actually worked, only seemed to because of bug in assertEquals(Map,Map) public void testKeyEqualsOrColonValueWithoutBracesStringToMapCoercion() { + if (!CommonAdaptorTypeCoercions.PARSE_MAPS_WITH_EQUALS_SYMBOL) Assert.fail("Known to be unsupported"); Map<?,?> s = TypeCoercions.coerce("a=1, b: 2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); + assertMapsEqual(s, ImmutableMap.of("a", "1", "b", 2)); } @SuppressWarnings("serial") @Test public void testYamlMapsDontGoTooFarWhenWantingListOfString() { List<?> s = TypeCoercions.coerce("[ a: 1, b: 2 ]", List.class); - Assert.assertEquals(s, ImmutableList.of(MutableMap.of("a", 1), MutableMap.of("b", 2))); + assertEquals(s, ImmutableList.of(MutableMap.of("a", 1), MutableMap.of("b", 2))); s = TypeCoercions.coerce("[ a: 1, b : 2 ]", new TypeToken<List<String>>() {}); - Assert.assertEquals(s, ImmutableList.of("a: 1", "b : 2")); + assertEquals(s, ImmutableList.of("a: 1", "b : 2")); } @Test public void testURItoStringCoercion() { String s = TypeCoercions.coerce(URI.create("http://localhost:1234/";), String.class); - Assert.assertEquals(s, "http://localhost:1234/";); + assertEquals(s, "http://localhost:1234/";); } @Test public void testURLtoStringCoercion() throws MalformedURLException { String s = TypeCoercions.coerce(new URL("http://localhost:1234/";), String.class); - Assert.assertEquals(s, "http://localhost:1234/";); + assertEquals(s, "http://localhost:1234/";); } @Test public void testAs() { Integer x = TypeCoercions.coerce(new WithAs("3"), Integer.class); - Assert.assertEquals(x, (Integer)3); + assertEquals(x, (Integer)3); } @Test public void testFrom() { WithFrom x = TypeCoercions.coerce("3", WithFrom.class); - Assert.assertEquals(x.value, 3); + assertEquals(x.value, 3); } @Test @@ -433,7 +455,7 @@ public class TypeCoercionsTest { public void testObjectInMapCoercion() { ClassWithMap r1 = TypeCoercions.coerce(MutableMap.of("properties", MutableMap.of("x", 1)), ClassWithMap.class); - Assert.assertEquals(r1.properties.get("x"), 1); + assertEquals(r1.properties.get("x"), 1); r1 = TypeCoercions.coerce(MutableMap.of("properties", MutableMap.of("x", new MyClazz())), ClassWithMap.class); Asserts.assertInstanceOf(r1.properties.get("x"), MyClazz.class); diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java index 1a1ed20928..20c8649b40 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java @@ -62,6 +62,11 @@ import com.google.common.reflect.TypeToken; public class CommonAdaptorTypeCoercions { + // It might be nice to support key=value syntax when trying to parse things as maps. + // We have never supported this, but a bug in testng made it look like we did. + // This flags relevant areas of code and tests + public static final boolean PARSE_MAPS_WITH_EQUALS_SYMBOL = false; + @Beta public static final double DELTA_FOR_COERCION = 0.000001; private final TypeCoercerExtensible coercer; @@ -473,9 +478,15 @@ public class CommonAdaptorTypeCoercions { }; Maybe<Map<?, ?>> r1 = null; - + + if (PARSE_MAPS_WITH_EQUALS_SYMBOL) { + // implement it here if we support it. could perhaps simply replace with ": " ? + // but ideally want more sophisticated quote processing and splitting + throw new IllegalStateException("Parsing maps with equals not currently supported"); + } + // first try wrapping in braces if needed - if (!inputS.trim().startsWith("{")) { + if (!inputS.trim().startsWith("{") && (inputS.contains(": "))) { r1 = parseYaml.apply("{ "+inputS+" }"); if (r1.isPresent()) return (Maybe<T>) r1; // fall back to parsing without braces, e.g. if it's multiline diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java index 8f85bf1e33..0a6ebd5105 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java @@ -38,6 +38,7 @@ import java.util.TimeZone; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableSet; +import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.text.StringPredicates; import org.apache.commons.lang3.builder.EqualsBuilder; import org.slf4j.Logger; @@ -347,6 +348,9 @@ public class TypeCoercionsTest { public void testJsonStringToMapCoercion() { Map<?,?> s = coerce("{ \"a\" : \"1\", b : 2 }", Map.class); Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); + + Maybe<Map> result = coercer.tryCoerce("x", Map.class); + if (result.isPresent()) Asserts.fail("Should have failed, instead produced map "+result.get()); } @Test @@ -374,36 +378,7 @@ public class TypeCoercionsTest { Assert.assertEquals(s, ImmutableMap.of("a", 1, "b", 2)); } - @Test - public void testKeyEqualsValueStringToMapCoercion() { - Map<?,?> s = coerce("a=1,b=2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", "2")); - } - - @Test - public void testJsonStringWithoutBracesOrSpaceDisallowedAsMapCoercion() { - Map<?,?> s = coerce("a:1,b:2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", "2")); - // NB: snakeyaml 1.17 required spaces after the colon, but 1.21 accepts the above - } - - @Test - public void testEqualsInBracesMapCoercion() { - Map<?,?> s = coerce("{ a = 1, b = '2' }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", 1, "b", "2")); - } - - @Test - public void testKeyEqualsOrColonValueWithBracesStringToMapCoercion() { - Map<?,?> s = coerce("{ a=1, b: 2 }", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); - } - - @Test - public void testKeyEqualsOrColonValueWithoutBracesStringToMapCoercion() { - Map<?,?> s = coerce("a=1, b: 2", Map.class); - Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); - } + // map tests removed, tested in downstream core/internalTypeCoercionsTest @Test public void testURItoStringCoercion() {
