Sylwester Lachiewicz created THRIFT-6204:
--------------------------------------------
Summary: 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
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)
