From d02be7ddf1bcde8e9225accc4284efe4fb51dbd2 Mon Sep 17 00:00:00 2001 From: Nicholas Blair Date: Wed, 4 Jan 2017 09:20:24 -0600 Subject: [PATCH 1/4] ignore IntelliJ IDEA generated files --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 125c653ed6..9670b5be02 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ **.ipr **.iml **.iws +.idea From 0ca3178127647c2193d78a2b5209bfa2e698bfeb Mon Sep 17 00:00:00 2001 From: Nicholas Blair Date: Wed, 4 Jan 2017 09:29:44 -0600 Subject: [PATCH 2/4] detect circular references, throw IllegalStateException Prior to this changeset, a circular reference encountered during HollowObjectMapper#initializeTypeState(Class) results in a StackOverflowError that is trickier to debug. This change fails fast in the event we see the same type twice while recursing, and includes a message pointing to the problem field. --- .../objectmapper/HollowObjectMapper.java | 1 + .../objectmapper/HollowObjectTypeMapper.java | 19 ++++++++++-- .../objectmapper/DirectCircularReference.java | 21 +++++++++++++ .../objectmapper/HollowObjectMapperTest.java | 13 +++++++- .../IndirectCircularReference.java | 31 +++++++++++++++++++ .../hollow/tools/diff/HollowDiffTest.java | 10 +++--- 6 files changed, 87 insertions(+), 8 deletions(-) create mode 100644 hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java create mode 100644 hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/IndirectCircularReference.java diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java index 7ccc21b06f..8671ff0d22 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java @@ -21,6 +21,7 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; +import java.util.HashSet; import java.util.concurrent.atomic.AtomicInteger; import java.util.Map; import java.util.Set; diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java index d04c6ac411..1a113a0255 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java @@ -30,14 +30,21 @@ import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import sun.misc.Unsafe; @SuppressWarnings("restriction") public class HollowObjectTypeMapper extends HollowTypeMapper { private static final Unsafe unsafe = HollowUnsafeHandle.getUnsafe(); - + private static final ThreadLocal>> visitedClasses = new ThreadLocal>>() { + @Override + protected Set> initialValue() { + return new HashSet<>(); + } + }; private final HollowObjectMapper parentMapper; private final String typeName; @@ -152,6 +159,7 @@ protected HollowTypeWriteState getTypeWriteState() { } private class MappedField { + private final String fieldName; private final long fieldOffset; private final Type type; @@ -188,7 +196,14 @@ private MappedField(Field f) { fieldType = FieldType.STRING; } else { fieldType = FieldType.REFERENCE; - subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null); + if(!visitedClasses.get().contains(f.getType())) { + // guard recursion: track visited classes in a threadlocal so we can detect circular references as we recurse + visitedClasses.get().add(f.getType()); + subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null); + visitedClasses.get().remove(f.getType()); + } else { + throw new IllegalStateException("circular reference detected on field " + f + "; this type of relationship is not supported"); + } } this.subTypeMapper = subTypeMapper; diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java new file mode 100644 index 0000000000..26cb700682 --- /dev/null +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java @@ -0,0 +1,21 @@ +package com.netflix.hollow.core.write.objectmapper; + +import java.util.Collections; +import java.util.List; + +/** + * Sample type that represents a direct circular reference between 2 classes. + */ +public class DirectCircularReference { + + private final String name; + private final List children; + + public DirectCircularReference(String name) { + this(name, Collections.emptyList()); + } + public DirectCircularReference(String name, List children) { + this.name = name; + this.children = children; + } +} diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java index 7fc4939520..c832727cd5 100644 --- a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java @@ -23,7 +23,6 @@ import com.netflix.hollow.api.objects.generic.GenericHollowObject; import com.netflix.hollow.core.index.HollowPrimaryKeyIndex; import com.netflix.hollow.core.index.key.PrimaryKey; -import com.netflix.hollow.core.write.objectmapper.HollowObjectMapper; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -77,6 +76,17 @@ public void testEnumAndInlineClass() throws IOException { Assert.assertEquals(3, subObj.getInt("val2")); } + @Test(expected = IllegalStateException.class) + public void testMappingCircularReference() throws IOException { + HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine); + mapper.initializeTypeState(DirectCircularReference.class); + } + @Test(expected = IllegalStateException.class) + public void testMappingDeeperCircularReference() throws IOException { + HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine); + mapper.initializeTypeState(IndirectCircularReference.TypeE.class); + } + private Map> map(Object... keyValues) { Map> map = new HashMap>(); int i = 0; @@ -129,4 +139,5 @@ public TestClass(int val1, int val2) { } + } diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/IndirectCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/IndirectCircularReference.java new file mode 100644 index 0000000000..1608c7aa41 --- /dev/null +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/IndirectCircularReference.java @@ -0,0 +1,31 @@ +package com.netflix.hollow.core.write.objectmapper; + +/** + * Object model representing an indirect or nested circular reference: + * + * E depends on F, F depends on G, and G depends back on E + */ +public class IndirectCircularReference { + + class TypeE { + private final TypeF f; + + public TypeE(TypeF f) { + this.f = f; + } + } + class TypeF { + private final TypeG g; + + public TypeF(TypeG g) { + this.g = g; + } + } + class TypeG { + private final TypeE e; + + public TypeG(TypeE e) { + this.e = e; + } + } +} diff --git a/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java b/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java index ba25e45546..2cfcbc38c5 100644 --- a/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java +++ b/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java @@ -74,14 +74,14 @@ public void setUp() { typeCSchema.addField("c1", FieldType.LONG); typeCSchema.addField("c2", FieldType.BOOLEAN); - typeDSchema = new HollowObjectSchema("TypeD", 3, "d1", "d2"); + typeDSchema = new HollowObjectSchema("DirectCircularReference", 3, "d1", "d2"); typeDSchema.addField("d1", FieldType.FLOAT); typeDSchema.addField("d2", FieldType.DOUBLE); typeDSchema.addField("d3", FieldType.BYTES); listOfTypeCSchema = new HollowListSchema("ListOfTypeC", "TypeC"); - setOfTypeDSchema = new HollowSetSchema("SetOfTypeD", "TypeD"); - mapOfTypeCToTypeDSchema = new HollowMapSchema("MapOfTypeCToTypeD", "TypeC", "TypeD"); + setOfTypeDSchema = new HollowSetSchema("SetOfTypeD", "DirectCircularReference"); + mapOfTypeCToTypeDSchema = new HollowMapSchema("MapOfTypeCToTypeD", "TypeC", "DirectCircularReference"); } @Test @@ -215,7 +215,7 @@ public void testAutoDiscoverTypeDiffs() throws Exception { Assert.assertEquals(1, diff.getTypeDiffs().size()); - HollowTypeDiff typeDDiff = diff.getTypeDiff("TypeD"); + HollowTypeDiff typeDDiff = diff.getTypeDiff("DirectCircularReference"); Assert.assertNotNull(typeDDiff); HollowDiffMatcher matcher = typeDDiff.getMatcher(); @@ -335,7 +335,7 @@ private int addDRec(HollowWriteStateEngine stateEngine, TypeDRec typeD) { rec.setFloat("d1", typeD.d1); rec.setDouble("d2", typeD.d2); rec.setBytes("d3", typeD.d3); - return stateEngine.add("TypeD", rec); + return stateEngine.add("DirectCircularReference", rec); } private TypeCRec[] cList(TypeCRec... cs) { From eed5c9368468c171f2d24dafaf112b47b633f374 Mon Sep 17 00:00:00 2001 From: Nicholas Blair Date: Wed, 4 Jan 2017 09:43:53 -0600 Subject: [PATCH 3/4] revert unintentional changes IDEA automatically replaced "TypeD" with "DirectCircularReference". --- .../com/netflix/hollow/tools/diff/HollowDiffTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java b/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java index 2cfcbc38c5..ba25e45546 100644 --- a/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java +++ b/hollow/src/test/java/com/netflix/hollow/tools/diff/HollowDiffTest.java @@ -74,14 +74,14 @@ public void setUp() { typeCSchema.addField("c1", FieldType.LONG); typeCSchema.addField("c2", FieldType.BOOLEAN); - typeDSchema = new HollowObjectSchema("DirectCircularReference", 3, "d1", "d2"); + typeDSchema = new HollowObjectSchema("TypeD", 3, "d1", "d2"); typeDSchema.addField("d1", FieldType.FLOAT); typeDSchema.addField("d2", FieldType.DOUBLE); typeDSchema.addField("d3", FieldType.BYTES); listOfTypeCSchema = new HollowListSchema("ListOfTypeC", "TypeC"); - setOfTypeDSchema = new HollowSetSchema("SetOfTypeD", "DirectCircularReference"); - mapOfTypeCToTypeDSchema = new HollowMapSchema("MapOfTypeCToTypeD", "TypeC", "DirectCircularReference"); + setOfTypeDSchema = new HollowSetSchema("SetOfTypeD", "TypeD"); + mapOfTypeCToTypeDSchema = new HollowMapSchema("MapOfTypeCToTypeD", "TypeC", "TypeD"); } @Test @@ -215,7 +215,7 @@ public void testAutoDiscoverTypeDiffs() throws Exception { Assert.assertEquals(1, diff.getTypeDiffs().size()); - HollowTypeDiff typeDDiff = diff.getTypeDiff("DirectCircularReference"); + HollowTypeDiff typeDDiff = diff.getTypeDiff("TypeD"); Assert.assertNotNull(typeDDiff); HollowDiffMatcher matcher = typeDDiff.getMatcher(); @@ -335,7 +335,7 @@ private int addDRec(HollowWriteStateEngine stateEngine, TypeDRec typeD) { rec.setFloat("d1", typeD.d1); rec.setDouble("d2", typeD.d2); rec.setBytes("d3", typeD.d3); - return stateEngine.add("DirectCircularReference", rec); + return stateEngine.add("TypeD", rec); } private TypeCRec[] cList(TypeCRec... cs) { From a2b8f2fb386c99a6a7b1de9ec958fe889fddb2d2 Mon Sep 17 00:00:00 2001 From: Nicholas Blair Date: Wed, 4 Jan 2017 16:00:24 -0600 Subject: [PATCH 4/4] refactor: replace ThreadLocal with method argument Set tracks types we've visited as we recurse. --- .../objectmapper/HollowListTypeMapper.java | 7 +++- .../objectmapper/HollowMapTypeMapper.java | 8 ++-- .../objectmapper/HollowObjectMapper.java | 13 +++--- .../objectmapper/HollowObjectTypeMapper.java | 25 +++++------ .../objectmapper/HollowSetTypeMapper.java | 5 ++- .../objectmapper/DirectCircularReference.java | 11 ++--- .../DirectListCircularReference.java | 18 ++++++++ .../DirectMapCircularReference.java | 17 ++++++++ .../DirectSetCircularReference.java | 17 ++++++++ .../objectmapper/HollowObjectMapperTest.java | 41 +++++++++++++++---- 10 files changed, 121 insertions(+), 41 deletions(-) create mode 100644 hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java create mode 100644 hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java create mode 100644 hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java index c96263d310..e3857ec799 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowListTypeMapper.java @@ -25,7 +25,10 @@ import com.netflix.hollow.core.write.HollowTypeWriteState; import com.netflix.hollow.core.write.HollowWriteRecord; import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.util.HashSet; import java.util.List; +import java.util.Set; public class HollowListTypeMapper extends HollowTypeMapper { @@ -37,8 +40,8 @@ public class HollowListTypeMapper extends HollowTypeMapper { private final HollowTypeMapper elementMapper; - public HollowListTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, boolean ignoreListOrdering) { - this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null); + public HollowListTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, boolean ignoreListOrdering, Set visited) { + this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited); String typeName = declaredName != null ? declaredName : getDefaultTypeName(type); this.schema = new HollowListSchema(typeName, elementMapper.getTypeName()); this.ignoreListOrdering = ignoreListOrdering; diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java index 4b9faaeda5..7c5f202014 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowMapTypeMapper.java @@ -26,7 +26,9 @@ import com.netflix.hollow.core.write.HollowWriteRecord; import com.netflix.hollow.core.write.HollowWriteStateEngine; import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.Map; +import java.util.Set; public class HollowMapTypeMapper extends HollowTypeMapper { @@ -38,9 +40,9 @@ public class HollowMapTypeMapper extends HollowTypeMapper { private HollowTypeMapper keyMapper; private HollowTypeMapper valueMapper; - public HollowMapTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys) { - this.keyMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null); - this.valueMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[1], null, null); + public HollowMapTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys, Set visited) { + this.keyMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited); + this.valueMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[1], null, null, visited); String typeName = declaredName != null ? declaredName : getDefaultTypeName(type); if(hashKeyFieldPaths == null && useDefaultHashKeys && (keyMapper instanceof HollowObjectTypeMapper)) diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java index 8671ff0d22..5dc28f8d34 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapper.java @@ -66,6 +66,9 @@ public void initializeTypeState(Class clazz) { } HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths) { + return getTypeMapper(type, declaredName, hashKeyFieldPaths, new HashSet()); + } + HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths, Set visited) { String typeName = declaredName != null ? declaredName : HollowObjectTypeMapper.getDefaultTypeName(type); HollowTypeMapper typeMapper = typeMappers.get(typeName); @@ -77,17 +80,17 @@ HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyF Class clazz = (Class) parameterizedType.getRawType(); if(List.class.isAssignableFrom(clazz)) { - typeMapper = new HollowListTypeMapper(this, parameterizedType, declaredName, ignoreListOrdering); + typeMapper = new HollowListTypeMapper(this, parameterizedType, declaredName, ignoreListOrdering, visited); } else if(Set.class.isAssignableFrom(clazz)) { - typeMapper = new HollowSetTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys); + typeMapper = new HollowSetTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys, visited); } else if(Map.class.isAssignableFrom(clazz)) { - typeMapper = new HollowMapTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys); + typeMapper = new HollowMapTypeMapper(this, parameterizedType, declaredName, hashKeyFieldPaths, stateEngine, useDefaultHashKeys, visited); } else { - return getTypeMapper(clazz, declaredName, hashKeyFieldPaths); + return getTypeMapper(clazz, declaredName, hashKeyFieldPaths, visited); } } else { - typeMapper = new HollowObjectTypeMapper(this, (Class)type, declaredName); + typeMapper = new HollowObjectTypeMapper(this, (Class)type, declaredName, visited); } HollowTypeMapper existing = typeMappers.putIfAbsent(typeName, typeMapper); diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java index 1a113a0255..c76eaa140f 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java @@ -39,12 +39,6 @@ public class HollowObjectTypeMapper extends HollowTypeMapper { private static final Unsafe unsafe = HollowUnsafeHandle.getUnsafe(); - private static final ThreadLocal>> visitedClasses = new ThreadLocal>>() { - @Override - protected Set> initialValue() { - return new HashSet<>(); - } - }; private final HollowObjectMapper parentMapper; private final String typeName; @@ -56,7 +50,7 @@ protected Set> initialValue() { private final List mappedFields; - public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class clazz, String declaredTypeName) { + public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class clazz, String declaredTypeName, Set visited) { this.parentMapper = parentMapper; this.clazz = clazz; this.typeName = declaredTypeName != null ? declaredTypeName : getDefaultTypeName(clazz); @@ -73,7 +67,7 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class clazz, S for(int i=0;i()); + } + private MappedField(Field f, Set visitedTypes) { this.fieldOffset = unsafe.objectFieldOffset(f); this.fieldName = f.getName(); this.type = f.getGenericType(); @@ -196,14 +193,14 @@ private MappedField(Field f) { fieldType = FieldType.STRING; } else { fieldType = FieldType.REFERENCE; - if(!visitedClasses.get().contains(f.getType())) { - // guard recursion: track visited classes in a threadlocal so we can detect circular references as we recurse - visitedClasses.get().add(f.getType()); - subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null); - visitedClasses.get().remove(f.getType()); - } else { + if(visitedTypes.contains(this.type)){ throw new IllegalStateException("circular reference detected on field " + f + "; this type of relationship is not supported"); } + // guard recursion here + visitedTypes.add(this.type); + subTypeMapper = parentMapper.getTypeMapper(type, typeNameAnnotation != null ? typeNameAnnotation.name() : null, hashKeyAnnotation != null ? hashKeyAnnotation.fields() : null, visitedTypes); + // once we've safely returned from a leaf node in recursion, we can remove this MappedField's type + visitedTypes.remove(this.type); } this.subTypeMapper = subTypeMapper; diff --git a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java index 8c55d053d8..24755a0f20 100644 --- a/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java +++ b/hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowSetTypeMapper.java @@ -26,6 +26,7 @@ import com.netflix.hollow.core.write.HollowWriteRecord; import com.netflix.hollow.core.write.HollowWriteStateEngine; import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.Set; public class HollowSetTypeMapper extends HollowTypeMapper { @@ -37,8 +38,8 @@ public class HollowSetTypeMapper extends HollowTypeMapper { private final HollowTypeMapper elementMapper; - public HollowSetTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys) { - this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null); + public HollowSetTypeMapper(HollowObjectMapper parentMapper, ParameterizedType type, String declaredName, String[] hashKeyFieldPaths, HollowWriteStateEngine stateEngine, boolean useDefaultHashKeys, Set visited) { + this.elementMapper = parentMapper.getTypeMapper(type.getActualTypeArguments()[0], null, null, visited); String typeName = declaredName != null ? declaredName : getDefaultTypeName(type); if(hashKeyFieldPaths == null && useDefaultHashKeys && (elementMapper instanceof HollowObjectTypeMapper)) diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java index 26cb700682..32032bf1c7 100644 --- a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java @@ -1,7 +1,5 @@ package com.netflix.hollow.core.write.objectmapper; -import java.util.Collections; -import java.util.List; /** * Sample type that represents a direct circular reference between 2 classes. @@ -9,13 +7,10 @@ public class DirectCircularReference { private final String name; - private final List children; + private final DirectCircularReference child; - public DirectCircularReference(String name) { - this(name, Collections.emptyList()); - } - public DirectCircularReference(String name, List children) { + public DirectCircularReference(String name, DirectCircularReference child) { this.name = name; - this.children = children; + this.child = child; } } diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java new file mode 100644 index 0000000000..a47b49e73f --- /dev/null +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectListCircularReference.java @@ -0,0 +1,18 @@ +package com.netflix.hollow.core.write.objectmapper; + +import java.util.Collections; +import java.util.List; + +/** + * Sample type that represents a direct circular reference between 2 classes, with a List containing the child. + */ +public class DirectListCircularReference { + + private final String name; + private final List children; + + public DirectListCircularReference(String name, List children) { + this.name = name; + this.children = children; + } +} diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java new file mode 100644 index 0000000000..4111be23c7 --- /dev/null +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectMapCircularReference.java @@ -0,0 +1,17 @@ +package com.netflix.hollow.core.write.objectmapper; + +import java.util.Map; + +/** + * Sample type that represents a direct circular reference between 2 classes, with a Map containing the child. + */ +public class DirectMapCircularReference { + + private final String name; + private final Map children; + + public DirectMapCircularReference(String name, Map children) { + this.name = name; + this.children = children; + } +} diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java new file mode 100644 index 0000000000..c6821e022c --- /dev/null +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectSetCircularReference.java @@ -0,0 +1,17 @@ +package com.netflix.hollow.core.write.objectmapper; + +import java.util.Set; + +/** + * Sample type that represents a direct circular reference between 2 classes, with a Set containing the child. + */ +public class DirectSetCircularReference { + + private final String name; + private final Set children; + + public DirectSetCircularReference(String name, Set children) { + this.name = name; + this.children = children; + } +} diff --git a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java index c832727cd5..3de24ae68d 100644 --- a/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java +++ b/hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/HollowObjectMapperTest.java @@ -76,17 +76,44 @@ public void testEnumAndInlineClass() throws IOException { Assert.assertEquals(3, subObj.getInt("val2")); } - @Test(expected = IllegalStateException.class) + @Test public void testMappingCircularReference() throws IOException { - HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine); - mapper.initializeTypeState(DirectCircularReference.class); + assertExpectedFailureMappingType(DirectCircularReference.class, "child"); } - @Test(expected = IllegalStateException.class) - public void testMappingDeeperCircularReference() throws IOException { - HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine); - mapper.initializeTypeState(IndirectCircularReference.TypeE.class); + @Test + public void testMappingCircularReferenceList() throws IOException { + assertExpectedFailureMappingType(DirectListCircularReference.class, "children"); + } + @Test + public void testMappingCircularReferenceSet() throws IOException { + assertExpectedFailureMappingType(DirectSetCircularReference.class, "children"); + } + @Test + public void testMappingCircularReferenceMap() throws IOException { + assertExpectedFailureMappingType(DirectMapCircularReference.class, "children"); + } + @Test + public void testMappingIndirectircularReference() throws IOException { + assertExpectedFailureMappingType(IndirectCircularReference.TypeE.class, "f"); } + /** + * Convenience method for experimenting with {@link HollowObjectMapper#initializeTypeState(Class)} + * on classes we know should fail due to circular references, confirming the exception message is correct. + * + * @param clazz class to initialize + * @param fieldName the name of the field that should trip the circular reference detection + */ + protected void assertExpectedFailureMappingType(Class clazz, String fieldName) { + final String expected = clazz.getSimpleName() + "." + fieldName; + try { + HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine); + mapper.initializeTypeState(clazz); + } catch (IllegalStateException e) { + + Assert.assertTrue(String.format("missing expected fieldname %s in the message, was %s", expected, e.getMessage()), e.getMessage().contains(expected)); + } + } private Map> map(Object... keyValues) { Map> map = new HashMap>(); int i = 0;