From a7e89de6107ee77cd8a3c2760deb37642db79225 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 8 Aug 2022 19:51:57 -0700 Subject: [PATCH] fix JsonNode leaking from JSON flattener (#12873) * fix JsonNode leaking from JSON flattener * adjustments --- .../common/parsers/JSONFlattenerMaker.java | 37 ++-- .../parsers/JSONFlattenerMakerTest.java | 172 ++++++++++++++++++ 2 files changed, 197 insertions(+), 12 deletions(-) create mode 100644 core/src/test/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMakerTest.java diff --git a/core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java b/core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java index 6df37ae1d11..a31bc2f8ff8 100644 --- a/core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java +++ b/core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java @@ -20,6 +20,7 @@ package org.apache.druid.java.util.common.parsers; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.BinaryNode; import com.google.common.collect.FluentIterable; import com.jayway.jsonpath.Configuration; import com.jayway.jsonpath.JsonPath; @@ -79,14 +80,14 @@ public class JSONFlattenerMaker implements ObjectFlatteners.FlattenerMaker makeJsonPathExtractor(final String expr) { final JsonPath jsonPath = JsonPath.compile(expr); - return node -> valueConversionFunction(jsonPath.read(node, JSONPATH_CONFIGURATION)); + return node -> finalizeConversionForMap(jsonPath.read(node, JSONPATH_CONFIGURATION)); } @Override @@ -96,7 +97,7 @@ public class JSONFlattenerMaker implements ObjectFlatteners.FlattenerMaker { try { - return valueConversionFunction(jsonQuery.apply(jsonNode).get(0)); + return finalizeConversionForMap(jsonQuery.apply(jsonNode).get(0)); } catch (JsonQueryException e) { throw new RuntimeException(e); @@ -114,14 +115,13 @@ public class JSONFlattenerMaker implements ObjectFlatteners.FlattenerMaker newList = new ArrayList<>(); for (JsonNode entry : val) { if (!entry.isNull()) { - newList.add(valueConversionFunction(entry)); + newList.add(finalizeConversionForMap(entry)); } } return newList; @@ -157,12 +167,15 @@ public class JSONFlattenerMaker implements ObjectFlatteners.FlattenerMaker newMap = new LinkedHashMap<>(); for (Iterator> it = val.fields(); it.hasNext(); ) { Map.Entry entry = it.next(); - newMap.put(entry.getKey(), valueConversionFunction(entry.getValue())); + newMap.put(entry.getKey(), finalizeConversionForMap(entry.getValue())); } return newMap; } - return val; + // All ValueNode implementations, as well as ArrayNode and ObjectNode will be handled by this point, so we should + // only be dealing with jackson specific types if we end up here (MissingNode, POJONode) so we can just return null + // so that we don't leak unhadled JsonNode objects + return null; } @Nullable diff --git a/core/src/test/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMakerTest.java b/core/src/test/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMakerTest.java new file mode 100644 index 00000000000..0a1b09578be --- /dev/null +++ b/core/src/test/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMakerTest.java @@ -0,0 +1,172 @@ +/* + * 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.druid.java.util.common.parsers; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.BinaryNode; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.apache.druid.java.util.common.StringUtils; +import org.junit.Assert; +import org.junit.Test; + +import java.math.BigInteger; +import java.util.List; +import java.util.Map; + +public class JSONFlattenerMakerTest +{ + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); + + private static final JSONFlattenerMaker FLATTENER_MAKER = new JSONFlattenerMaker(true); + + @Test + public void testStrings() throws JsonProcessingException + { + JsonNode node; + Object result; + + String s1 = "hello"; + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(s1)); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(s1, result); + + String s2 = "hello \uD900"; + String s2Json = "\"hello \uD900\""; + node = OBJECT_MAPPER.readTree(s2Json); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + // normal equals doesn't pass for this, so check using the same + Assert.assertArrayEquals(StringUtils.toUtf8(s2), StringUtils.toUtf8((String) result)); + } + + @Test + public void testNumbers() throws JsonProcessingException + { + JsonNode node; + Object result; + Integer i1 = 123; + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(i1)); + Assert.assertTrue(node.isInt()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(i1.longValue(), result); + + Long l1 = 1L + Integer.MAX_VALUE; + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(l1)); + Assert.assertTrue(node.isLong()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(l1, result); + + Float f1 = 230.333f; + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(f1)); + Assert.assertTrue(node.isNumber()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(230.333, result); + + // float.max value plus some (using float max constant, even in a comment makes checkstyle sad) + Double d1 = 0x1.fffffeP+127 + 100.0; + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(d1)); + Assert.assertTrue(node.isDouble()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(d1, result); + + BigInteger bigInt = new BigInteger(String.valueOf(Long.MAX_VALUE)); + BigInteger bigInt2 = new BigInteger(String.valueOf(Long.MAX_VALUE)); + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(bigInt.add(bigInt2))); + Assert.assertTrue(node.isBigInteger()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(bigInt.add(bigInt).doubleValue(), result); + } + + @Test + public void testBoolean() throws JsonProcessingException + { + Boolean bool = true; + JsonNode node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(bool)); + Assert.assertTrue(node.isBoolean()); + Object result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(bool, result); + } + + @Test + public void testBinary() + { + byte[] data = new byte[]{0x01, 0x02, 0x03}; + // make binary node directly for test, object mapper used in tests deserializes to TextNode with base64 string + JsonNode node = new BinaryNode(data); + Object result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(data, result); + } + + @Test + public void testNested() throws JsonProcessingException + { + JsonNode node; + Object result; + List intArray = ImmutableList.of(1, 2, 3); + List expectedIntArray = ImmutableList.of(1L, 2L, 3L); + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(intArray)); + Assert.assertTrue(node.isArray()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(expectedIntArray, result); + + Map theMap = + ImmutableMap.builder() + .put("bool", true) + .put("int", 1) + .put("long", 1L) + .put("float", 0.11f) + .put("double", 0.33) + .put("binary", new byte[]{0x01, 0x02, 0x03}) + .put("list", ImmutableList.of("foo", "bar", "baz")) + .put("anotherList", intArray) + .build(); + + Map expectedMap = + ImmutableMap.builder() + .put("bool", true) + .put("int", 1L) + .put("long", 1L) + .put("float", 0.11) + .put("double", 0.33) + .put("binary", StringUtils.encodeBase64String(new byte[]{0x01, 0x02, 0x03})) + .put("list", ImmutableList.of("foo", "bar", "baz")) + .put("anotherList", expectedIntArray) + .build(); + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(theMap)); + Assert.assertTrue(node.isObject()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(expectedMap, result); + + List theList = ImmutableList.of( + theMap, + theMap + ); + List expectedList = ImmutableList.of( + expectedMap, + expectedMap + ); + node = OBJECT_MAPPER.readTree(OBJECT_MAPPER.writeValueAsString(theList)); + Assert.assertTrue(node.isArray()); + result = FLATTENER_MAKER.finalizeConversionForMap(node); + Assert.assertEquals(expectedList, result); + } +}