From d3c6e6b237d8cf3ff45b69c845109570c1200adb Mon Sep 17 00:00:00 2001 From: Marc Carter Date: Sat, 9 Oct 2021 20:24:52 +0100 Subject: [PATCH 1/3] Tidy DEDUCTION tests including extra commentary --- .../jsontype/TestPolymorphicDeduction.java | 82 ++++++++++++------- 1 file changed, 54 insertions(+), 28 deletions(-) diff --git a/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java b/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java index 64b58449de..e3ae3e9ca7 100644 --- a/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java +++ b/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java @@ -11,8 +11,10 @@ import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.MapperFeature; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectReader; import com.fasterxml.jackson.databind.exc.InvalidDefinitionException; import com.fasterxml.jackson.databind.exc.InvalidTypeIdException; +import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException; import com.fasterxml.jackson.databind.json.JsonMapper; import com.fasterxml.jackson.databind.type.TypeFactory; @@ -23,7 +25,7 @@ public class TestPolymorphicDeduction extends BaseMapTest { @JsonTypeInfo(use = DEDUCTION) - @JsonSubTypes( {@Type(LiveCat.class), @Type(DeadCat.class), @Type(Fleabag.class)}) + @JsonSubTypes( {@Type(LiveCat.class), @Type(DeadCat.class), @Type(Fleabag.class)}) // NOT including Cat // A general supertype with no properties - used for tests involving {} interface Feline {} @@ -60,10 +62,11 @@ static class Box { /********************************************************** */ + private static final String catJson = a2q("{'name':'Felix'}"); private static final String deadCatJson = a2q("{'name':'Felix','causeOfDeath':'entropy'}"); private static final String liveCatJson = a2q("{'name':'Felix','angry':true}"); private static final String luckyCatJson = a2q("{'name':'Felix','angry':true,'lives':8}"); - private static final String ambiguousCatJson = a2q("{'name':'Felix','age':2}"); + private static final String unknownCatJson = a2q("{'name':'Felix','age':2}"); private static final String fleabagJson = a2q("{}"); private static final String box1Json = a2q("{'feline':" + liveCatJson + "}"); private static final String box2Json = a2q("{'feline':" + deadCatJson + "}"); @@ -183,10 +186,13 @@ public void testArrayInference() throws Exception { } public void testIgnoreProperties() throws Exception { - Cat cat = MAPPER.reader() - .without(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) - .readValue(luckyCatJson, Cat.class); - assertTrue(cat instanceof LiveCat); + // Given: + ObjectReader reader = MAPPER.reader() + .without(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + // When: + Cat cat = reader.readValue(luckyCatJson, Cat.class); + // Then: + assertTrue("Should ignore 'lives' property", cat instanceof LiveCat); assertSame(cat.getClass(), LiveCat.class); assertEquals("Felix", cat.name); assertTrue(((LiveCat)cat).angry); @@ -198,39 +204,45 @@ static class AnotherLiveCat extends Cat { public void testAmbiguousClasses() throws Exception { try { + // Given: ObjectMapper mapper = JsonMapper.builder() // Don't use shared mapper! - .registerSubtypes(AnotherLiveCat.class) - .build(); - /*Cat cat =*/ mapper.readValue(liveCatJson, Cat.class); - fail("Should not get here"); + .registerSubtypes(AnotherLiveCat.class) + .build(); + // When: + mapper.readValue(liveCatJson, Cat.class); + + fail("AnotherLiveCat and LiveCat have same signature and should fail to register"); } catch (InvalidDefinitionException e) { - verifyException(e, "Subtypes "); - verifyException(e, "have the same signature"); - verifyException(e, "cannot be uniquely deduced"); + // Then: + verifyException(e, "Subtypes "); + verifyException(e, "have the same signature"); + verifyException(e, "cannot be uniquely deduced"); } } public void testAmbiguousProperties() throws Exception { try { - /*Cat cat =*/ MAPPER.readValue(ambiguousCatJson, Cat.class); - fail("Should not get here"); + // When: + MAPPER.readValue(catJson, Feline.class); + + fail("Feline doesn't include the Cat subtype"); } catch (InvalidTypeIdException e) { - verifyException(e, "Cannot deduce unique subtype"); + // Then: + verifyException(e, "Cannot deduce unique subtype"); } } public void testFailOnInvalidSubtype() throws Exception { - // Given: - JsonMapper mapper = JsonMapper.builder() // Don't use shared mapper! - .disable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE) - .build(); // When: - Cat cat = mapper.readValue(ambiguousCatJson, Cat.class); + ObjectReader reader = MAPPER.reader() + .without(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE); + // Given: + Cat cat = reader.readValue(unknownCatJson, Cat.class); // Then: - assertNull(cat); + assertNull("Should return null not an Exception", cat); } - @JsonTypeInfo(use = DEDUCTION, defaultImpl = Cat.class) + @JsonTypeInfo(use = DEDUCTION, defaultImpl = AnotherLiveCat.class) abstract static class CatMixin { } @@ -241,12 +253,26 @@ public void testDefaultImpl() throws Exception { .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) .build(); // When: - Cat cat = mapper.readValue(ambiguousCatJson, Cat.class); + Feline cat = mapper.readValue(unknownCatJson, Cat.class); // Then: - // Even though "age":2 implies this was a failed subtype, we are instructed to fallback to Cat regardless. - assertTrue(cat instanceof Cat); - assertSame(Cat.class, cat.getClass()); - assertEquals("Felix", cat.name); + assertTrue("Should fallback when unknown property blocks deduction", cat instanceof AnotherLiveCat); + assertSame(AnotherLiveCat.class, cat.getClass()); + } + + public void testDefaultImplSupercededByUnknownProperty() throws Exception { + try { + // Given: + JsonMapper mapper = JsonMapper.builder() // Don't use shared mapper! + .addMixIn(Cat.class, CatMixin.class) + //.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) + .build(); + // When: + mapper.readValue(unknownCatJson, Cat.class); + + fail("Unknown property supercedes defaultImpl"); + } catch (UnrecognizedPropertyException e) { + // Then: + } } public void testSimpleSerialization() throws Exception { From 3e4fb9d15c88156084622740d6e2873b1b38f575 Mon Sep 17 00:00:00 2001 From: Marc Carter Date: Sat, 9 Oct 2021 21:08:59 +0100 Subject: [PATCH 2/3] Fail-fast DEDUCTION processing --- .../databind/jsontype/impl/AsDeductionTypeDeserializer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java index 757992cf06..12f66de677 100644 --- a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java @@ -126,7 +126,7 @@ public Object deserializeTypedFromObject(JsonParser p, DeserializationContext ct final TokenBuffer tb = ctxt.bufferForInputBuffering(p); boolean ignoreCase = ctxt.isEnabled(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES); - for (; t == JsonToken.FIELD_NAME; t = p.nextToken()) { + for (; t == JsonToken.FIELD_NAME && !candidates.isEmpty(); t = p.nextToken()) { String name = p.currentName(); if (ignoreCase) name = name.toLowerCase(); From bd16c78a12966575c6e6e679b44cdac142101ae6 Mon Sep 17 00:00:00 2001 From: Marc Carter Date: Mon, 11 Oct 2021 20:41:04 +0100 Subject: [PATCH 3/3] Allow DEDUCTION of supertypes --- .../impl/AsDeductionTypeDeserializer.java | 22 ++++++- .../jsontype/impl/BitSetComparator.java | 25 ++++++++ .../jsontype/TestPolymorphicDeduction.java | 34 +++++++++-- .../jsontype/impl/BitSetComparatorTest.java | 61 +++++++++++++++++++ 4 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 src/main/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparator.java create mode 100644 src/test/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparatorTest.java diff --git a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java index 12f66de677..4010a8a15d 100644 --- a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/AsDeductionTypeDeserializer.java @@ -63,7 +63,8 @@ protected Map buildFingerprints(DeserializationConfig config, Co boolean ignoreCase = config.isEnabled(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES); int nextField = 0; - Map fingerprints = new HashMap<>(); + // Sorting by fingerprints such that supertypes always come first + Map fingerprints = new TreeMap<>(new BitSetComparator().reversed()); for (NamedType subtype : subtypes) { JavaType subtyped = config.getTypeFactory().constructType(subtype.getType()); @@ -141,12 +142,29 @@ public Object deserializeTypedFromObject(JsonParser p, DeserializationContext ct } } } + // We have zero or multiple candidates after parsing the whole input - // We have zero or multiple candidates, deduction has failed + if (!candidates.isEmpty() && firstCandidateIsSupertypeOfOthers(candidates)) { + return _deserializeTypedForId(p, ctxt, tb, subtypeFingerprints.get(candidates.get(0))); + } + + // deduction has failed String msgToReportIfDefaultImplFailsToo = String.format("Cannot deduce unique subtype of %s (%d candidates match)", ClassUtil.getTypeDescription(_baseType), candidates.size()); return _deserializeTypedUsingDefaultImpl(p, ctxt, tb, msgToReportIfDefaultImplFailsToo); } + private static boolean firstCandidateIsSupertypeOfOthers(List candidates) { + // Pre-sorted fingerprints means the _first_ item may be a supertype... + BitSet supertypeFields = candidates.get(0); + + // bitset is mutable, must take a copy :( ThreadLocal? + BitSet commonFields = (BitSet)supertypeFields.clone(); + for (BitSet candidate : candidates) { + commonFields.and(candidate); + } + return commonFields.equals(supertypeFields); + } + // Keep only fingerprints containing this field private static void prune(List candidates, int bit) { for (Iterator iter = candidates.iterator(); iter.hasNext(); ) { diff --git a/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparator.java b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparator.java new file mode 100644 index 0000000000..c206491613 --- /dev/null +++ b/src/main/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparator.java @@ -0,0 +1,25 @@ +package com.fasterxml.jackson.databind.jsontype.impl; + +import java.util.BitSet; +import java.util.Comparator; + +/** + * Terse but inefficient (for large arrays) ordering of {@link BitSet} as though it + * were an unsigned integer. + */ +public class BitSetComparator implements Comparator { + + // This could be done much more efficiently with access to the long[] + @Override + public int compare(BitSet left, BitSet right) { + if (left.equals(right)) return 0; + BitSet diff = (BitSet)left.clone(); + diff.xor(right); // diff = left ^ right + int firstDifference = diff.length() - 1; + + if (firstDifference == -1) return 0; + // due to xor, this high-bit is 1 in either left OR right + return right.get(firstDifference) ? 1 : -1; + } + +} diff --git a/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java b/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java index e3ae3e9ca7..57971c4d76 100644 --- a/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java +++ b/src/test/java/com/fasterxml/jackson/databind/jsontype/TestPolymorphicDeduction.java @@ -198,6 +198,30 @@ public void testIgnoreProperties() throws Exception { assertTrue(((LiveCat)cat).angry); } + public void testAmbiguousPropertiesMatchSupertype() throws Exception { + // Given: + JsonMapper mapper = JsonMapper.builder() // Don't use shared mapper! + .build(); + // When: + Feline feline = mapper.readValue(catJson, Cat.class); + // Then: + assertTrue("Cat is included in subtypes and is an exact match", feline instanceof Cat); + assertSame(feline.getClass(), Cat.class); + assertEquals("Felix", ((Cat)feline).name); + } + + public void testIgnorePropertiesCanMatchSupertype() throws Exception { + // Given: + ObjectReader reader = MAPPER.reader() + .without(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); + // When: + Cat cat = reader.readValue(unknownCatJson, Cat.class); + // Then: + assertTrue("Should ignore 'age' property, leaving 'name' to match Cat", cat instanceof Cat); + assertSame(cat.getClass(), Cat.class); + assertEquals("Felix", cat.name); + } + static class AnotherLiveCat extends Cat { public boolean angry; } @@ -237,23 +261,23 @@ public void testFailOnInvalidSubtype() throws Exception { ObjectReader reader = MAPPER.reader() .without(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE); // Given: - Cat cat = reader.readValue(unknownCatJson, Cat.class); + Feline cat = reader.readValue(unknownCatJson, Feline.class); // Then: assertNull("Should return null not an Exception", cat); } @JsonTypeInfo(use = DEDUCTION, defaultImpl = AnotherLiveCat.class) - abstract static class CatMixin { + abstract static class FelineMixin { } public void testDefaultImpl() throws Exception { // Given: JsonMapper mapper = JsonMapper.builder() // Don't use shared mapper! - .addMixIn(Cat.class, CatMixin.class) + .addMixIn(Feline.class, FelineMixin.class) .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) .build(); // When: - Feline cat = mapper.readValue(unknownCatJson, Cat.class); + Feline cat = mapper.readValue(unknownCatJson, Feline.class); // Then: assertTrue("Should fallback when unknown property blocks deduction", cat instanceof AnotherLiveCat); assertSame(AnotherLiveCat.class, cat.getClass()); @@ -263,7 +287,7 @@ public void testDefaultImplSupercededByUnknownProperty() throws Exception { try { // Given: JsonMapper mapper = JsonMapper.builder() // Don't use shared mapper! - .addMixIn(Cat.class, CatMixin.class) + .addMixIn(Cat.class, FelineMixin.class) //.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) .build(); // When: diff --git a/src/test/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparatorTest.java b/src/test/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparatorTest.java new file mode 100644 index 0000000000..bd0ecc9f12 --- /dev/null +++ b/src/test/java/com/fasterxml/jackson/databind/jsontype/impl/BitSetComparatorTest.java @@ -0,0 +1,61 @@ +package com.fasterxml.jackson.databind.jsontype.impl; + +import java.util.ArrayList; +import java.util.BitSet; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.TreeSet; +import java.util.stream.Collectors; + +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertEquals; + +public class BitSetComparatorTest { + + private final TreeSet tree = new TreeSet<>(new BitSetComparator()); + + private Map dictionary = new HashMap<>(); + + @Before + public void setUp() { + dictionary.put(bitset("11111111_11111111"), "a"); + dictionary.put(bitset("00111111_11111111"), "b"); + dictionary.put(bitset("00111111_00111111"), "c"); + dictionary.put(bitset("00000001_00111111"), "d"); + dictionary.put(bitset("00000000_11111111"), "e"); + dictionary.put(bitset("11111110"), "f"); + dictionary.put(bitset("00011110"), "g"); + dictionary.put(bitset("1"), "h"); + dictionary.put(bitset(""), "i"); + } + + @Test + public void sortsInputs() { + // Given: + List inputs = new ArrayList<>(dictionary.keySet()); + Collections.shuffle(inputs, new Random(1966)); + // When: + tree.addAll(inputs); + // Then: + String output = tree.stream().map(dictionary::get).collect(Collectors.joining()); + assertEquals("abcdefghi", output); + } + + private static BitSet bitset(String content) { + BitSet bitset = new BitSet(content.length()); + int bit = 0; + char[] chars = content.toCharArray(); + + for (int i = chars.length-1; i >= 0; i--) { + if (chars[i] >= '0') { + bitset.set(bit++, chars[i] != '0'); + } + } + return bitset; + } + +}