-
Notifications
You must be signed in to change notification settings - Fork 357
fix(rust): enable Union type cross-language serialization between Rust and Java #3094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(rust): enable Union type cross-language serialization between Rust and Java #3094
Conversation
19cada8 to
b11e307
Compare
|
@ariesdevil Most of such bugs are caused by the type system differences between laungages. I'm refactoring the whole xlang serialization to mark all fields not null by default, I think this refactor will resolve most such tricky issue. here is the principle: Java:
Rust
Go
C++
Python
For native serialization, keep current nullable/ref principles for max compatibility |
060bf8e to
ef83261
Compare
|
@chaokunyang Thanks for informing me of the changes. I updated the PR desc and some code, PTAL again. |
|
Hi @ariesdevil , I finished xlang nullable refactor in #3093. It conflict with your pr, could you megre main branch and resolve the conflict? And since all fields are not null in rust by default except Optional, maybe the problem is more simple. For enum, java and c++ will take it as not null by default now. |
ef83261 to
33f5fb6
Compare
java/fory-core/src/main/java/org/apache/fory/serializer/AbstractObjectSerializer.java
Outdated
Show resolved
Hide resolved
4e2f868 to
dc3fe7d
Compare
java/fory-core/src/main/java/org/apache/fory/serializer/AbstractObjectSerializer.java
Outdated
Show resolved
Hide resolved
dc3fe7d to
10a2913
Compare
|
|
||
| #[inline(always)] | ||
| fn fory_actual_type_id(type_id: u32, register_by_name: bool, compatible: bool) -> u32 { | ||
| fn fory_actual_type_id(type_id: u32, register_by_name: bool, compatible: bool, xlang: bool) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this xlang param really necessary? could we avoid add this parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not in hotpath, add a new parameter is OK. Maybe we should remove this method in future, and handle type id idrectly in type_resolver.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's necessary, for union-compatible enum, xlang specific returned TypeId:
xlang=true-> returnTypeId::UNIONfor cross language modexlang=falsereturn enum TypeId otherwise
10a2913 to
5c8e84e
Compare
5c8e84e to
c84bba7
Compare
c84bba7 to
05bce45
Compare
chaokunyang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks for your patient review, I learned a lot. |
|
@ariesdevil #3107 refactored java ObjectSerializer, we don't need special code path for enum now |
@chaokunyang Wonderful job, happy new year : ) |
Why?
Struct fingerprint mismatch for enum fields
When computing struct version hash for cross-language compatibility, Java and C++ treat enum fields as nullable=true by default. However, Rust's proc-macro cannot determine at compile time whether a field type is an enum, causing fingerprint mismatch.
Cross-language Union serialization fails
Java's AbstractObjectSerializer expects to read type_id for non-final fields, but Rust/C++ skip type_id for Union fields per xlang spec. This caused Type id 104 not registered errors when Java tried to deserialize Rust-serialized Union data.
What does this PR do?
Rust changes:
field_need_write_type_info()to handle UNION TypeIdJava changes:
AbstractObjectSerializer: Skiptype_idread for Union typesUnionSerializer: Addread()/write()delegating toxread()/xwrite()Tests:
Related issues
Does this PR introduce any user-facing change?
[ ] Does this PR introduce any public API change?
[x] Does this PR introduce any binary protocol compatibility change?
Note: Struct version hash for structs containing enum fields will now match Java/C++.
Benchmark
Others
It's really hard to determine this bug 😭...