fix: HollowObjectMapper throws if it encounters a duplicate field nam…

…e while walking the POJO class hierarchy (#666)
Netflix · Feb 22, 2024 · 6bd58a7 · 6bd58a7
1 parent 8857c90
commit 6bd58a7
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 1 deletion.
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
@@ -92,7 +92,7 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, S
                     throw new IllegalArgumentException("Unexpected array " + currentClass.getSimpleName() + " passed as field. Consider using collections or marking as transient.");
                 }
                 Field[] declaredFields = currentClass.getDeclaredFields();
-    
+
                 for(int i=0;i<declaredFields.length;i++) {
                     Field declaredField = declaredFields[i];
                     int modifiers = declaredField.getModifiers();
@@ -120,7 +120,11 @@ public HollowObjectTypeMapper(HollowObjectMapper parentMapper, Class<?> clazz, S
 
         this.schema = new HollowObjectSchema(typeName, mappedFields.size(), getKeyFieldPaths(clazz));
 
+        Set<String> fieldNamesSeen = new HashSet<>();
         for(MappedField field : mappedFields) {
+            if(!fieldNamesSeen.add(field.getFieldName()))
+                throw new IllegalArgumentException("Duplicate field name '" + field.getFieldName() + "' found in class hierarchy for class " + clazz.getName());
+
             if(field.getFieldType() == MappedFieldType.REFERENCE) {
                 schema.addField(field.getFieldName(), field.getFieldType().getSchemaFieldType(), field.getReferencedTypeName());
             } else {

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
@@ -325,6 +325,17 @@ public void testIntPreassignedOrdinal() {
         Assert.assertEquals(1, o.__assigned_ordinal);
     }
 
+    @Test
+    public void testFailsToCreateSchemaIfThereAreDuplicateFields() {
+        try {
+            HollowObjectMapper mapper = new HollowObjectMapper(writeStateEngine);
+            mapper.initializeTypeState(Child.class);
+            Assert.fail("Expected Exception not thrown");
+        } catch (IllegalArgumentException e) {
+            Assert.assertEquals("Duplicate field name 'myField1' found in class hierarchy for class com.netflix.hollow.core.write.objectmapper.HollowObjectMapperTest$Child", e.getMessage());
+        }
+    }
+
     /**
      * 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.
@@ -547,4 +558,11 @@ static class TypeWithIntAssignedOrdinal {
         int __assigned_ordinal = HollowConstants.ORDINAL_NONE;
     }
 
+    static class Parent {
+        String myField1;
+    }
+
+    static class Child extends Parent {
+        String myField1;
+    }
 }