Skip to content

Commit

Permalink
THRIFT-5845: Return TException for union check in Write
Browse files Browse the repository at this point in the history
Client: go

In compiler generated Write method for union types, return a TException
(TProtocolException) when the number of fields set is not exactly 1, to
help customer logic to decide whether to reuse a connection after an
error.

While I'm here, also do the same thing for the uniqueness check failure
for set fields in Write as well.
  • Loading branch information
fishy committed Jan 9, 2025
1 parent 947ad66 commit 54aa74a
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 7 deletions.
12 changes: 6 additions & 6 deletions compiler/cpp/src/thrift/generate/t_go_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1701,7 +1701,7 @@ void t_go_generator::generate_go_struct_reader(ostream& out,
out << indent() << "if !isset" << field_name << "{" << '\n';
indent_up();
out << indent() << "return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, "
"fmt.Errorf(\"Required field " << field_name << " is not set\"));" << '\n';
<< "fmt.Errorf(\"Required field " << field_name << " is not set\"))" << '\n';
indent_down();
out << indent() << "}" << '\n';
}
Expand Down Expand Up @@ -1748,7 +1748,8 @@ void t_go_generator::generate_go_struct_writer(ostream& out,
std::string tstruct_name(publicize(tstruct->get_name()));
out << indent() << "if c := p.CountSetFields" << tstruct_name << "(); c != 1 {" << '\n';
indent_up();
out << indent() << "return fmt.Errorf(\"%T write union: exactly one field must be set (%d set)\", p, c)" << '\n';
out << indent() << "return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, "
<< "fmt.Errorf(\"%T write union: exactly one field must be set (%d set)\", p, c))" << '\n';
indent_down();
out << indent() << "}" << '\n';
}
Expand Down Expand Up @@ -3719,10 +3720,9 @@ void t_go_generator::generate_serialize_container(ostream& out,
indent_down();
out << indent() << "}(" << wrapped_prefix << "[i], " << wrapped_prefix << "[j]) {" << '\n';
indent_up();
out << indent()
<< "return thrift.PrependError(\"\", fmt.Errorf(\"%T error writing set field: slice is not "
"unique\", "
<< wrapped_prefix << "))" << '\n';
out << indent() << "return thrift.NewTProtocolExceptionWithType(thrift.INVALID_DATA, "
<< "fmt.Errorf(\"%T error writing set field: slice is not " "unique\", "
<< wrapped_prefix << "))" << '\n';
indent_down();
out << indent() << "}" << '\n';
indent_down();
Expand Down
3 changes: 2 additions & 1 deletion lib/go/test/UnionBinaryTest.thrift
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@
union Sample {
1: map<string, string> u1,
2: binary u2,
3: list<string> u3
3: list<string> u3,
4: set<string> u4,
}
69 changes: 69 additions & 0 deletions lib/go/test/tests/write_texception_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package tests

import (
"context"
"errors"
"testing"

"github.com/apache/thrift/lib/go/test/gopath/src/unionbinarytest"
"github.com/apache/thrift/lib/go/thrift"
)

func TestWriteUnionTException(t *testing.T) {
// See https://issues.apache.org/jira/browse/THRIFT-5845
s := unionbinarytest.NewSample()
proto := thrift.NewTBinaryProtocolConf(thrift.NewTMemoryBuffer(), nil)
err := s.Write(context.Background(), proto)
t.Log(err)
if err == nil {
t.Fatal("Writing empty union did not produce error")
}
var te thrift.TException
if !errors.As(err, &te) {
t.Fatalf("Error from writing empty union is not TException: (%T) %v", err, err)
}
if typ := te.TExceptionType(); typ != thrift.TExceptionTypeProtocol && typ != thrift.TExceptionTypeTransport {
t.Errorf("Got TExceptionType %v, want one of TProtocolException or TTransportException", typ)
}
}

func TestWriteSetTException(t *testing.T) {
// See https://issues.apache.org/jira/browse/THRIFT-5845
s := unionbinarytest.NewSample()
s.U4 = []string{
"foo",
"foo", // duplicate
}
proto := thrift.NewTBinaryProtocolConf(thrift.NewTMemoryBuffer(), nil)
err := s.Write(context.Background(), proto)
t.Log(err)
if err == nil {
t.Fatal("Writing duplicate set did not produce error")
}
var te thrift.TException
if !errors.As(err, &te) {
t.Fatalf("Error from writing duplicate set is not TException: (%T) %v", err, err)
}
if typ := te.TExceptionType(); typ != thrift.TExceptionTypeProtocol && typ != thrift.TExceptionTypeTransport {
t.Errorf("Got TExceptionType %v, want one of TProtocolException or TTransportException", typ)
}
}

0 comments on commit 54aa74a

Please sign in to comment.