proto: fix handling of required fields after multiple violations (#679)
The previous logic only treated a required not set violation as non-fatal
for the first instance. Afterwards, the logic incorrectly switched back
to being fatal.
diff --git a/proto/all_test.go b/proto/all_test.go
index a68d91d..1bea4b6 100644
--- a/proto/all_test.go
+++ b/proto/all_test.go
@@ -2324,6 +2324,28 @@
}
}
+func TestRequired(t *testing.T) {
+ // The F_BoolRequired field appears after all of the required fields.
+ // It should still be handled even after multiple required field violations.
+ m := &GoTest{F_BoolRequired: Bool(true)}
+ got, err := Marshal(m)
+ if _, ok := err.(*RequiredNotSetError); !ok {
+ t.Errorf("Marshal() = %v, want RequiredNotSetError error", err)
+ }
+ if want := []byte{0x50, 0x01}; !bytes.Equal(got, want) {
+ t.Errorf("Marshal() = %x, want %x", got, want)
+ }
+
+ m = new(GoTest)
+ err = Unmarshal(got, m)
+ if _, ok := err.(*RequiredNotSetError); !ok {
+ t.Errorf("Marshal() = %v, want RequiredNotSetError error", err)
+ }
+ if !m.GetF_BoolRequired() {
+ t.Error("m.F_BoolRequired = false, want true")
+ }
+}
+
// Benchmarks
func testMsg() *GoTest {
diff --git a/proto/table_marshal.go b/proto/table_marshal.go
index eafe04d..b167944 100644
--- a/proto/table_marshal.go
+++ b/proto/table_marshal.go
@@ -252,11 +252,13 @@
}
}
for _, f := range u.fields {
- if f.required && errLater == nil {
+ if f.required {
if ptr.offset(f.field).getPointer().isNil() {
// Required field is not set.
// We record the error but keep going, to give a complete marshaling.
- errLater = &RequiredNotSetError{f.name}
+ if errLater == nil {
+ errLater = &RequiredNotSetError{f.name}
+ }
continue
}
}
@@ -2592,7 +2594,7 @@
p := toAddrPointer(&v, ei.isptr)
b, err = ei.marshaler(b, p, 3<<3|WireBytes, deterministic)
b = append(b, 1<<3|WireEndGroup)
- if nerr.Merge(err) {
+ if !nerr.Merge(err) {
return b, err
}
}
diff --git a/proto/table_unmarshal.go b/proto/table_unmarshal.go
index de868ae..ebf1caa 100644
--- a/proto/table_unmarshal.go
+++ b/proto/table_unmarshal.go
@@ -175,10 +175,12 @@
reqMask |= f.reqMask
continue
}
- if r, ok := err.(*RequiredNotSetError); ok && errLater == nil {
+ if r, ok := err.(*RequiredNotSetError); ok {
// Remember this error, but keep parsing. We need to produce
// a full parse even if a required field is missing.
- errLater = r
+ if errLater == nil {
+ errLater = r
+ }
reqMask |= f.reqMask
continue
}
