Repository navigation
Copy the property values in BeanSerializer.copy - #1535
Merged
Merged
Conversation
BeanSerializer.copy set the property values of the original on the copy, so Kryo.copy returned a shallow copy of a bean: the copy shared mutable property values with the original, and a property that referenced the bean referenced the original instead of the copy. The copy is now referenced before the property values are copied with Kryo.copy, like in FieldSerializer.copy. With Kryo.copyShallow, the property values are still set as they are.
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified, and all reviewers assessed it as ready.
No open issues 🎉
What changed in this PR
Fixes BeanSerializer.copy to deep-copy bean properties while preserving self-references and shallow-copy behavior.
Changes:
- References the copied bean before copying properties.
- Deep-copies property values with
Kryo.copy. - Adds regression coverage for deep, circular, and shallow copies.
| File | Description |
|---|---|
test/com/esotericsoftware/kryo/serializers/BeanSerializerTest.java |
Tests deep-copy isolation, self-references, and shallow copies. |
src/com/esotericsoftware/kryo/serializers/BeanSerializer.java |
Corrects bean property copying and reference handling. |
Review effort: Lite
Collaborator
|
Thanks again @CCCLL! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1534.
BeanSerializer.copyset the property values of the original on the copy, soKryo.copyreturned a shallow copy of a bean: the copy shared mutable property values with the original, and a property that referenced the bean referenced the original instead of the copy.The copy is now referenced with
Kryo.referencebefore the property values are copied withKryo.copy, like inFieldSerializer.copy. WithKryo.copyShallow,Kryo.copyreturns the property values as they are, so a shallow copy is unchanged. Like withFieldSerializer, a deep copy now needs the classes of the property values to be registered if registration is required.BeanSerializerTest.testCopychecks that a copy doesn't share a property value with the original, that a property referencing the bean references the copy, and that a shallow copy still shares the property values. It fails without the fix, and with either change alone: withoutKryo.copythe property value is shared, and withoutKryo.referencecopying the bean that references itself overflows the stack.mvn -pl main testpasses on JDK 8 (built with JDK 11), 11, 17, 21 and 25, andmvn -pl main formatter:validatepasses. With the change applied to kryo-6, its tests pass on JDK 17, 21 and 25.