From 96f2bf6c70662d52601b80e3312949a8e249769b Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Tue, 5 May 2020 22:14:43 -0700 Subject: [PATCH] Fix the issue with ParametersUtils where input map is mutated --- .../cassandra/CassandraEventHandlerDAO.java | 2 +- .../core/execution/ParametersUtils.java | 18 +++--- .../core/execution/TestParametersUtils.java | 59 +++++++++++++++++++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/cassandra-persistence/src/main/java/com/netflix/conductor/dao/cassandra/CassandraEventHandlerDAO.java b/cassandra-persistence/src/main/java/com/netflix/conductor/dao/cassandra/CassandraEventHandlerDAO.java index 97bf894930..45e5618e23 100644 --- a/cassandra-persistence/src/main/java/com/netflix/conductor/dao/cassandra/CassandraEventHandlerDAO.java +++ b/cassandra-persistence/src/main/java/com/netflix/conductor/dao/cassandra/CassandraEventHandlerDAO.java @@ -48,7 +48,7 @@ public class CassandraEventHandlerDAO extends CassandraBaseDAO implements EventH private static final Logger LOGGER = LoggerFactory.getLogger(CassandraEventHandlerDAO.class); private static final String CLASS_NAME = CassandraEventHandlerDAO.class.getSimpleName(); - private Map eventHandlerCache = new HashMap<>(); + private volatile Map eventHandlerCache = new HashMap<>(); private final PreparedStatement insertEventHandlerStatement; private final PreparedStatement selectAllEventHandlersStatement; diff --git a/core/src/main/java/com/netflix/conductor/core/execution/ParametersUtils.java b/core/src/main/java/com/netflix/conductor/core/execution/ParametersUtils.java index 528742b92d..6e2b3e0daa 100644 --- a/core/src/main/java/com/netflix/conductor/core/execution/ParametersUtils.java +++ b/core/src/main/java/com/netflix/conductor/core/execution/ParametersUtils.java @@ -150,7 +150,7 @@ public Map replace(Map input, Object json) { } Configuration option = Configuration.defaultConfiguration().addOptions(Option.SUPPRESS_EXCEPTIONS); DocumentContext documentContext = JsonPath.parse(doc, option); - return replace(new HashMap<>(input), documentContext, null); + return replace(input, documentContext, null); } public Object replace(String paramString) { @@ -161,23 +161,23 @@ public Object replace(String paramString) { @SuppressWarnings("unchecked") private Map replace(Map input, DocumentContext documentContext, String taskId) { + Map result = new HashMap<>(); for (Entry e : input.entrySet()) { + Object newValue; Object value = e.getValue(); if (value instanceof String) { - Object replaced = replaceVariables(value.toString(), documentContext, taskId); - e.setValue(replaced); + newValue = replaceVariables(value.toString(), documentContext, taskId); } else if (value instanceof Map) { //recursive call - Object replaced = replace((Map) value, documentContext, taskId); - e.setValue(replaced); + newValue = replace((Map) value, documentContext, taskId); } else if (value instanceof List) { - Object replaced = replaceList((List) value, taskId, documentContext); - e.setValue(replaced); + newValue = replaceList((List) value, taskId, documentContext); } else { - e.setValue(value); + newValue = value; } + result.put(e.getKey(), newValue); } - return input; + return result; } @SuppressWarnings("unchecked") diff --git a/core/src/test/java/com/netflix/conductor/core/execution/TestParametersUtils.java b/core/src/test/java/com/netflix/conductor/core/execution/TestParametersUtils.java index 6aa0d36a16..af865778cc 100644 --- a/core/src/test/java/com/netflix/conductor/core/execution/TestParametersUtils.java +++ b/core/src/test/java/com/netflix/conductor/core/execution/TestParametersUtils.java @@ -7,6 +7,7 @@ import org.junit.Before; import org.junit.Test; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -141,4 +142,62 @@ public void testReplaceConcurrent() throws ExecutionException, InterruptedExcept executorService.shutdown(); } + + // Tests ParametersUtils with Map and List input values, and verifies input map is not mutated by ParametersUtils. + @Test + public void testReplaceInputWithMapAndList() throws Exception { + Map map = new HashMap<>(); + map.put("name", "conductor"); + map.put("version", 2); + map.put("externalId", "{\"taskRefName\":\"t001\",\"workflowId\":\"w002\"}"); + + Map input = new HashMap<>(); + input.put("k1", "${$.externalId}"); + input.put("k2", "${name}"); + input.put("k3", "${version}"); + + Map mapValue = new HashMap<>(); + mapValue.put("name", "${name}"); + mapValue.put("version", "${version}"); + input.put("map", mapValue); + + List listValue = new ArrayList<>(); + listValue.add("${name}"); + listValue.add("${version}"); + input.put("list", listValue); + + Object jsonObj = objectMapper.readValue(objectMapper.writeValueAsString(map), Object.class); + + Map replaced = parametersUtils.replace(input, jsonObj); + assertNotNull(replaced); + + // Verify that values are replaced correctly. + assertEquals("{\"taskRefName\":\"t001\",\"workflowId\":\"w002\"}", replaced.get("k1")); + assertEquals("conductor", replaced.get("k2")); + assertEquals(2, replaced.get("k3")); + + Map replacedMap = (Map) replaced.get("map"); + assertEquals("conductor", replacedMap.get("name")); + assertEquals(2, replacedMap.get("version")); + + List replacedList = (List) replaced.get("list"); + assertEquals(2, replacedList.size()); + assertEquals("conductor", replacedList.get(0)); + assertEquals(2, replacedList.get(1)); + + + // Verify that input map is not mutated + assertEquals("${$.externalId}", input.get("k1")); + assertEquals("${name}", input.get("k2")); + assertEquals("${version}", input.get("k3")); + + Map inputMap = (Map) input.get("map"); + assertEquals("${name}", inputMap.get("name")); + assertEquals("${version}", inputMap.get("version")); + + List inputList = (List) input.get("list"); + assertEquals(2, inputList.size()); + assertEquals("${name}", inputList.get(0)); + assertEquals("${version}", inputList.get(1)); + } }