From 7ac66891475dc08afab1c3f4717f7c971976aa03 Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Fri, 11 Sep 2026 09:21:15 +0200 Subject: [PATCH 1/2] THRIFT-6204: Omit an unset default-requiredness field from the Go writer Client: go Default requiredness means "write if set" (doc/specs/idl.md), and Python, Java, Node.js and C# all omit such a field when it holds no value. The Go writer wrote it unconditionally, so a nil pointer field either panicked on the way out or, when its struct had no members, went onto the wire as an empty struct. Guard the write with the generated IsSet helper, as optional fields already are. This changes bytes on the wire, and not only for the case that panicked. A nil field whose struct has no members is written today and is omitted after this: before: 0c 00 01 00 08 00 02 00 00 00 07 00 after: 08 00 02 00 00 00 07 00 A peer that declares that field required accepts the first and rejects the second with "Required field E is not set". is_pointer_field() is true for every struct, union and exception field whatever its requiredness, and for every cpp.ref field, so the guard is wider than unions alone. Regenerating every IDL under test/, lib/go/test/ and tutorial/ changes 75 of 650 generated files and guards 227 write sites, 59 of the changed files holding service Args and Result structs. Co-Authored-By: Claude Fable 5.1 --- .../cpp/src/thrift/generate/t_go_generator.cc | 8 ++++-- lib/go/test/tests/union_default_value_test.go | 28 +++++++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc index 5f7828b8683..86d3aaa27e7 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc @@ -2122,7 +2122,11 @@ void t_go_generator::generate_go_struct_writer(ostream& out, << "(ctx context.Context, oprot thrift.TProtocol) (err error) {" << '\n'; indent_up(); - if (field_required == t_field::T_OPTIONAL) { + // Default requiredness means "write if set" (doc/specs/idl.md), and a + // pointer field is unset exactly when it is nil. + bool check_if_set = field_required == t_field::T_OPTIONAL + || (field_required == t_field::T_OPT_IN_REQ_OUT && is_pointer_field(*f_iter)); + if (check_if_set) { out << indent() << "if p.IsSet" << publicize(field_name) << "() {" << '\n'; indent_up(); } @@ -2146,7 +2150,7 @@ void t_go_generator::generate_go_struct_writer(ostream& out, indent_down(); out << indent() << "}" << '\n'; - if (field_required == t_field::T_OPTIONAL) { + if (check_if_set) { indent_down(); out << indent() << "}" << '\n'; } diff --git a/lib/go/test/tests/union_default_value_test.go b/lib/go/test/tests/union_default_value_test.go index f90a0e0ed04..58efc10a52d 100644 --- a/lib/go/test/tests/union_default_value_test.go +++ b/lib/go/test/tests/union_default_value_test.go @@ -21,7 +21,6 @@ package tests import ( "context" - "strings" "testing" "github.com/apache/thrift/lib/go/test/gopath/src/uniondefaultvaluetest" @@ -54,14 +53,25 @@ func TestStructWithUnsetUnion(t *testing.T) { buf := thrift.NewTMemoryBuffer() proto := thrift.NewTBinaryProtocolConf(buf, nil) - // Writing a struct whose union field is nil used to dereference the nil - // receiver inside CountSetFields and panic. It now reports which union - // could not be written instead. - err := s.Write(context.Background(), proto) - if err == nil { - t.Fatal("Expected an error writing a struct with an unset union, got nil") + // Default requiredness means "write if set", so a nil union field is + // left off the wire rather than reported as an error. + if err := s.Write(context.Background(), proto); err != nil { + t.Fatalf("Unexpected error writing a struct with an unset union: %v", err) + } + + // Field 2 must not appear: the only field header on the wire is field 1. + for _, b := range buf.Bytes() { + if b == 0x0c { + t.Errorf("Expected no struct field header on the wire, got % x", buf.Bytes()) + break + } + } + + readBack := uniondefaultvaluetest.NewStructWithUnsetUnion() + if err := readBack.Read(context.Background(), thrift.NewTBinaryProtocolConf(buf, nil)); err != nil { + t.Fatalf("Unexpected error reading the struct back: %v", err) } - if !strings.Contains(err.Error(), "exactly one field must be set") { - t.Errorf("Expected the union arity error, got %v", err) + if readBack.F_2 != nil { + t.Errorf("Expected readBack.F_2 to be nil, got %+v", readBack.F_2) } } From cfaeb7890408d1194fb9e82ae29f917b2cbca5ff Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Tue, 15 Sep 2026 22:48:41 +0200 Subject: [PATCH 2/2] Use t.Context and slices.Contains in the unset-union test Client: go Co-Authored-By: Claude Fable 5.1 --- lib/go/test/tests/union_default_value_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/go/test/tests/union_default_value_test.go b/lib/go/test/tests/union_default_value_test.go index 58efc10a52d..fdd0b0332ef 100644 --- a/lib/go/test/tests/union_default_value_test.go +++ b/lib/go/test/tests/union_default_value_test.go @@ -20,7 +20,7 @@ package tests import ( - "context" + "slices" "testing" "github.com/apache/thrift/lib/go/test/gopath/src/uniondefaultvaluetest" @@ -42,7 +42,7 @@ func TestNilUnion(t *testing.T) { } proto := thrift.NewTBinaryProtocolConf(thrift.NewTMemoryBuffer(), nil) - err := d.Write(context.Background(), proto) + err := d.Write(t.Context(), proto) if err == nil { t.Error("Expected error when writing nil union, got nil") } @@ -55,20 +55,17 @@ func TestStructWithUnsetUnion(t *testing.T) { // Default requiredness means "write if set", so a nil union field is // left off the wire rather than reported as an error. - if err := s.Write(context.Background(), proto); err != nil { + if err := s.Write(t.Context(), proto); err != nil { t.Fatalf("Unexpected error writing a struct with an unset union: %v", err) } // Field 2 must not appear: the only field header on the wire is field 1. - for _, b := range buf.Bytes() { - if b == 0x0c { - t.Errorf("Expected no struct field header on the wire, got % x", buf.Bytes()) - break - } + if slices.Contains(buf.Bytes(), byte(thrift.STRUCT)) { + t.Errorf("Expected no struct field header on the wire, got % x", buf.Bytes()) } readBack := uniondefaultvaluetest.NewStructWithUnsetUnion() - if err := readBack.Read(context.Background(), thrift.NewTBinaryProtocolConf(buf, nil)); err != nil { + if err := readBack.Read(t.Context(), thrift.NewTBinaryProtocolConf(buf, nil)); err != nil { t.Fatalf("Unexpected error reading the struct back: %v", err) } if readBack.F_2 != nil {