From e06448e67bb33e44a59bf7aca2ddf87e60d5e40e Mon Sep 17 00:00:00 2001 From: Adrian-Stefan Mares Date: Mon, 28 Mar 2022 18:20:16 +0200 Subject: [PATCH] setter.go,main_test.go: Relax oneof validation on set --- main_test.go | 244 ++++++++++++++++++++++++++++- module/setter.go | 41 ++--- testdata/testdata.pb.setters.fm.go | 85 +++------- 3 files changed, 284 insertions(+), 86 deletions(-) diff --git a/main_test.go b/main_test.go index bad5d44..c6d84c4 100644 --- a/main_test.go +++ b/main_test.go @@ -26,6 +26,7 @@ import ( "time" "github.com/TheThingsIndustries/protoc-gen-fieldmask/testdata" + "github.com/gogo/protobuf/types" "github.com/kr/pretty" "github.com/mohae/deepcopy" "github.com/smartystreets/assertions" @@ -490,12 +491,29 @@ var setFieldsTestCases = []struct { }, Paths: []string{"testOneof.d"}, Result: &testdata.Test{ - TestOneof: &testdata.Test_CustomNameOneof{ - CustomNameOneof: 42, + TestOneof: &testdata.Test_D{ + D: 42, + }, + G: &testdata.Empty{}, + }, + }, + { + Name: "destination testOneof empty", + Destination: &testdata.Test{ + G: &testdata.Empty{}, + }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_D{ + D: 42, + }, + }, + Paths: []string{"testOneof.d"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_D{ + D: 42, }, G: &testdata.Empty{}, }, - ErrorAssertion: func(t *testing.T, err error) bool { return assertions.New(t).So(err, should.BeError) }, }, { Name: "source testOneof mismatch", @@ -512,12 +530,48 @@ var setFieldsTestCases = []struct { }, Paths: []string{"testOneof.d"}, Result: &testdata.Test{ - TestOneof: &testdata.Test_D{ - D: 42, + G: &testdata.Empty{}, + }, + }, + { + Name: "source testOneof empty", + Destination: &testdata.Test{ + G: &testdata.Empty{}, + }, + Source: &testdata.Test{}, + Paths: []string{"testOneof.d"}, + Result: &testdata.Test{ + G: &testdata.Empty{}, + }, + }, + { + Name: "source+destination testOneof mismatch", + Destination: &testdata.Test{ + TestOneof: &testdata.Test_F{ + F: []byte{0x42}, }, G: &testdata.Empty{}, }, - ErrorAssertion: func(t *testing.T, err error) bool { return assertions.New(t).So(err, should.BeError) }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_CustomNameOneof{ + CustomNameOneof: 42, + }, + }, + Paths: []string{"testOneof.d"}, + Result: &testdata.Test{ + G: &testdata.Empty{}, + }, + }, + { + Name: "source+destination testOneof empty", + Destination: &testdata.Test{ + G: &testdata.Empty{}, + }, + Source: &testdata.Test{}, + Paths: []string{"testOneof.d"}, + Result: &testdata.Test{ + G: &testdata.Empty{}, + }, }, { Name: "unset testOneof", @@ -567,6 +621,184 @@ var setFieldsTestCases = []struct { }, }, }, + { + Name: "source testOneof.k.a.testNestedNestedOneOf.g mismatch", + Destination: &testdata.Test{}, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_F{ + F: 42, + }, + }, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + }, + { + Name: "source testOneof.k.a.testNestedNestedOneOf.g empty", + Destination: &testdata.Test{}, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + }, + { + Name: "destination testOneof.k.a.testNestedNestedOneOf.g mismatch", + Destination: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_F{ + F: 42, + }, + }, + }, + }, + }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_G{ + G: &types.UInt64Value{ + Value: 42, + }, + }, + }, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_G{ + G: &types.UInt64Value{ + Value: 42, + }, + }, + }, + }, + }, + }, + }, + { + Name: "destination testOneof.k.a.testNestedNestedOneOf.g empty", + Destination: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_G{ + G: &types.UInt64Value{ + Value: 42, + }, + }, + }, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_G{ + G: &types.UInt64Value{ + Value: 42, + }, + }, + }, + }, + }, + }, + }, + { + Name: "source+destination testOneof.k.a.testNestedNestedOneOf.g mismatch", + Destination: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_F{ + F: 42, + }, + }, + }, + }, + }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{ + TestNestedNestedOneOf: &testdata.Test_TestNested_TestNestedNested_E{ + E: &testdata.Empty{}, + }, + }, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + }, + { + Name: "source+destination testOneof.k.a.testNestedNestedOneOf.g empty", + Destination: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + Source: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + Paths: []string{"testOneof.k.a.testNestedNestedOneOf.g"}, + Result: &testdata.Test{ + TestOneof: &testdata.Test_K{ + K: &testdata.Test_TestNested{ + A: &testdata.Test_TestNested_TestNestedNested{}, + }, + }, + }, + }, { Name: "non-nullable c.a", Destination: &testdata.Test{ diff --git a/module/setter.go b/module/setter.go index d87eac8..e2a6f5e 100644 --- a/module/setter.go +++ b/module/setter.go @@ -37,18 +37,10 @@ func (m *setterModule) buildSetFieldsCase(buf *strings.Builder, imports importMa if f.InOneOf() { buildIndented(buf, tabCount+1, fmt.Sprintf(`_, srcOk := src.%s.(*%s) -if !srcOk && src.%s != nil { - return fmt.Errorf("attempt to set oneof '%s', while different oneof is set in source") -} _, dstOk := dst.%s.(*%s) -if !dstOk && dst.%s != nil { - return fmt.Errorf("attempt to set oneof '%s', while different oneof is set in destination") -}`, - m.ctx.Name(f.OneOf()), m.ctx.OneofOption(f), m.ctx.Name(f.OneOf()), - f.Name(), +_ = dstOk`, + m.ctx.Name(f.OneOf()), m.ctx.OneofOption(f), m.ctx.Name(f.OneOf()), m.ctx.OneofOption(f), - m.ctx.Name(f.OneOf()), - f.Name(), )) } @@ -70,11 +62,19 @@ if !dstOk && dst.%s != nil { fPath = m.ctx.Name(f.OneOf()).String() } - buildIndented(buf, tabCount, fmt.Sprintf(`if src != nil { + if f.InOneOf() { + buildIndented(buf, tabCount, fmt.Sprintf(`if srcOk { + dst.%s = src.%s + } else {`, + fPath, fPath, + )) + } else { + buildIndented(buf, tabCount, fmt.Sprintf(`if src != nil { dst.%s = src.%s } else {`, - fPath, fPath, - )) + fPath, fPath, + )) + } if goType.IsPointer() { buildIndented(buf, tabCount, fmt.Sprintf(` dst.%s = nil @@ -91,9 +91,9 @@ if !dstOk && dst.%s != nil { } if f.InOneOf() { - buildIndented(buf, tabCount, fmt.Sprintf(` dst.%s = &%s{} + buildIndented(buf, tabCount, fmt.Sprintf(` dst.%s = nil }`, - fPath, m.ctx.OneofOption(f), + fPath, )) return nil } @@ -128,22 +128,23 @@ if !dstOk && dst.%s != nil { switch { case f.InOneOf(): fPath := fmt.Sprintf("%s.(*%s).%s", m.ctx.Name(f.OneOf()), m.ctx.OneofOption(f), fName) - buildIndented(buf, tabCount+2, fmt.Sprintf(`if !srcOk && !dstOk { - continue -} -if srcOk { + buildIndented(buf, tabCount+2, fmt.Sprintf(`if srcOk { newSrc = src.%s } if dstOk { newDst = dst.%s -} else { +} else if srcOk { newDst = &%s{} dst.%s = &%s{%s: newDst} +} else { + dst.%s = nil + continue }`, fPath, fPath, goType.Value(), m.ctx.Name(f.OneOf()), m.ctx.OneofOption(f), fName, + m.ctx.Name(f.OneOf()), )) case goType.IsPointer(): diff --git a/testdata/testdata.pb.setters.fm.go b/testdata/testdata.pb.setters.fm.go index 3447cf7..3878845 100644 --- a/testdata/testdata.pb.setters.fm.go +++ b/testdata/testdata.pb.setters.fm.go @@ -170,83 +170,63 @@ func (dst *Test) SetFields(src *Test, paths ...string) error { switch oneofName { case "d": _, srcOk := src.TestOneof.(*Test_D) - if !srcOk && src.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'd', while different oneof is set in source") - } _, dstOk := dst.TestOneof.(*Test_D) - if !dstOk && dst.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'd', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { return fmt.Errorf("'d' has no subfields, but %s were specified", oneofSubs) } - if src != nil { + if srcOk { dst.TestOneof = src.TestOneof } else { - dst.TestOneof = &Test_D{} + dst.TestOneof = nil } case "e": _, srcOk := src.TestOneof.(*Test_CustomNameOneof) - if !srcOk && src.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'e', while different oneof is set in source") - } _, dstOk := dst.TestOneof.(*Test_CustomNameOneof) - if !dstOk && dst.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'e', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { return fmt.Errorf("'e' has no subfields, but %s were specified", oneofSubs) } - if src != nil { + if srcOk { dst.TestOneof = src.TestOneof } else { - dst.TestOneof = &Test_CustomNameOneof{} + dst.TestOneof = nil } case "f": _, srcOk := src.TestOneof.(*Test_F) - if !srcOk && src.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'f', while different oneof is set in source") - } _, dstOk := dst.TestOneof.(*Test_F) - if !dstOk && dst.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'f', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { return fmt.Errorf("'f' has no subfields, but %s were specified", oneofSubs) } - if src != nil { + if srcOk { dst.TestOneof = src.TestOneof } else { dst.TestOneof = nil } case "k": _, srcOk := src.TestOneof.(*Test_K) - if !srcOk && src.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'k', while different oneof is set in source") - } _, dstOk := dst.TestOneof.(*Test_K) - if !dstOk && dst.TestOneof != nil { - return fmt.Errorf("attempt to set oneof 'k', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { var newDst, newSrc *Test_TestNested - if !srcOk && !dstOk { - continue - } if srcOk { newSrc = src.TestOneof.(*Test_K).K } if dstOk { newDst = dst.TestOneof.(*Test_K).K - } else { + } else if srcOk { newDst = &Test_TestNested{} dst.TestOneof = &Test_K{K: newDst} + } else { + dst.TestOneof = nil + continue } if err := newDst.SetFields(newSrc, oneofSubs...); err != nil { return err } } else { - if src != nil { + if srcOk { dst.TestOneof = src.TestOneof } else { dst.TestOneof = nil @@ -458,32 +438,27 @@ func (dst *Test_TestNested_TestNestedNested) SetFields(src *Test_TestNested_Test switch oneofName { case "e": _, srcOk := src.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_E) - if !srcOk && src.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'e', while different oneof is set in source") - } _, dstOk := dst.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_E) - if !dstOk && dst.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'e', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { var newDst, newSrc *Empty - if !srcOk && !dstOk { - continue - } if srcOk { newSrc = src.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_E).E } if dstOk { newDst = dst.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_E).E - } else { + } else if srcOk { newDst = &Empty{} dst.TestNestedNestedOneOf = &Test_TestNested_TestNestedNested_E{E: newDst} + } else { + dst.TestNestedNestedOneOf = nil + continue } if err := newDst.SetFields(newSrc, oneofSubs...); err != nil { return err } } else { - if src != nil { + if srcOk { dst.TestNestedNestedOneOf = src.TestNestedNestedOneOf } else { dst.TestNestedNestedOneOf = nil @@ -491,34 +466,24 @@ func (dst *Test_TestNested_TestNestedNested) SetFields(src *Test_TestNested_Test } case "f": _, srcOk := src.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_F) - if !srcOk && src.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'f', while different oneof is set in source") - } _, dstOk := dst.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_F) - if !dstOk && dst.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'f', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { return fmt.Errorf("'f' has no subfields, but %s were specified", oneofSubs) } - if src != nil { + if srcOk { dst.TestNestedNestedOneOf = src.TestNestedNestedOneOf } else { - dst.TestNestedNestedOneOf = &Test_TestNested_TestNestedNested_F{} + dst.TestNestedNestedOneOf = nil } case "g": _, srcOk := src.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_G) - if !srcOk && src.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'g', while different oneof is set in source") - } _, dstOk := dst.TestNestedNestedOneOf.(*Test_TestNested_TestNestedNested_G) - if !dstOk && dst.TestNestedNestedOneOf != nil { - return fmt.Errorf("attempt to set oneof 'g', while different oneof is set in destination") - } + _ = dstOk if len(oneofSubs) > 0 { return fmt.Errorf("'g' has no subfields, but %s were specified", oneofSubs) } - if src != nil { + if srcOk { dst.TestNestedNestedOneOf = src.TestNestedNestedOneOf } else { dst.TestNestedNestedOneOf = nil