Merge pull request #8 from nblair/detect-circular-reference

Watch for circular references during HollowObjectMapper#initializeTypeState, fail fast

.gitignore

@@ -7,3 +7,4 @@
 **.ipr
 **.iml
 **.iws
+.idea

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<Type> 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;

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<Type> 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))

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;
@@ -65,6 +66,9 @@ public void initializeTypeState(Class<?> clazz) {
     }
 
     HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths) {
+        return getTypeMapper(type, declaredName, hashKeyFieldPaths, new HashSet<Type>());
+    }
+    HollowTypeMapper getTypeMapper(Type type, String declaredName, String[] hashKeyFieldPaths, Set<Type> visited) {
         String typeName = declaredName != null ? declaredName : HollowObjectTypeMapper.getDefaultTypeName(type);
 
         HollowTypeMapper typeMapper = typeMappers.get(typeName);
@@ -76,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);

hollow/src/main/java/com/netflix/hollow/core/write/objectmapper/HollowObjectTypeMapper.java

@@ -30,14 +30,15 @@
 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 final HollowObjectMapper parentMapper;
 
     private final String typeName;
@@ -49,7 +50,7 @@ public class HollowObjectTypeMapper extends HollowTypeMapper {
 
     private final List<MappedField> mappedFields;
 
-    public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, String declaredTypeName) {
+    public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, String declaredTypeName, Set<Type> visited) {
         this.parentMapper = parentMapper;
         this.clazz = clazz;
         this.typeName = declaredTypeName != null ? declaredTypeName : getDefaultTypeName(clazz);
@@ -66,7 +67,7 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, S
 
             for(int i=0;i<declaredFields.length;i++) {
                 if(!Modifier.isStatic(declaredFields[i].getModifiers()) && !"__assigned_ordinal".equals(declaredFields[i].getName())) {
-                    mappedFields.add(new MappedField(declaredFields[i]));
+                    mappedFields.add(new MappedField(declaredFields[i], visited));
                 }
             }
 
@@ -152,6 +153,7 @@ protected HollowTypeWriteState getTypeWriteState() {
     }
 
     private class MappedField {
+
         private final String fieldName;
         private final long fieldOffset;
         private final Type type;
@@ -162,6 +164,9 @@ private class MappedField {
         private final boolean isEnumNameField;
 
         private MappedField(Field f) {
+            this(f, new HashSet<Type>());
+        }
+        private MappedField(Field f, Set<Type> visitedTypes) {
             this.fieldOffset = unsafe.objectFieldOffset(f);
             this.fieldName = f.getName();
             this.type = f.getGenericType();
@@ -188,7 +193,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(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;

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<Type> 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))

hollow/src/test/java/com/netflix/hollow/core/write/objectmapper/DirectCircularReference.java

@@ -0,0 +1,16 @@
+package com.netflix.hollow.core.write.objectmapper;
+
+
+/**
+ * Sample type that represents a direct circular reference between 2 classes.
+ */
+public class DirectCircularReference {
+
+    private final String name;
+    private final DirectCircularReference child;
+
+    public DirectCircularReference(String name, DirectCircularReference child) {
+        this.name = name;
+        this.child = child;
+    }
+}

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<DirectListCircularReference> children;
+
+    public DirectListCircularReference(String name, List<DirectListCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}

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<String, DirectMapCircularReference> children;
+
+    public DirectMapCircularReference(String name, Map<String, DirectMapCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}

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<DirectSetCircularReference> children;
+
+    public DirectSetCircularReference(String name, Set<DirectSetCircularReference> children) {
+        this.name = name;
+        this.children = children;
+    }
+}

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,44 @@ public void testEnumAndInlineClass() throws IOException {
         Assert.assertEquals(3, subObj.getInt("val2"));
     }
 
+    @Test
+    public void testMappingCircularReference() throws IOException {
+        assertExpectedFailureMappingType(DirectCircularReference.class, "child");
+    }
+    @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<String, List<Integer>> map(Object... keyValues) {
         Map<String, List<Integer>> map = new HashMap<String, List<Integer>>();
         int i = 0;
@@ -129,4 +166,5 @@ public TestClass(int val1, int val2) {
     }
 
 
+
 }

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;
+        }
+    }
+}