[
https://issues.apache.org/jira/browse/THRIFT-4650?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16651222#comment-16651222
]
ASF GitHub Bot commented on THRIFT-4650:
----------------------------------------
dcelasun closed pull request #1610: THRIFT-4650: fixes incorrectly marking
required fields as being set
URL: https://github.com/apache/thrift/pull/1610
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc
b/compiler/cpp/src/thrift/generate/t_go_generator.cc
index b5742f6ec0..0807efbe9e 100644
--- a/compiler/cpp/src/thrift/generate/t_go_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc
@@ -1591,17 +1591,19 @@ void t_go_generator::generate_go_struct_reader(ostream&
out,
<< endl;
out << indent() << " return err" << endl;
out << indent() << " }" << endl;
+
+ // Mark required field as read
+ if ((*f_iter)->get_req() == t_field::T_REQUIRED) {
+ const string field_name(publicize(escape_string((*f_iter)->get_name())));
+ out << indent() << " isset" << field_name << " = true" << endl;
+ }
+
out << indent() << "} else {" << endl;
out << indent() << " if err := iprot.Skip(fieldTypeId); err != nil {" <<
endl;
out << indent() << " return err" << endl;
out << indent() << " }" << endl;
out << indent() << "}" << endl;
- // Mark required field as read
- if ((*f_iter)->get_req() == t_field::T_REQUIRED) {
- const string field_name(publicize(escape_string((*f_iter)->get_name())));
- out << indent() << "isset" << field_name << " = true" << endl;
- }
indent_down();
}
diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am
index b7ba870857..78d468129c 100644
--- a/lib/go/test/Makefile.am
+++ b/lib/go/test/Makefile.am
@@ -27,6 +27,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
MultiplexedProtocolTest.thrift \
OnewayTest.thrift \
OptionalFieldsTest.thrift \
+ RequiredFieldTest.thrift \
ServicesTest.thrift \
GoTagTest.thrift \
TypedefFieldTest.thrift \
@@ -46,6 +47,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
$(THRIFT) $(THRIFTARGS) MultiplexedProtocolTest.thrift
$(THRIFT) $(THRIFTARGS) OnewayTest.thrift
$(THRIFT) $(THRIFTARGS) OptionalFieldsTest.thrift
+ $(THRIFT) $(THRIFTARGS) RequiredFieldTest.thrift
$(THRIFT) $(THRIFTARGS) ServicesTest.thrift
$(THRIFT) $(THRIFTARGS) GoTagTest.thrift
$(THRIFT) $(THRIFTARGS) TypedefFieldTest.thrift
@@ -96,6 +98,7 @@ EXTRA_DIST = \
NamespacedTest.thrift \
OnewayTest.thrift \
OptionalFieldsTest.thrift \
+ RequiredFieldTest.thrift \
RefAnnotationFieldsTest.thrift \
UnionDefaultValueTest.thrift \
UnionBinaryTest.thrift \
diff --git a/lib/go/test/RequiredFieldTest.thrift
b/lib/go/test/RequiredFieldTest.thrift
new file mode 100644
index 0000000000..4a2dcaeba9
--- /dev/null
+++ b/lib/go/test/RequiredFieldTest.thrift
@@ -0,0 +1,7 @@
+struct RequiredField {
+ 1: required string name
+}
+
+struct OtherThing {
+ 1: required i16 value
+}
diff --git a/lib/go/test/tests/required_fields_test.go
b/lib/go/test/tests/required_fields_test.go
index 287ef60c3b..3fa414ad82 100644
--- a/lib/go/test/tests/required_fields_test.go
+++ b/lib/go/test/tests/required_fields_test.go
@@ -20,12 +20,45 @@
package tests
import (
+ "context"
"github.com/golang/mock/gomock"
"optionalfieldstest"
+ "requiredfieldtest"
"testing"
"thrift"
)
+func TestRequiredField_SucecssWhenSet(t *testing.T) {
+ // create a new RequiredField instance with the required field set
+ source := &requiredfieldtest.RequiredField{Name: "this is a test"}
+ sourceData, err := thrift.NewTSerializer().Write(context.Background(),
source)
+ if err != nil {
+ t.Fatalf("failed to serialize %T: %v", source, err)
+ }
+
+ d := thrift.NewTDeserializer()
+ err = d.Read(&requiredfieldtest.RequiredField{}, sourceData)
+ if err != nil {
+ t.Fatalf("Did not expect an error when trying to deserialize
the requiredfieldtest.RequiredField: %v", err)
+ }
+}
+
+func TestRequiredField_ErrorWhenMissing(t *testing.T) {
+ // create a new OtherThing instance, without setting the required field
+ source := &requiredfieldtest.OtherThing{}
+ sourceData, err := thrift.NewTSerializer().Write(context.Background(),
source)
+ if err != nil {
+ t.Fatalf("failed to serialize %T: %v", source, err)
+ }
+
+ // attempt to deserialize into a different type (which should fail)
+ d := thrift.NewTDeserializer()
+ err = d.Read(&requiredfieldtest.RequiredField{}, sourceData)
+ if err == nil {
+ t.Fatal("Expected an error when trying to deserialize an object
which is missing a required field")
+ }
+}
+
func TestStructReadRequiredFields(t *testing.T) {
mockCtrl := gomock.NewController(t)
protocol := NewMockTProtocol(mockCtrl)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Required field incorrectly marked as set when fieldType does not match
> ----------------------------------------------------------------------
>
> Key: THRIFT-4650
> URL: https://issues.apache.org/jira/browse/THRIFT-4650
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Affects Versions: 0.11.0
> Reporter: Craig W
> Priority: Major
> Fix For: 0.12.0
>
>
> The "Read" function that gets generated incorrectly marks required fields as
> being set when the type of the field does not match what's expected.
> {{For example, the following IDL:}}
> {{struct Foo {}}
> {{ 1: required string id}}
> {{}}}
> {{The generated Read function has the following:}}
> {{switch fieldId {}}
> {{ case 1:}}
> {{ if fieldTypeId == thrift.STRING {}}
> {{ if err := p.ReadField1(iprot); err != nil {}}
> {{ return err}}
> }
> {{ } else {}}
> {{ if err := iprot.Skip(fieldTypeId); err != nil {}}
> {{ return err}}
> }
> }
> {{ issetID = true}}
> {{}}}
> {{I ran into a case where I attempted to deserialize another message type
> into type Foo, and I expected it to return an error, but it did not.}}
> f : = Foo{}
> err := thrift.NewTDeserializer().Read(&f, bytes)
> {{I think [this
> code|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_go_generator.cc#L1600]
> in the generator needs to be moved inside the first "if" block, so that its
> only set to true if it actually reads the data of the correct type.}}
> {{ }}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
