Repository: googleapis/api-linter
Pull request #822 (2021-04-01 21:51:04)

fix: Suppress core::aip0158::request-page-token-field for non-request messages
Based on an internal bug report: If a message isn't actually a request but happens to have a `page_size` field, it shouldn't be required to have a `page_token` field. An example of this is a message that itself will be serialized as a page token, e.g.

```proto
message FooPageToken {
  int32 page_size = 1;
  int32 page_num = 2;
}
```

This seems to have been an oversight in #496.

It is debatable whether we should apply this to the corresponding `isPaginatedResponseMessage` function, though it seems less likely to come up.

lukesneeringer: Oh, huh. Yeah, that is an interesting bug. Shocked it took two years for anyone to run into that.

Patch:
diff --git a/rules/aip0158/aip0158.go b/rules/aip0158/aip0158.go
index 060b61b..fef53c0 100644
--- a/rules/aip0158/aip0158.go
+++ b/rules/aip0158/aip0158.go
@@ -17,6 +17,7 @@ package aip0158
 
 import (
 	"regexp"
+	"strings"
 
 	"github.com/googleapis/api-linter/lint"
 	"github.com/jhump/protoreflect/desc"
@@ -40,7 +41,14 @@ var paginatedRes = regexp.MustCompile("^(List|Search)[A-Za-z0-9]*Response$")
 
 // Return true if this is an AIP-158 List request message, false otherwise.
 func isPaginatedRequestMessage(m *desc.MessageDescriptor) bool {
-	return paginatedReq.MatchString(m.GetName()) || m.FindFieldByName("page_size") != nil || m.FindFieldByName("page_token") != nil
+	if paginatedReq.MatchString(m.GetName()) {
+		return true
+	}
+	// Ignore messages that happen to have these fields but are not requests.
+	if !strings.HasSuffix(m.GetName(), "Request") {
+		return false
+	}
+	return m.FindFieldByName("page_size") != nil || m.FindFieldByName("page_token") != nil
 }
 
 // Return true if this is an AIP-158 List response message, false otherwise.


Test patch:
diff --git a/rules/aip0158/request_page_size_field_test.go b/rules/aip0158/request_page_size_field_test.go
index 03c24e7..fee57da 100644
--- a/rules/aip0158/request_page_size_field_test.go
+++ b/rules/aip0158/request_page_size_field_test.go
@@ -53,6 +53,13 @@ func TestRequestPaginationPageSize(t *testing.T) {
 				return m.FindFieldByName("page_size")
 			},
 		},
+		{
+			"IrrelevantMessage",
+			"ListFooPageToken",
+			[]field{{"page_token", builder.FieldTypeString()}},
+			nil,
+			nil,
+		},
 	}
 
 	// Run each test individually.
@@ -81,7 +88,7 @@ func TestRequestPaginationPageSize(t *testing.T) {
 			// Run the lint rule, and establish that it returns the correct problems.
 			problems := requestPaginationPageSize.Lint(message.GetFile())
 			if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" {
-				t.Errorf(diff)
+				t.Error(diff)
 			}
 		})
 	}
diff --git a/rules/aip0158/request_page_token_field_test.go b/rules/aip0158/request_page_token_field_test.go
index 86277f5..8385f3a 100644
--- a/rules/aip0158/request_page_token_field_test.go
+++ b/rules/aip0158/request_page_token_field_test.go
@@ -54,6 +54,13 @@ func TestRequestPaginationPageToken(t *testing.T) {
 				return m.FindFieldByName("page_token")
 			},
 		},
+		{
+			"IrrelevantMessage",
+			"ListFooPageToken",
+			[]field{{"page_size", builder.FieldTypeInt32()}},
+			nil,
+			nil,
+		},
 	}
 
 	// Run each test individually.
@@ -82,7 +89,7 @@ func TestRequestPaginationPageToken(t *testing.T) {
 			// Run the lint rule, and establish that it returns the correct problems.
 			problems := requestPaginationPageToken.Lint(message.GetFile())
 			if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" {
-				t.Errorf(diff)
+				t.Error(diff)
 			}
 		})
 	}