Pigsy-Monk opened a new issue, #3798: URL: https://github.com/apache/fory/issues/3798
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/fory/issues) and found no similar issues. ### Version # Fory: Self-Referential & Multi-Param Collection Serialization Bugs ## 1. Overview Two bugs with the same root cause were discovered in Fory 1.3.0. Both stem from `buildFieldType()` using the wrong method to resolve the element type of non-standard Collection implementations. | Bug | Scenario | Symptom | Root Cause | |------|------|------|------| | Bug 1 | `Box<T> implements List<Box<?>>` (self-referential) | `ClassCastException` in `populateTypeMappings` | `getTypeParameter0()` returns `T=Item`, not the element type `Box<?>` | | Bug 2 | `MyList<A, E> extends ArrayList<E>` (multi-param) | `ClassCastException`: `Integer cannot be cast to String` in JIT `writeCollection$` | `getTypeParameter0()` returns `A`, not the element type `E` | **Shared root cause**: `FieldTypes.buildFieldType()` (line 272-283) uses `genericType.getTypeParameter0()` to obtain the element type of a Collection. This method returns the **class's own first generic parameter**, not the **element type resolved from the Collection interface hierarchy**. For standard collections (e.g., `ArrayList<E>`), the two happen to coincide. For non-standard ones, they diverge. --- ## 2. Bug 1: Self-Referential Collection ### Reproduction ```java Box<T extends Item> implements SingleItemList<Box<? extends Item>> { private int id; @Override public Box<?> getFirst() { return this; } } Shelf { String name; List<Box<? extends Item>> boxes; } // ClassCastException during serialization fory.serialize(out, new Shelf("my-shelf", new Box<>())); ``` ### Failure Chain ``` buildFieldType(Box<? extends Item>) → COLLECTION_TYPE.isSupertypeOf → true → getTypeParameter0() → GenericType(Item) ← wrong! returns Box's type param T → Schema stores Box's element type as Item toTypeToken() reconciliation: → ObjectFieldType.toTypeToken(Box<Item>) → Box(raw) → Box<Item> != Box(raw) → collectionOf(Box.class, Box, meta) → getSubtype(Box.class) → populateTypeMappings(Box<? extends Item>, Box) → (ParameterizedType) Box → ClassCastException! ``` ### Root Cause Box's generic parameter `T` (bound to `Item`) is not the same as Box's element type as a `List` (`Box<? extends Item>`). `getTypeParameter0()` conflates the two. --- ## 3. Bug 2: Multi-Param Custom Collection ### Reproduction ```java MyList<A, E> extends ArrayList<E> { private A metadata; } Container { String name; MyList<String, Integer> numbers; } // ClassCastException during serialization fory.serialize(out, new Container("test", new MyList<>("meta", List.of(1, 2, 3)))); ``` Stack trace: ``` ClassCastException: Integer cannot be cast to String at ContainerForyRefCodec_0.writeCollection$(line 68) ``` ### Failure Chain ``` buildFieldType(MyList<String, Integer>) → COLLECTION_TYPE.isSupertypeOf → true (MyList IS-A ArrayList IS-A List) → getTypeParameter0() → GenericType(String) ← wrong! returns MyList's first param A → Schema stores MyList's element type as String JIT codegen: → writeCollection$ casts each element as (String) → actual elements are Integer → ClassCastException ``` ### Why Bug 2 is More Dangerous Bug 1 throws immediately, making it easy to discover. Bug 2 silently stores the wrong element type in the Schema. If `A` and `E` happen to be compatible types (e.g., `A=Object`, `E=String`), it may not throw but produce **semantically incorrect serialization data**. ### Contrast: Map Already Has Protection `getMapKeyValueType()` has an additional guard: ```java rawType.getTypeParameters().length == 2 ``` This verifies the class's declared parameter count equals Map's parameter count (2). If they differ, the fast path is skipped and the hierarchy is traversed. **The Collection branch has no such protection.** --- ## 4. `getTypeParameter0()` vs `getElementType()` — Semantic Comparison | Scenario | `getTypeParameter0()` | `getElementType()` | Equivalent? | |------|------|------|------| | `ArrayList<String>` | `GenericType(String)` | `TypeRef<String>` | ✅ Yes | | `HashSet<Long>` | `GenericType(Long)` | `TypeRef<Long>` | ✅ Yes | | `List<List<String>>` | `GenericType(List<String>)` | `TypeRef<List<String>>` | ✅ Yes | | raw `ArrayList` (no generics) | null → Object | Object | ✅ Yes | | `Box<T extends Item>` (self-ref) | `GenericType(Item)` ← **wrong** | `TypeRef<Box<? extends Item>>` ← **correct** | ❌ No | | `MyList<A, E> extends ArrayList<E>` | `GenericType(A)` ← **wrong** | `TypeRef<E>` ← **correct** | ❌ No | **Core semantic error**: The Collection branch is entered precisely because "this class IS-A Collection". At that point the question should be "what does it contain, as a Collection?" (`getElementType()`). Instead, the code asks "what generic parameter did this class declare?" (`getTypeParameter0()`). --- ## 5. Solution Reference (for component maintainers) ### 5.1 Schema-Level Fix: Use `getElementType()` with Self-Reference Detection **File**: `FieldTypes.java` line 272-283 **Before**: ```java if (COLLECTION_TYPE.isSupertypeOf(genericType.getTypeRef()) || (isXlang && (resolver.isCollection(rawType) || resolver.isSet(rawType)))) { return new CollectionFieldType( typeId, nullable, trackingRef, buildFieldType(resolver, null, genericType.getTypeParameter0() == null ? GenericType.build(Object.class) : genericType.getTypeParameter0())); } ``` **After**: ```java if (COLLECTION_TYPE.isSupertypeOf(genericType.getTypeRef()) || (isXlang && (resolver.isCollection(rawType) || resolver.isSet(rawType)))) { // Self-reference detection: prevents infinite recursion // (e.g., Box implements List<Box<?>>) FieldType selfRefFieldType = getSelfRefCollectionFieldType( resolver, genericType.getTypeRef(), rawType, typeId, nullable, trackingRef); if (selfRefFieldType != null) { return selfRefFieldType; } return new CollectionFieldType( typeId, nullable, trackingRef, buildFieldType(resolver, null, GenericType.build(TypeUtils.getElementType(genericType.getTypeRef())))); } // New helper method private static FieldType getSelfRefCollectionFieldType( TypeResolver resolver, TypeRef<?> typeRef, Class<?> rawType, int typeId, boolean nullable, boolean trackingRef) { TypeRef<?> elementTypeRef = TypeUtils.getElementType(typeRef); if (typeRef.resolveAllWildcards().equals(elementTypeRef.resolveAllWildcards())) { if (resolver.isRegisteredById(rawType)) { return new RegisteredFieldType(nullable, trackingRef, typeId, -1); } return new ObjectFieldType(typeId, nullable, trackingRef); } return null; } ``` **Notes**: 1. `getElementType()` resolves element type from the Collection hierarchy. For standard collections, results are identical to `getTypeParameter0()` (zero side effects). For non-standard collections, returns the correct element type. 2. Self-reference detection prevents infinite recursion: `getElementType(Box<?>)` returns `Box<?>`, which would otherwise loop forever. 3. `resolveAllWildcards().equals(...)` correctly distinguishes self-reference from nested collections: `ArrayList<ArrayList<String>>` is not falsely detected. 4. Zero overhead for standard collections. `buildFieldType` is a one-time initialization. ### 5.2 Serializer-Level Fix: `SingleItemListSerializer` Adapter **Problem**: After the Schema fix, self-referential collections (e.g., Box) are correctly marked as `ObjectFieldType` in the Schema. However, JIT codec serializer selection is based on class hierarchy (`Box IS-A Collection`), not on Schema `FieldType`. This causes: 1. **JIT codec cast CCE**: Box's JIT codec extends `GeneratedCompatibleSerializer` (not `CollectionLikeSerializer`), but Shelf's codec casts it to `CollectionLikeSerializer`. 2. **Write/read asymmetry**: The write path writes typeInfo when `serializer == null`, but the read path skips reading typeInfo after obtaining the serializer from `readCollectionCodegen` — mismatched buffer consumption. **Solution**: Create a `CollectionLikeSerializer` adapter with `supportCodegenHook=false` that delegates actual read/write to `ObjectSerializer`: ```java public class SingleItemListSerializer<T extends Collection<?>> extends CollectionLikeSerializer<T> { private final TypeResolver typeResolver; private volatile Serializer<T> delegateSerializer; public SingleItemListSerializer(TypeResolver typeResolver, Class<T> cls) { super(typeResolver, cls, false); // supportCodegenHook = false this.typeResolver = typeResolver; } private Serializer<T> getDelegate() { if (delegateSerializer == null) { synchronized (this) { if (delegateSerializer == null) { delegateSerializer = new ObjectSerializer<>( typeResolver, (Class<T>) type); } } } return delegateSerializer; } @Override public void write(WriteContext writeContext, T value) { getDelegate().write(writeContext, value); } @Override public T read(ReadContext readContext) { return getDelegate().read(readContext); } // onCollectionWrite / onCollectionRead are never invoked // (supportCodegenHook = false) public static boolean isSelfRefCollection(Class<?> cls) { if (!Collection.class.isAssignableFrom(cls)) return false; if (TypeUtils.isPrimitiveListClass(cls)) return false; try { TypeRef<?> typeRef = TypeRef.of(cls); TypeRef<?> elementType = TypeUtils.getElementType(typeRef); return elementType.getRawType() == cls; } catch (Exception e) { return false; } } } ``` Register via `SerializerFactory`: ```java public class SelfRefCollectionSerializerFactory implements SerializerFactory { @Override public Serializer createSerializer(TypeResolver typeResolver, Class<?> cls) { if (SingleItemListSerializer.isSelfRefCollection(cls)) { return new SingleItemListSerializer<>(typeResolver, cls); } return null; } } // Usage ForyBuilder builder = ForyBuilder.builder() .withSerializerFactory(new SelfRefCollectionSerializerFactory()); ``` **Notes**: 1. The adapter `extends CollectionLikeSerializer` → satisfies JIT codec's cast check. 2. `supportCodegenHook=false` → both write and read paths go through `serializer.write/read` directly, bypassing typeInfo write/read — buffer consumption is consistent. 3. The `SerializerFactory` intercepts before `ClassResolver.getSerializerClass()`'s `isCollection` check, skipping `DefaultJavaCollectionSerializer` selection. 4. The delegate uses `ObjectSerializer` rather than the JIT codec because the factory is called **before** the JIT codec is generated. Calling `typeResolver.getSerializer(cls)` would recursively trigger the factory. ### 5.3 How the Two Fixes Work Together | Layer | Schema Fix (5.1) | Serializer Fix (5.2) | |------|------|------| | Scope | Schema construction (`buildFieldType`) | Serializer selection (`getSerializerClass`) | | Addresses | Wrong element type (root cause of Bug 1 & 2) | JIT codec cast CCE and write/read asymmetry | | Independence | Fixes Bug 2 (MyList) alone | Fixes Bug 1's serializer issues alone | | Required | Yes (root cause) | Yes (JIT codec serializer selection is class-hierarchy-based, not Schema FieldType-based) | --- ## 6. Discussion ### 6.1 Why Can't JIT Codec Serializer Selection Use Schema's FieldType? The JIT codec serializer selection chain: ``` BaseObjectCodecBuilder.useCollectionSerialization(cls) → typeResolver.isCollection(cls) → Collection.class.isAssignableFrom(cls) // ← pure class hierarchy check ``` This is a `final` method on `TypeResolver` and cannot be overridden. Even though the Schema marks Box as `ObjectFieldType`, the JIT codec still determines Box IS-A Collection through class hierarchy and takes the Collection serialization path. ### 6.2 Why Not Add Self-Reference Detection Directly in `useCollectionSerialization`? This is another viable direction — adding self-reference detection at the 5 decision points in `BaseObjectCodecBuilder` so that self-referential collections take the Object path instead of the Collection path. However, this requires changes across multiple JIT codec decision points. The current approach (`SerializerFactory` + adapter) uses Fory's existing extension mechanism, avoiding changes to core JIT codec logic. ### 6.3 Why Use `ObjectSerializer` as the Delegate Instead of the JIT Codec? `SerializerFactory.createSerializer()` is called **before** the serializer is created. At that point, the JIT codec for the self-referential collection has not been generated yet. Calling `typeResolver.getSerializer(cls)` would recursively trigger the factory → infinite recursion. `ObjectSerializer` is the interpreter serializer — it reads and writes object fields directly. It is functionally correct (the performance impact is acceptable for this edge case). ### 6.4 Suggested Framework Improvements 1. **Make `TypeResolver.isCollection()` overridable**: Currently a `final` method. If non-final, a custom `ClassResolver` subclass could resolve the path selection for self-referential collections. 2. **Add `ForyBuilder.withTypeResolver()` or `withClassResolver()` configuration**: Currently `ClassResolver` is hard-coded in the `Fory` constructor with no injection point. 3. **Add a `isSelfRefCollection` flag to `Descriptor`**: Allows JIT codec to obtain self-reference judgment from Schema information rather than relying on class hierarchy. ### Component(s) Java ### Minimal reproduce step package org.apache.fory.collection; import org.apache.fory.Fory; import org.apache.fory.ThreadLocalFory; import org.apache.fory.config.CompatibleMode; import org.apache.fory.config.ForyBuilder; import org.apache.fory.config.Int64Encoding; import org.apache.fory.config.Language; import org.apache.fory.io.ForyInputStream; import org.apache.fory.serializer.Serializer; import org.apache.fory.serializer.collection.SelfRefCollectionSerializerFactory; import org.testng.Assert; import org.testng.annotations.Test; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.util.*; import java.util.Collections; public class SelfRefListTest { /** * The type parameter bound: a {@code Box} can only contain items of this type. */ public static class Item { } /** * Single-element {@link List} with default method implementations. * Like a box that can hold exactly one thing. */ public interface SingleItemList<E> extends List<E> { E getFirst(); @Override default int size() { return 1; } @Override default boolean isEmpty() { return false; } @Override default boolean contains(Object o) { return Objects.equals(getFirst(), o); } @Override default Iterator<E> iterator() { return Collections.singletonList(getFirst()).iterator(); } @Override default Object[] toArray() { return new Object[]{getFirst()}; } @SuppressWarnings("unchecked") @Override default <T> T[] toArray(T[] a) { return (T[]) Collections.singletonList(getFirst()).toArray(); } @Override default E get(int i) { if (i != 0) throw new IndexOutOfBoundsException("Index: " + i + ", Size: 1"); return getFirst(); } @Override default int indexOf(Object o) { return Objects.equals(getFirst(), o) ? 0 : -1; } @Override default int lastIndexOf(Object o) { return indexOf(o); } @Override default ListIterator<E> listIterator() { return Collections.singletonList(getFirst()).listIterator(); } @Override default ListIterator<E> listIterator(int i) { return Collections.singletonList(getFirst()).listIterator(i); } @Override default List<E> subList(int f, int t) { return Collections.singletonList(getFirst()).subList(f, t); } @Override default boolean containsAll(Collection<?> c) { for (Object o : c) { if (!Objects.equals(getFirst(), o)) return false; } return true; } @Override default boolean add(E e) { throw new UnsupportedOperationException(); } @Override default boolean remove(Object o) { throw new UnsupportedOperationException(); } @Override default boolean addAll(Collection<? extends E> c) { throw new UnsupportedOperationException(); } @Override default boolean addAll(int i, Collection<? extends E> c) { throw new UnsupportedOperationException(); } @Override default boolean removeAll(Collection<?> c) { throw new UnsupportedOperationException(); } @Override default boolean retainAll(Collection<?> c) { throw new UnsupportedOperationException(); } @Override default void clear() { throw new UnsupportedOperationException(); } @Override default E set(int i, E e) { throw new UnsupportedOperationException(); } @Override default void add(int i, E e) { throw new UnsupportedOperationException(); } @Override default E remove(int i) { throw new UnsupportedOperationException(); } } /** * A box that can contain another box — like Russian nesting dolls. * {@code T extends Item} but {@code SingleItemList<Box<?>>} uses an unbounded * wildcard, which resolves to {@code Object} — and {@code Object} does not * extend {@code Item}. This triggers the {@code ClassCastException} in Fory's JIT codegen. */ public static class Box<T extends Item> implements SingleItemList<Box<? extends Item>> { private int id; @Override public Box<?> getFirst() { return this; } public int getId() { return id; } public void setId(int id) { this.id = id; } } /** * A shelf holding a list of boxes. * When Fory JIT-compiles its serializer, resolving the field's element type * triggers the {@code ClassCastException}. */ public static class Shelf { private String name; private List<Box<? extends Item>> boxes; public Shelf() { } public Shelf(String name, List<Box<? extends Item>> boxes) { this.name = name; this.boxes = boxes; } public String getName() { return name; } public void setName(String name) { this.name = name; } public List<Box<?>> getBoxes() { return boxes; } public void setBoxes(List<Box<?>> boxes) { this.boxes = boxes; } } // ==================== tests ==================== @Test public void testShelfFailsWithoutFix() { Shelf shelf = new Shelf("my-shelf", new Box<>()); byte[] bytes = serialize(createFory(false), shelf); Shelf cloned = deserialize(createFory(false), bytes); Assert.assertNotNull(cloned); } private ThreadLocalFory createFory(boolean withFix) { return new ThreadLocalFory(builder -> { ForyBuilder b = builder .withLanguage(Language.JAVA) .requireClassRegistration(false) .withRefTracking(true) .withCompatibleMode(CompatibleMode.COMPATIBLE) .withAsyncCompilation(false) .withIntCompressed(true) .withCodegen(true) .withLongCompressed(Int64Encoding.VARINT) .withIntArrayCompressed(true) .withLongArrayCompressed(true); Fory fory = b.build(); if (withFix) { org.apache.fory.resolver.TypeResolver tr = fory.getTypeResolver(); fory.registerSerializer(Shelf.class, new ShelfSerializer(tr, Shelf.class)); } return fory; }); } private ThreadLocalFory createForyWithFactory() { return new ThreadLocalFory(builder -> { ForyBuilder b = builder .withLanguage(Language.JAVA) .requireClassRegistration(false) .withRefTracking(true) .withCompatibleMode(CompatibleMode.COMPATIBLE) .withAsyncCompilation(false) .withIntCompressed(true) .withCodegen(true) .withLongCompressed(Int64Encoding.VARINT) .withIntArrayCompressed(true) .withLongArrayCompressed(true) .withSerializerFactory(new SelfRefCollectionSerializerFactory()); return b.build(); }); } private <T> byte[] serialize(ThreadLocalFory fory, T object) { ByteArrayOutputStream out = new ByteArrayOutputStream(); fory.serialize(out, object); return out.toByteArray(); } @SuppressWarnings("unchecked") private <T> T deserialize(ThreadLocalFory fory, byte[] bytes) { return (T) fory.deserialize(new ForyInputStream(new ByteArrayInputStream(bytes))); } // ==================== fix ==================== private static class ShelfSerializer<T extends Shelf> extends Serializer<T> { public ShelfSerializer(org.apache.fory.resolver.TypeResolver r, Class<T> c) { super(r.getConfig(), c); } @Override public void write(org.apache.fory.context.WriteContext ctx, T obj) { ctx.writeRef(obj.getName()); ctx.writeRef(obj.getBoxes()); } @Override public T read(org.apache.fory.context.ReadContext ctx) { Shelf obj = new Shelf(); ctx.reference(obj); obj.setName((String) ctx.readRef()); obj.setBoxes((List<Box<?>>) ctx.readRef()); return (T) obj; } } } ### What did you expect to see? Assert.assertNotNull(cloned) succeed ### What did you see instead? ClassCastException ### Anything Else? _No response_ ### Are you willing to submit a PR? - [x] I'm willing to submit a PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
