Skip to content

THRIFT-6204: Omit an unset default-requiredness field from the Go writer #3827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
slachiewicz wants to merge 2 commits into
base: master
Choose a base branch
from
+24 −13
Open

THRIFT-6204: Omit an unset default-requiredness field from the Go writer #3827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ac6689
THRIFT-6204: Omit an unset default-requiredness field from the Go writer
slachiewicz Sep 11, 2026
cfaeb78
Use t.Context and slices.Contains in the unset-union test
slachiewicz Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions compiler/cpp/src/thrift/generate/t_go_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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';
}
Expand Down
29 changes: 18 additions & 11 deletions lib/go/test/tests/union_default_value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@
package tests

import (
"context"
"strings"
"slices"
"testing"

"github.com/apache/thrift/lib/go/test/gopath/src/uniondefaultvaluetest"
Expand All @@ -43,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")
}
Expand All @@ -54,14 +53,22 @@ 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(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.
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(t.Context(), 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)
}
}
Loading