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

    https://github.com/apache/storm/pull/2583#discussion_r172666072
  
    --- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
    @@ -1030,8 +1034,35 @@ private static Object normalizeConfValue(Object obj) 
{
             return ret;
         }
     
    -    public static boolean isValidConf(Map<String, Object> topoConf) {
    -        return normalizeConf(topoConf).equals(normalizeConf((Map<String, 
Object>) JSONValue.parse(JSONValue.toJSONString(topoConf))));
    +    @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);
    +    }
    +
    +    @VisibleForTesting
    +    static boolean checkMapEquality(Map<String, Object> orig, Map<String, 
Object> deser) {
    +   MapDifference<String, Object> diff = Maps.difference(orig, deser);
    --- End diff --
    
    We need to guard against deser being null.
    
    We are calling `JSONValue.parse` which returns null if it cannot parse an 
object.  This here will end up throwing an NPE if deser is null.  It would 
probably be better to switch to `JSONValue.parseWithException` and catch the 
exception and handle it instead.


---

Reply via email to