kpumuk commented on PR #3414:
URL: https://github.com/apache/thrift/pull/3414#issuecomment-4340407178
The remaining issue is that unknown union variants are swallowed in more
places than optional user-struct fields.
`render_struct_sync_read` is shared by normal structs, generated RPC args
structs, and generated RPC result structs. The new branch catches
`UnknownUnionVariant` unconditionally and leaves the field temp as `None`. That
is correct for an optional user-struct field, but not for required fields or
RPC wrapper fields.
Minimal IDL:
```thrift
union Response {
1: i32 known
}
service Svc {
Response get()
}
```
For a newer server response containing an unknown `Response` variant, the
generated result reader leaves `result_value` unset. Then
`SvcGetResult::ok_or()` reports:
```text
ApplicationErrorKind::MissingResult
```
That hides the real protocol/schema mismatch. Required union args/fields
have the same shape: the union read error is converted into a later
missing-field error.
Regression test template, placed inside the generated module or adapted into
an integration test:
```rust
#[test]
fn unknown_union_variant_in_rpc_result_is_not_missing_result() {
use std::io::Cursor;
use thrift::protocol::{
TBinaryInputProtocol, TBinaryOutputProtocol, TFieldIdentifier,
TOutputProtocol,
TSerializable, TStructIdentifier, TType,
};
let mut write_buf = Vec::new();
{
let cursor = Cursor::new(&mut write_buf);
let mut prot = TBinaryOutputProtocol::new(cursor, false);
prot.write_struct_begin(&TStructIdentifier::new("SvcGetResult")).unwrap();
prot.write_field_begin(&TFieldIdentifier::new("result_value",
TType::Struct, 0)).unwrap();
prot.write_struct_begin(&TStructIdentifier::new("Response")).unwrap();
prot.write_field_begin(&TFieldIdentifier::new("future_variant",
TType::I32, 99)).unwrap();
prot.write_i32(7).unwrap();
prot.write_field_end().unwrap();
prot.write_field_stop().unwrap();
prot.write_struct_end().unwrap();
prot.write_field_end().unwrap();
prot.write_field_stop().unwrap();
prot.write_struct_end().unwrap();
}
let read_cursor = Cursor::new(write_buf);
let mut rprot = TBinaryInputProtocol::new(read_cursor, false);
let result = SvcGetResult::read_from_in_protocol(&mut rprot).unwrap();
let err = result
.ok_or()
.expect_err("unknown success union variant should not become
MissingResult");
assert!(matches!(
err,
thrift::Error::Protocol(ref e)
if e.kind == thrift::ProtocolErrorKind::UnknownUnionVariant
));
}
```
The fix should only suppress `UnknownUnionVariant` where treating the field
as absent is actually intended, e.g. optional regular struct fields. For
required fields and RPC args/result structs, the original union error should
propagate.
--
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]
