Rust codegen: forward-compatible union deserialization in structs#3414
Open
santiagomed wants to merge 1 commit intoapache:masterfrom
Open
Rust codegen: forward-compatible union deserialization in structs#3414santiagomed wants to merge 1 commit intoapache:masterfrom
santiagomed wants to merge 1 commit intoapache:masterfrom
Conversation
6c16a79 to
d3225c2
Compare
…n structs
When a struct field has a union type, the generated Rust deserializer
now catches 'received empty union' errors from unknown variants and
treats them as None instead of propagating the error.
Previously, if a Thrift union received a field ID not present in the
local IDL, the union deserializer returned Err('received empty union'),
which cascaded up and caused the entire parent struct deserialization
to fail. This broke forward compatibility.
The fix wraps union field reads in struct deserializers with a match
that catches the specific 'received empty union' error and silently
skips the field. The struct field (already Option<UnionType>) remains
None, matching the behavior as if the field were absent. All other
errors (malformed data, I/O failures) still propagate normally.
Handles both direct union fields and Box<Union> (forward typedef)
fields by resolving the type before generating the method call.
This aligns Rust with Java, Go, Python, and C++ Thrift behavior.
d3225c2 to
61e19a6
Compare
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.
Problem
When a struct contains a union-typed field and the wire data has a union variant (field ID) not present in the local IDL, the generated Rust code fails to deserialize the entire struct — not just the unknown union field.
The root cause: the union's
read_from_in_protocolcorrectly returnsErr("received empty union")for unknown variants (fixed in THRIFT-5926 / PR #3336). But the parent struct's generated deserializer propagates this error via?, causing the whole struct to fail.This breaks Thrift's forward-compatibility guarantee. When a server adds a new union variant, all clients running an older IDL version fail to parse any message containing that variant — even though the client doesn't need to understand the new variant to process the rest of the struct.
Java, Go, Python, and C++ Thrift all handle this gracefully: unknown union variants are silently skipped, and the struct field is left unset.
Fix
In
render_struct_sync_read, detect struct fields whose type is a union. For those fields, wrap theread_from_in_protocolcall in amatchthat catches the"received empty union"error and treats it asNone(field absent):Non-union fields are unchanged. The union's own
TSerializableimpl is unchanged — it still returnsErrfor unknown variants, preserving the contract for callers that read unions directly.Impact
TSerializabletrait or the thrift runtime libraryNoneinstead of causing parse failure