fix(AIP-4232): correct repeated field check (googleapis#1337)

aep-dev · Feb 16, 2024 · b383639 · b383639
1 parent d617cc1
commit b383639
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
  }