STORM-2649 More detailed check of config serialization
Project: http://git-wip-us.apache.org/repos/asf/storm/repo Commit: http://git-wip-us.apache.org/repos/asf/storm/commit/0a48fd9d Tree: http://git-wip-us.apache.org/repos/asf/storm/tree/0a48fd9d Diff: http://git-wip-us.apache.org/repos/asf/storm/diff/0a48fd9d Branch: refs/heads/master Commit: 0a48fd9ded9a9afbc08ef17d59292891d4aff2fc Parents: b99d80c Author: Hajós Gergely <[email protected]> Authored: Mon Apr 2 12:47:24 2018 +0200 Committer: Hajós Gergely <[email protected]> Committed: Mon Apr 2 12:47:24 2018 +0200 ---------------------------------------------------------------------- .../src/jvm/org/apache/storm/utils/Utils.java | 25 +++++++++++++------- .../org/apache/storm/TestConfigValidate.java | 19 ++++++++++++++- .../jvm/org/apache/storm/utils/UtilsTest.java | 24 +++++++++---------- 3 files changed, 47 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/storm/blob/0a48fd9d/storm-client/src/jvm/org/apache/storm/utils/Utils.java ---------------------------------------------------------------------- diff --git a/storm-client/src/jvm/org/apache/storm/utils/Utils.java b/storm-client/src/jvm/org/apache/storm/utils/Utils.java index 4ba527b..d6eddfc 100644 --- a/storm-client/src/jvm/org/apache/storm/utils/Utils.java +++ b/storm-client/src/jvm/org/apache/storm/utils/Utils.java @@ -1037,24 +1037,33 @@ public class Utils { @SuppressWarnings("unchecked") public static boolean isValidConf(Map<String, Object> topoConfIn) { Map<String, Object> origTopoConf = normalizeConf(topoConfIn); - Map<String, Object> deserTopoConf = normalizeConf( - (Map<String, Object>) JSONValue.parse(JSONValue.toJSONString(topoConfIn))); - return checkMapEquality(origTopoConf, deserTopoConf); + try { + Map<String, Object> deserTopoConf = normalizeConf( + (Map<String, Object>) JSONValue.parseWithException(JSONValue.toJSONString(topoConfIn))); + return isValidConf(origTopoConf, deserTopoConf); + } catch (ParseException e) { + LOG.error("Json serialized config could not be deserialized", e); + } + return false; } @VisibleForTesting - static boolean checkMapEquality(Map<String, Object> orig, Map<String, Object> deser) { + static boolean isValidConf(Map<String, Object> orig, Map<String, Object> deser) { MapDifference<String, Object> diff = Maps.difference(orig, deser); if (diff.areEqual()) { return true; } for (Map.Entry<String, Object> entryOnLeft : diff.entriesOnlyOnLeft().entrySet()) { - LOG.warn("Config property not serializable. Name: {} - Value: {}", entryOnLeft.getKey(), - entryOnLeft.getValue()); + LOG.warn("Config property ({}) is found in original config, but missing from the " + + "serialized-deserialized config. This is due to an internal error in " + + "serialization. Name: {} - Value: {}", + entryOnLeft.getKey(), entryOnLeft.getKey(), entryOnLeft.getValue()); } for (Map.Entry<String, Object> entryOnRight : diff.entriesOnlyOnRight().entrySet()) { - LOG.warn("Some config property changed during serialization. Changed Name: {} - Value: {}", - entryOnRight.getKey(), entryOnRight.getValue()); + LOG.warn("Config property ({}) is not found in original config, but present in " + + "serialized-deserialized config. This is due to an internal error in " + + "serialization. Name: {} - Value: {}", + entryOnRight.getKey(), entryOnRight.getKey(), entryOnRight.getValue()); } for (Map.Entry<String, ValueDifference<Object>> entryDiffers : diff.entriesDiffering().entrySet()) { Object leftValue = entryDiffers.getValue().leftValue(); http://git-wip-us.apache.org/repos/asf/storm/blob/0a48fd9d/storm-client/test/jvm/org/apache/storm/TestConfigValidate.java ---------------------------------------------------------------------- diff --git a/storm-client/test/jvm/org/apache/storm/TestConfigValidate.java b/storm-client/test/jvm/org/apache/storm/TestConfigValidate.java index b3e25c1..0911900 100644 --- a/storm-client/test/jvm/org/apache/storm/TestConfigValidate.java +++ b/storm-client/test/jvm/org/apache/storm/TestConfigValidate.java @@ -212,8 +212,25 @@ public class TestConfigValidate { testList.add(2); testList.add(new Integer("3")); testList.add(new Long("4")); + testList.add(new Float("3")); + testList.add(new Double("4")); + testList.add(ImmutableList.of("asdf", 3)); conf.put("eee", testList); - Utils.isValidConf(conf); + Assert.assertTrue(Utils.isValidConf(conf)); + } + + @Test + public void testNonValidConfigChar() { + Map<String, Object> conf = new HashMap<String, Object>(); + conf.put("q", ImmutableList.of("asdf", 'c')); + Assert.assertFalse(Utils.isValidConf(conf)); + } + + @Test + public void testNonValidConfigRandomObject() { + Map<String, Object> conf = new HashMap<String, Object>(); + conf.put("q", ImmutableList.of("asdf", new TestConfigValidate())); + Assert.assertFalse(Utils.isValidConf(conf)); } @Test http://git-wip-us.apache.org/repos/asf/storm/blob/0a48fd9d/storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java ---------------------------------------------------------------------- diff --git a/storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java b/storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java index 7f06f9a..664364b 100644 --- a/storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java +++ b/storm-client/test/jvm/org/apache/storm/utils/UtilsTest.java @@ -178,47 +178,47 @@ public class UtilsTest { } @Test - public void testCheckMapEqualityEmpty() { + public void testIsValidConfEmpty() { Map<String, Object> map0 = ImmutableMap.of(); - Assert.assertTrue(Utils.checkMapEquality(map0, map0)); + Assert.assertTrue(Utils.isValidConf(map0, map0)); } @Test - public void testCheckMapEqualityIdentical() { + public void testIsValidConfIdentical() { Map<String, Object> map1 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'), "k2", "as"); - Assert.assertTrue(Utils.checkMapEquality(map1, map1)); + Assert.assertTrue(Utils.isValidConf(map1, map1)); } @Test - public void testCheckMapEqualityEqual() { + public void testIsValidConfEqual() { Map<String, Object> map1 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'), "k2", "as"); Map<String, Object> map2 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'), "k2", "as"); - Assert.assertTrue(Utils.checkMapEquality(map1, map2)); // test deep equal + Assert.assertTrue(Utils.isValidConf(map1, map2)); // test deep equal } @Test - public void testCheckMapEqualityNotEqual() { + public void testIsValidConfNotEqual() { Map<String, Object> map1 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 'f'), "k2", "as"); Map<String, Object> map3 = ImmutableMap.of("k0", ImmutableList.of(1L, 2L), "k1", ImmutableSet.of('s', 't'), "k2", "as"); - Assert.assertFalse(Utils.checkMapEquality(map1, map3)); + Assert.assertFalse(Utils.isValidConf(map1, map3)); } @Test - public void testCheckMapEqualityPrimitiveNotEqual() { + public void testIsValidConfPrimitiveNotEqual() { Map<String, Object> map4 = ImmutableMap.of("k0", 2L); Map<String, Object> map5 = ImmutableMap.of("k0", 3L); - Assert.assertFalse(Utils.checkMapEquality(map4, map5)); + Assert.assertFalse(Utils.isValidConf(map4, map5)); } @Test - public void testCheckMapEqualityEmptyNotEqual() { + public void testIsValidConfEmptyNotEqual() { Map<String, Object> map0 = ImmutableMap.of(); Map<String, Object> map5 = ImmutableMap.of("k0", 3L); - Assert.assertFalse(Utils.checkMapEquality(map0, map5)); + Assert.assertFalse(Utils.isValidConf(map0, map5)); } }
