From d69476f04e3c907932ed3a00a5becff839b5320b Mon Sep 17 00:00:00 2001 From: noahdietz Date: Fri, 16 Feb 2024 09:23:00 -0800 Subject: [PATCH] fix(AIP-4232): correct repeated field check --- rules/aip4232/repeated_fields.go | 22 +++++++++++++--------- rules/aip4232/repeated_fields_test.go | 22 ++++++++++++++++------ rules/internal/utils/find.go | 5 +++-- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/rules/aip4232/repeated_fields.go b/rules/aip4232/repeated_fields.go index e223eddf3..89748df35 100644 --- a/rules/aip4232/repeated_fields.go +++ b/rules/aip4232/repeated_fields.go @@ -33,17 +33,21 @@ var repeatedFields = &lint.MethodRule{ in := m.GetInputType() for _, sig := range sigs { - for i, name := range sig { - // Skip the last field in the signature, it can be repeated. - if i == len(sig)-1 { - break + for _, name := range sig { + if !strings.Contains(name, ".") { + continue } + chain := strings.Split(name, ".") - if f := in.FindFieldByName(name); f != nil && f.IsRepeated() { - problems = append(problems, lint.Problem{ - Message: fmt.Sprintf("Field %q in method_signature %q: only the last field can be repeated.", name, strings.Join(sig, ",")), - Descriptor: m, - }) + // Exclude the last one from the look up since it can be repeated. + for i := range chain[:len(chain)-1] { + n := strings.Join(chain[:i+1], ".") + if f := utils.FindFieldDotNotation(in, n); f != nil && f.IsRepeated() { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf("Field %q in method_signature %q: only the last field in a signature argument can be repeated.", name, strings.Join(sig, ",")), + Descriptor: m, + }) + } } } } diff --git a/rules/aip4232/repeated_fields_test.go b/rules/aip4232/repeated_fields_test.go index 8ee382dec..d2e12f238 100644 --- a/rules/aip4232/repeated_fields_test.go +++ b/rules/aip4232/repeated_fields_test.go @@ -27,10 +27,10 @@ func TestRepeatedFields(t *testing.T) { SecondSig string problems testutils.Problems }{ - {"Valid", "name,paperback_only,editions", "name,editions", nil}, - {"InvalidFirstSignature", "name,editions,paperback_only", "name,editions", testutils.Problems{{Message: "only the last"}}}, - {"InvalidSecondSignature", "name,editions", "name,editions,paperback_only", testutils.Problems{{Message: "only the last"}}}, - {"BothInvalid", "name,editions,paperback_only", "editions,name", testutils.Problems{{Message: "only the last"}, {Message: "only the last"}}}, + {"Valid", "name,paperback_only,book.editions", "name,editions", nil}, + {"InvalidFirstSignature", "name,book.shelves.shelf,paperback_only", "name,editions", testutils.Problems{{Message: "only the last"}}}, + {"InvalidSecondSignature", "name,book.editions", "name,book.shelves.shelf,paperback_only", testutils.Problems{{Message: "only the last"}}}, + {"BothInvalid", "name,book.shelves.shelf,paperback_only", "name,book.shelves.shelf,paperback_only", testutils.Problems{{Message: "only the last"}, {Message: "only the last"}}}, } for _, test := range tests { @@ -47,8 +47,18 @@ func TestRepeatedFields(t *testing.T) { string name = 1; bool paperback_only = 2; - - repeated int32 editions = 3; + + Book book = 3; + } + message Book { + string name = 1; + + repeated int32 editions = 2; + + repeated Shelf shelves = 3; + } + message Shelf { + string shelf = 1; } message ArchiveBookResponse {} `, test) diff --git a/rules/internal/utils/find.go b/rules/internal/utils/find.go index 054e72042..b932e3c97 100644 --- a/rules/internal/utils/find.go +++ b/rules/internal/utils/find.go @@ -134,13 +134,14 @@ func GetRepeatedMessageFields(m *desc.MessageDescriptor) []*desc.FieldDescriptor // google.api.method_signature annotations. func FindFieldDotNotation(msg *desc.MessageDescriptor, ref string) *desc.FieldDescriptor { path := strings.Split(ref, ".") - for _, seg := range path { + end := len(path) - 1 + for i, seg := range path { field := msg.FindFieldByName(seg) if field == nil { return nil } - if m := field.GetMessageType(); m != nil { + if m := field.GetMessageType(); m != nil && i != end { msg = m continue }