[
https://issues.apache.org/jira/browse/THRIFT-6204?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6204.
--------------------------------
Fix Version/s: 0.26.0
Assignee: Sylwester Lachiewicz
Resolution: Fixed
> Go writes an unset default-requiredness struct field instead of omitting it
> ---------------------------------------------------------------------------
>
> Key: THRIFT-6204
> URL: https://issues.apache.org/jira/browse/THRIFT-6204
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Reporter: Sylwester Lachiewicz
> Assignee: Sylwester Lachiewicz
> Priority: Major
> Fix For: 0.26.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> For a struct field with default requiredness whose type is a struct, union or
> exception, the Go generator emits an unconditional write. When such a field
> is nil at runtime, neither outcome matches the IDL specification:
> * if the field's struct has at least one member, the write path dereferences
> the nil pointer and panics;
> * if it has no members, the field goes onto the wire as an empty struct.
> [doc/specs/idl.md|https://github.com/apache/thrift/blob/master/doc/specs/idl.md]
> gives default requiredness "write if set" semantics, which Python, Java,
> Node.js and C# implement by omitting the field.
> h3. Why this is not a drop-in fix
> Guarding the write with the generated {{IsSet}} helper is a one-line change
> in
> [t_go_generator.cc|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_go_generator.cc],
> but it changes the bytes on the wire for the second case above, where
> nothing crashes today.
> Holder with a nil zero-member struct field 1 and {{i32 n = 7}} in field 2,
> binary protocol:
> {noformat}
> master: 0c 00 01 00 08 00 02 00 00 00 07 00 field 1 present, as an empty
> struct
> guarded: 08 00 02 00 00 00 07 00 field 1 absent
> {noformat}
> A peer that declares field 1 as {{required}} accepts the first and rejects
> the second:
> {noformat}
> master bytes -> err=<nil>
> guarded bytes -> err=Required field E is not set
> {noformat}
> h3. Blast radius
> Regenerating every IDL under {{test/}}, {{lib/go/test/}} and {{tutorial/}}
> with and without the guard:
> || Measure || Count ||
> | generated files changed | 75 of 650 |
> | write sites newly guarded | 227 |
> | changed files containing service {{Args}}/{{Result}} structs | 59 |
> The last row is the one that matters: RPC argument and result encoding is in
> scope, not just user-declared structs.
> h3. Scope note
> {{is_pointer_field()}} returns true for every struct, union and exception
> field regardless of requiredness, and for every field carrying a {{cpp.ref}}
> annotation. A guard keyed on it therefore reaches considerably further than
> the union case that first surfaced this.
> Because working code changes shape, this looks like a change for a major
> version, or one behind a {{go:}} generator option, rather than something to
> fold into a bug fix. The nil-union half, which only turns a panic into an
> error and touches no wire bytes, is split off as THRIFT-5806.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
