feat(java): capture and forward extra fields in compatible mode#3861
feat(java): capture and forward extra fields in compatible mode#3861Tatenda-k wants to merge 1 commit into
Conversation
Add ForyExtraFields sink, an opt-in mechanism for preserving unmatched remote fields during compatible-mode deserialization and replaying them on re-serialization. Covers both the interpreter and generated codepaths,
| + sinkTypeDefId | ||
| + " has not been read into this class on this Fory instance."); | ||
| } | ||
| resolver.writeTypeInfo(this, headerTypeInfo); |
There was a problem hiding this comment.
Preserve the original type header when the writer class is unavailable
When the intermediary cannot load the original writer class, headerTypeInfo.type is UnknownStruct with NONEXISTENT_META_SHARED_ID. Calling writeTypeInfo here emits that placeholder and then writes a target-class replay body. The normal unknown-class path must go through UnknownStructSerializer.write, which rewrites the placeholder to the original compatible type id/user id and emits the remote TypeDef. Without that rewrite, the forwarded stream is undecodable in the rolling-deployment case this feature is intended to support. Please route this header through a schema-aware writer and cover it with isolated classloaders.
| + sinkTypeDefId | ||
| + " has not been read into this class on this Fory instance."); | ||
| } | ||
| resolver.writeTypeInfo(this, headerTypeInfo); |
There was a problem hiding this comment.
Key replay metadata by the remote TypeDef identity
writeSharedClassMeta deduplicates metadata by typeInfo.type (Class<?>), but two remote versions of the same class have different TypeDef ids. If captured objects from both versions are forwarded in one root graph, the second header references the first TypeDef while its body is written by the second replay serializer, causing field corruption or reader-index drift. Replay metadata needs to be keyed by the checked TypeDef identity, with a regression test containing two versions of the same class in one graph.
| AbstractObjectSerializer.writeBuildInFieldValue( | ||
| writeContext, typeResolver, refWriter, fieldInfo, buffer, fieldValue); | ||
| } else { | ||
| AbstractObjectSerializer.writeBuildInField( |
There was a problem hiding this comment.
Handle converted fields when replaying the remote schema
A compatible scalar conversion such as remote int score to local long score produces a descriptor with a converter but no fieldAccessor. If any other field populates the sink, this branch calls writeBuildInField with that null accessor. The generated path has the same bug because it treats every descriptor.getField() == null as an unmatched field and reads an absent sink entry. Please define the reverse replay behavior for converter fields (or preserve their remote value) and test interpreter and codegen with a converted field plus an extra field.
| public static void capture( | ||
| Object target, FieldAccessor sinkAccessor, TypeDef typeDef, String name, Object value) { | ||
| ForyExtraFields extraField = (ForyExtraFields) sinkAccessor.getObject(target); | ||
| if (extraField == null) { |
There was a problem hiding this comment.
Attach the TypeDef when the sink is already initialized
This assigns typeDef only when the sink field was null. A common declaration such as final ForyExtraFields extraFields = new ForyExtraFields() captures map entries but leaves typeDef null, so tryWriteExtraFieldsSchema silently skips replay and the downstream peer loses the unknown fields. On first capture, an existing unbound sink also needs to bind the remote TypeDef, and subsequent captures should verify that the schema identity is consistent.
| */ | ||
| public final class ForyExtraFields { | ||
|
|
||
| private final Map<String, Object> fields = new HashMap<>(); |
There was a problem hiding this comment.
Use the complete remote field identity as the sink key
A simple field name is not unique in a native Java TypeDef: superclass and subclass layers may legally contain fields with the same name, distinguished by declaring class and field id/name. Capturing both into this map overwrites one value, and replay writes both slots from the surviving value. Tagged fields are also decoded as $tagN when the writer class is unavailable, so the documented lookup by original name cannot work. Please store a stable schema field identity and expose an unambiguous lookup contract.
| Object target, FieldAccessor sinkAccessor, TypeDef typeDef, String name, Object value) { | ||
| ForyExtraFields extraField = (ForyExtraFields) sinkAccessor.getObject(target); | ||
| if (extraField == null) { | ||
| extraField = new ForyExtraFields(); |
There was a problem hiding this comment.
Charge retained sink owners to the graph-memory budget
The first capture retains a new ForyExtraFields, HashMap, backing/entry storage, and potentially a boxed primitive, but neither interpreter nor generated callers reserve graph memory for these owners. A valid list containing many partial objects can therefore grow the retained result far beyond maxGraphMemoryBytes. The compatible serializer/codegen owner should reserve stable lower-bound costs before allocating or growing the sink, and the budget rejection path needs a regression test.
| Generics generics = readContext.getGenerics(); | ||
| for (SerializationFieldInfo fieldInfo : allFields) { | ||
| fields[counter++] = readField(readContext, refReader, generics, fieldInfo, buffer, null); | ||
| fields[counter++] = |
There was a problem hiding this comment.
Do not opt records into a sink path that cannot capture fields
A record component of type ForyExtraFields is detected as a sink, but the interpreter record path reads into an Object[] with captureUnmatched=false, so unknown fields are still skipped. Generated code attempts to call capture before the record exists, using the record collector/array rather than a record instance, and fails on the accessor receiver. Please either implement constructor-owned sink collection for records or reject record sinks explicitly in code and documentation.
| FieldAccessor.class, | ||
| "createAccessor", | ||
| TypeRef.of(FieldAccessor.class), | ||
| getOrCreateField( |
There was a problem hiding this comment.
Keep the sink field's declaring class in generated accessors
The scanner returns the exact inherited sink Field, but this code discards its declaring class and resolves only beanClass + name. If a subclass legally hides the inherited sink with a different-typed field of the same name, generated code binds the subclass field. That causes valid input to fail and can write a ForyExtraFields reference into the wrong field on Unsafe-based accessors. Please retain the exact Field/accessor or resolve it through the original declaring class.
| private static Optional<FieldAccessor> scanForExtraField(Class<?> cls) { | ||
| for (Class<?> c = cls; c != null && c != Object.class; c = c.getSuperclass()) { | ||
| for (Field f : c.getDeclaredFields()) { | ||
| if (ForyExtraFields.class == f.getType()) { |
There was a problem hiding this comment.
Ignore static fields when discovering an extra-fields sink
Discovery checks only the field type, so a static ForyExtraFields constant is selected even though FieldAccessor.createAccessor rejects static fields. Serializer initialization then fails for an otherwise valid model, including xlang or non-compatible configurations where this feature should be irrelevant. Please restrict discovery to instance fields and add a static-field regression test.
|
|
||
| /** | ||
| * Opt-in sink for compatible-mode extra fields. A class participates by declaring a field of this | ||
| * type; the serialization framework detects it, excludes it from the normal field set, and routes |
There was a problem hiding this comment.
Actually exclude the selected sink from normal serialization metadata
The contract says the framework excludes this field, but no descriptor, TypeDef, or ObjectSerializer owner filters it. A fresh sink-bearing type therefore exposes this framework field in its local schema, and an initialized sink can be serialized as a normal nested object instead of replaying the remote schema. Please exclude the exact selected instance field in the owning descriptor path and assert that it is absent from the local TypeDef/body.
| typeInfo.setSerializer(this, newStaticGeneratedStructSerializer(sc, cls, typeDef)); | ||
| } else if (sc == CompatibleSerializer.class) { | ||
| typeInfo.setSerializer(this, new CompatibleSerializer(this, cls, typeDef)); | ||
| CompatibleSerializer<?> cs = new CompatibleSerializer<>(this, cls, typeDef); |
There was a problem hiding this comment.
Implement capture and replay for the required static and layer serializers
Only the runtime CompatibleSerializer and generated-compatible branches register replay serializers. StaticGeneratedStructSerializer still skips unknown fields, and the CompatibleLayer/ObjectStream path does the same. The issue this PR closes explicitly includes annotation processor, KSP, Scala derive, StaticCompatible, and CompatibleLayer support; deferring them in the guide leaves those supported Java surfaces silently dropping data, including the recommended GraalVM/static-codegen path. Please complete those owner paths or stop closing the full issue and narrow the documented scope.
| void setSerializer(TypeResolver resolver, Serializer<?> serializer) { | ||
| this.serializer = serializer; | ||
| needToWriteTypeDef = serializer != null && resolver.needToWriteTypeDef(serializer); | ||
| this.extraFieldsSinkAccessor = type == null ? null : ForyExtraFields.findSinkAccessor(type); |
There was a problem hiding this comment.
Preserve the sink accessor across TypeInfo copies
extraFieldsSinkAccessor is initialized only in this setter, while both copy(...) methods keep the serializer but drop this state. The supported sequence of materializing/registering a serializer before numeric class registration therefore produces a copied TypeInfo that can capture remote fields but no longer enters replay on write. Treat the accessor as type-owned metadata and initialize or preserve it in every constructor/copy path.
| extRegistry | ||
| .extraFieldsSerializers | ||
| .computeIfAbsent(cls, k -> new ConcurrentHashMap<>()) | ||
| .putIfAbsent(typeDefId, gen); |
There was a problem hiding this comment.
Replace the interpreter replay serializer after async compilation
The async path normally registers CompatibleSerializer first, then the compilation callback calls this method with the generated serializer. putIfAbsent leaves the interpreter entry permanently installed (or makes the result timing-dependent if compilation wins the race). The current JIT test checks the normal TypeInfo serializer rather than this replay cache, so it passes without exercising generated replay. Please perform a safe interpreter-to-generated handoff and assert the actual replay owner.
| * the associated Class (or its ClassLoader) from being collected. On Android/GraalVM it falls | ||
| * back to a ConcurrentHashMap. | ||
| */ | ||
| private static final ClassValueCache<Optional<FieldAccessor>> SINK_CACHE = |
There was a problem hiding this comment.
Avoid a process-global strong class cache on Android
The Android fallback for this static ClassValueCache is an unbounded ConcurrentHashMap<Class<?>, Object>. Both positive accessors and negative Optional.empty() entries strongly retain user classes, so repeatedly creating and discarding DexClassLoaders/Fory runtimes leaks the loaders across runtimes. The accessor already has a natural per-TypeInfo owner; please remove the process-global cache or provide a genuinely weak-key fallback.
| depth--; | ||
| return; | ||
| } | ||
| if (typeInfo.hasExtraFieldsSink() && tryWriteExtraFieldsSchema(resolver, typeInfo, obj)) { |
There was a problem hiding this comment.
Keep the opt-in feature out of unrelated object-write hot paths
This adds a call and branch to every dynamic object write, including types without a sink and xlang/same-schema configurations. The generated polymorphic path adds the same work, while serializer setup scans every class. Please gate the feature during cold native-compatible setup/codegen so unrelated hot paths remain unchanged, and provide the zero-overhead benchmark or generated-code assertion required for this performance-sensitive path.
| return typeDef; | ||
| } | ||
|
|
||
| public static void capture( |
There was a problem hiding this comment.
Keep mutation and reflection helpers out of the public sink API
The guide says application code has read-only access, but capture, findSinkField, and findSinkAccessor are public, unmarked implementation APIs. Callers can use them to mutate entries and replace the TypeDef association, breaking replay invariants while also exposing FieldAccessor as public surface. Move the generated-code bridge to an @Internal support owner and keep ForyExtraFields limited to the stable lookup API.
Adds ForyExtraFields sink, an opt-in mechanism for preserving unmatched remote fields during compatible-mode deserialization and replaying them on re-serialization.
Covers both the interpreter and generated codepaths,
Why?
When the upstream and downstream attributes are inconsistent, for example, the upstream is 10 fields, the downstream is not synchronized to update the field attributes, and there are only 9 attributes, then the downstream non-existent attributes will be ignored during deserialization.
What does this PR do?
This PR adds support for preserving unknown fields during compatible serialization using
ForyExtraFields.A class opts in by declaring a
ForyExtraFieldssink. Fields present in the remote schema but not in the local schema are captured in the sink.Captured fields are replayed using the original remote
TypeDef, allowing them to be preserved across compatible serialization round trips involving different schemas.Replay serializers are cached by
(class, TypeDef), allowing a local class to replayForyExtraFieldscaptured from multiple remote schemas.Tests cover both enabled and disabled reference tracking, as well as both
codegen=trueandcodegen=false.Related issues
fixes #1799
AI Contribution Checklist
yes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.AI Contribution Checklist
Does this PR introduce any user-facing change?
Benchmark