Repository: googleapis/api-linter
Pull request #190 (2019-09-24 14:08:16)

[feat] Add OnlyIf to rules.
There is a really common pattern which looks like...

```go
// Sanity check: Is this a descriptor that this actually
// cares about.
if !relevantMessage(message) {
  return nil
}
```

This formalizes that into an (optional) `OnlyIf` function that
restricts a rule to a scope.

mingzhi: Nitpicking: `Predicate` seems to be a more common name.
lukesneeringer: I considered `Predicate`, but I think a lot of people do not know what that means. I think `OnlyIf` will be clearer for more people.

Patch:
diff --git a/lint/rule.go b/lint/rule.go
index 9702bfc..6feaefa 100644
--- a/lint/rule.go
+++ b/lint/rule.go
@@ -74,6 +74,10 @@ type MessageRule struct {
 	// of Problems it finds.
 	LintMessage func(*desc.MessageDescriptor) []Problem
 
+	// OnlyIf accepts a MessageDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.MessageDescriptor) bool
+
 	noPositional struct{}
 }
 
@@ -94,7 +98,9 @@ func (r *MessageRule) Lint(fd *desc.FileDescriptor) []Problem {
 
 	// Iterate over each message and process rules for each message.
 	for _, message := range getAllMessages(fd) {
-		problems = append(problems, r.LintMessage(message)...)
+		if r.OnlyIf == nil || r.OnlyIf(message) {
+			problems = append(problems, r.LintMessage(message)...)
+		}
 	}
 	return problems
 }
@@ -108,6 +114,10 @@ type FieldRule struct {
 	// Problems it finds.
 	LintField func(*desc.FieldDescriptor) []Problem
 
+	// OnlyIf accepts a FieldDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.FieldDescriptor) bool
+
 	noPositional struct{}
 }
 
@@ -129,7 +139,9 @@ func (r *FieldRule) Lint(fd *desc.FileDescriptor) []Problem {
 	// message.
 	for _, message := range getAllMessages(fd) {
 		for _, field := range message.GetFields() {
-			problems = append(problems, r.LintField(field)...)
+			if r.OnlyIf == nil || r.OnlyIf(field) {
+				problems = append(problems, r.LintField(field)...)
+			}
 		}
 	}
 	return problems
@@ -143,6 +155,10 @@ type ServiceRule struct {
 	// LintService accepts a ServiceDescriptor and lints it.
 	LintService func(*desc.ServiceDescriptor) []Problem
 
+	// OnlyIf accepts a ServiceDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.ServiceDescriptor) bool
+
 	noPositional struct{}
 }
 
@@ -160,7 +176,9 @@ func (r *ServiceRule) GetURI() string {
 func (r *ServiceRule) Lint(fd *desc.FileDescriptor) []Problem {
 	problems := []Problem{}
 	for _, service := range fd.GetServices() {
-		problems = append(problems, r.LintService(service)...)
+		if r.OnlyIf == nil || r.OnlyIf(service) {
+			problems = append(problems, r.LintService(service)...)
+		}
 	}
 	return problems
 }
@@ -173,6 +191,10 @@ type MethodRule struct {
 	// LintMethod accepts a MethodDescriptor and lints it.
 	LintMethod func(*desc.MethodDescriptor) []Problem
 
+	// OnlyIf accepts a MethodDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.MethodDescriptor) bool
+
 	noPositional struct{}
 }
 
@@ -191,7 +213,9 @@ func (r *MethodRule) Lint(fd *desc.FileDescriptor) []Problem {
 	problems := []Problem{}
 	for _, service := range fd.GetServices() {
 		for _, method := range service.GetMethods() {
-			problems = append(problems, r.LintMethod(method)...)
+			if r.OnlyIf == nil || r.OnlyIf(method) {
+				problems = append(problems, r.LintMethod(method)...)
+			}
 		}
 	}
 	return problems
@@ -205,6 +229,10 @@ type EnumRule struct {
 	// LintEnum accepts a EnumDescriptor and lints it.
 	LintEnum func(*desc.EnumDescriptor) []Problem
 
+	// OnlyIf accepts an EnumDescriptor and determines whether this rule
+	// is applicable.
+	OnlyIf func(*desc.EnumDescriptor) bool
+
 	noPositional struct{}
 }
 
@@ -225,13 +253,17 @@ func (r *EnumRule) Lint(fd *desc.FileDescriptor) []Problem {
 
 	// Lint enums that are at the top level of the file.
 	for _, enum := range fd.GetEnumTypes() {
-		problems = append(problems, r.LintEnum(enum)...)
+		if r.OnlyIf == nil || r.OnlyIf(enum) {
+			problems = append(problems, r.LintEnum(enum)...)
+		}
 	}
 
 	// Lint enums that are nested within messages.
 	for _, message := range getAllMessages(fd) {
 		for _, enum := range message.GetNestedEnumTypes() {
-			problems = append(problems, r.LintEnum(enum)...)
+			if r.OnlyIf == nil || r.OnlyIf(enum) {
+				problems = append(problems, r.LintEnum(enum)...)
+			}
 		}
 	}
 	return problems
diff --git a/rules/aip0131/message_rules.go b/rules/aip0131/message_rules.go
index f420823..678e242 100644
--- a/rules/aip0131/message_rules.go
+++ b/rules/aip0131/message_rules.go
@@ -24,15 +24,10 @@ import (
 
 // The Get standard method should only have expected fields.
 var standardFields = &lint.MessageRule{
-	Name: lint.NewRuleName("core", "0131", "request-message", "name-field"),
-	URI:  "https://aip.dev/131#request-message",
+	Name:   lint.NewRuleName("core", "0131", "request-message", "name-field"),
+	URI:    "https://aip.dev/131#request-message",
+	OnlyIf: isGetRequestMessage,
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetRequestMessage(m) {
-			return nil
-		}
-
 		// Rule check: Establish that a name field is present.
 		name := m.FindFieldByName("name")
 		if name == nil {
@@ -56,15 +51,10 @@ var standardFields = &lint.MessageRule{
 
 // Get methods should not have unrecognized fields.
 var unknownFields = &lint.MessageRule{
-	Name: lint.NewRuleName("core", "0131", "request-message", "unknown-fields"),
-	URI:  "https://aip.dev/131#request-message",
+	Name:   lint.NewRuleName("core", "0131", "request-message", "unknown-fields"),
+	URI:    "https://aip.dev/131#request-message",
+	OnlyIf: isGetRequestMessage,
 	LintMessage: func(m *desc.MessageDescriptor) (problems []lint.Problem) {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetRequestMessage(m) {
-			return
-		}
-
 		// Rule check: Establish that there are no unexpected fields.
 		allowedFields := map[string]struct{}{
 			"name":      {}, // AIP-131
diff --git a/rules/aip0131/method_rules.go b/rules/aip0131/method_rules.go
index 84bb1ec..53b2c30 100644
--- a/rules/aip0131/method_rules.go
+++ b/rules/aip0131/method_rules.go
@@ -24,15 +24,10 @@ import (
 
 // Get messages should have a properly named Request message.
 var requestMessageName = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0131", "request-message", "name"),
-	URI:  "https://aip.dev/131#guidance",
+	Name:   lint.NewRuleName("core", "0131", "request-message", "name"),
+	URI:    "https://aip.dev/131#guidance",
+	OnlyIf: isGetMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetMethod(m) {
-			return nil
-		}
-
 		// Rule check: Establish that for methods such as `GetFoo`, the request
 		// message is named `GetFooRequest`.
 		if got, want := m.GetInputType().GetName(), m.GetName()+"Request"; got != want {
@@ -52,15 +47,10 @@ var requestMessageName = &lint.MethodRule{
 
 // Get messages should use the resource as the response message
 var responseMessageName = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0131", "response-message", "name"),
-	URI:  "https://aip.dev/131#guidance",
+	Name:   lint.NewRuleName("core", "0131", "response-message", "name"),
+	URI:    "https://aip.dev/131#guidance",
+	OnlyIf: isGetMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetMethod(m) {
-			return nil
-		}
-
 		// Rule check: Establish that for methods such as `GetFoo`, the response
 		// message is named `Foo`.
 		if got, want := m.GetOutputType().GetName(), m.GetName()[3:]; got != want {
@@ -80,15 +70,10 @@ var responseMessageName = &lint.MethodRule{
 
 // Get methods should use the HTTP GET verb.
 var httpVerb = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0131", "http-verb"),
-	URI:  "https://aip.dev/131#guidance",
+	Name:   lint.NewRuleName("core", "0131", "http-verb"),
+	URI:    "https://aip.dev/131#guidance",
+	OnlyIf: isGetMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetMethod(m) {
-			return nil
-		}
-
 		// Rule check: Establish that the RPC uses HTTP GET.
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if httpRule.GetGet() == "" {
@@ -105,15 +90,10 @@ var httpVerb = &lint.MethodRule{
 
 // Get methods should have a proper HTTP pattern.
 var httpNameField = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0131", "http-body"),
-	URI:  "https://aip.dev/131#guidance",
+	Name:   lint.NewRuleName("core", "0131", "http-body"),
+	URI:    "https://aip.dev/131#guidance",
+	OnlyIf: isGetMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetMethod(m) {
-			return nil
-		}
-
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if uri := httpRule.GetGet(); uri != "" {
@@ -132,15 +112,10 @@ var httpNameField = &lint.MethodRule{
 
 // Get methods should not have an HTTP body.
 var httpBody = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0131", "http-name"),
-	URI:  "https://aip.dev/131#guidance",
+	Name:   lint.NewRuleName("core", "0131", "http-name"),
+	URI:    "https://aip.dev/131#guidance",
+	OnlyIf: isGetMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Get methods for the purpose of this rule;
-		// ignore everything else.
-		if !isGetMethod(m) {
-			return nil
-		}
-
 		// Establish that the RPC has no HTTP body.
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if httpRule.GetBody() != "" {
diff --git a/rules/aip0132/message_rules.go b/rules/aip0132/message_rules.go
index 81c2c65..150f13f 100644
--- a/rules/aip0132/message_rules.go
+++ b/rules/aip0132/message_rules.go
@@ -24,15 +24,10 @@ import (
 
 // The List standard method should contain a parent field.
 var standardFields = &lint.MessageRule{
-	Name: lint.NewRuleName("core", "0132", "request-message", "parent-field"),
-	URI:  "https://aip.dev/132#request-message",
+	Name:   lint.NewRuleName("core", "0132", "request-message", "parent-field"),
+	URI:    "https://aip.dev/132#request-message",
+	OnlyIf: isListRequestMessage,
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		// We only care about List- methods for the purpose of this rule;
-		// ignore everything else.
-		if !isListRequestMessage(m) {
-			return nil
-		}
-
 		// Rule check: Establish that a `parent` field is present.
 		parentField := m.FindFieldByName("parent")
 		if parentField == nil {
@@ -56,15 +51,10 @@ var standardFields = &lint.MessageRule{
 
 // List methods should not have unrecognized fields.
 var unknownFields = &lint.MessageRule{
-	Name: lint.NewRuleName("core", "0132", "request-message", "unknown-fields"),
-	URI:  "https://aip.dev/132#request-message",
+	Name:   lint.NewRuleName("core", "0132", "request-message", "unknown-fields"),
+	URI:    "https://aip.dev/132#request-message",
+	OnlyIf: isListRequestMessage,
 	LintMessage: func(m *desc.MessageDescriptor) (problems []lint.Problem) {
-		// We only care about List- methods for the purpose of this rule;
-		// ignore everything else.
-		if !isListRequestMessage(m) {
-			return
-		}
-
 		// Rule check: Establish that there are no unexpected fields.
 		//
 		// Additionally, we type check only the fields defined in AIP-132,
diff --git a/rules/aip0132/method_rules.go b/rules/aip0132/method_rules.go
index 1cce132..95d9e0c 100644
--- a/rules/aip0132/method_rules.go
+++ b/rules/aip0132/method_rules.go
@@ -23,15 +23,10 @@ import (
 
 // List messages should have a properly named Request message.
 var requestMessageName = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0132", "request-message", "name"),
-	URI:  "https://aip.dev/132#guidance",
+	Name:   lint.NewRuleName("core", "0132", "request-message", "name"),
+	URI:    "https://aip.dev/132#guidance",
+	OnlyIf: isListMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about List- methods for the purpose of this rule;
-		// ignore everything else.
-		if !isListMethod(m) {
-			return nil
-		}
-
 		// Rule check: Establish that for methods such as `ListFoos`, the request
 		// message is named `ListFoosRequest`.
 		if got, want := m.GetInputType().GetName(), m.GetName()+"Request"; got != want {
@@ -51,15 +46,10 @@ var requestMessageName = &lint.MethodRule{
 
 // List messages should use a `ListFoosResponse` response message.
 var responseMessageName = &lint.MethodRule{
-	Name: lint.NewRuleName("core", "0132", "response-message", "name"),
-	URI:  "https://aip.dev/132#guidance",
+	Name:   lint.NewRuleName("core", "0132", "response-message", "name"),
+	URI:    "https://aip.dev/132#guidance",
+	OnlyIf: isListMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about List- methods for the purpose of this rule;
-		// ignore everything else.
-		if !isListMethod(m) {
-			return nil
-		}
-
 		// Rule check: Establish that for methods such as `ListFoos`, the response
 		// message is named `ListFoosResponse`.
 		if got, want := m.GetOutputType().GetName(), m.GetName()+"Response"; got != want {


Test patch:
diff --git a/lint/rule_test.go b/lint/rule_test.go
index 38c2adb..a33719b 100644
--- a/lint/rule_test.go
+++ b/lint/rule_test.go
@@ -67,11 +67,11 @@ func TestMessageRule(t *testing.T) {
 			rule := &MessageRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  uri,
+				OnlyIf: func(m *desc.MessageDescriptor) bool {
+					return m.GetName() == "Author"
+				},
 				LintMessage: func(m *desc.MessageDescriptor) []Problem {
-					if m.GetName() == "Author" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
@@ -99,11 +99,11 @@ func TestMessageRuleNested(t *testing.T) {
 			rule := &MessageRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  uri,
+				OnlyIf: func(m *desc.MessageDescriptor) bool {
+					return m.GetName() == "Author"
+				},
 				LintMessage: func(m *desc.MessageDescriptor) []Problem {
-					if m.GetName() == "Author" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
@@ -134,11 +134,11 @@ func TestFieldRule(t *testing.T) {
 			rule := &FieldRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  uri,
+				OnlyIf: func(f *desc.FieldDescriptor) bool {
+					return f.GetName() == "edition_count"
+				},
 				LintField: func(f *desc.FieldDescriptor) []Problem {
-					if f.GetName() == "edition_count" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
@@ -204,11 +204,11 @@ func TestMethodRule(t *testing.T) {
 			rule := &MethodRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  fmt.Sprintf("https://aip.dev/%s", t.Name()),
+				OnlyIf: func(m *desc.MethodDescriptor) bool {
+					return m.GetName() == "CreateBook"
+				},
 				LintMethod: func(m *desc.MethodDescriptor) []Problem {
-					if m.GetName() == "CreateBook" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
@@ -235,11 +235,11 @@ func TestEnumRule(t *testing.T) {
 			rule := &EnumRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  fmt.Sprintf("https://aip.dev/%s", t.Name()),
+				OnlyIf: func(e *desc.EnumDescriptor) bool {
+					return e.GetName() == "Edition"
+				},
 				LintEnum: func(e *desc.EnumDescriptor) []Problem {
-					if e.GetName() == "Edition" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
@@ -268,11 +268,11 @@ func TestEnumRuleNested(t *testing.T) {
 			rule := &EnumRule{
 				Name: NewRuleName("test", test.testName),
 				URI:  fmt.Sprintf("https://aip.dev/%s", t.Name()),
+				OnlyIf: func(e *desc.EnumDescriptor) bool {
+					return e.GetName() == "Edition"
+				},
 				LintEnum: func(e *desc.EnumDescriptor) []Problem {
-					if e.GetName() == "Edition" {
-						return test.problems
-					}
-					return nil
+					return test.problems
 				},
 			}
 
diff --git a/rules/aip0131/message_rules_test.go b/rules/aip0131/message_rules_test.go
index b87b8a4..e178c01 100644
--- a/rules/aip0131/message_rules_test.go
+++ b/rules/aip0131/message_rules_test.go
@@ -51,7 +51,7 @@ func TestStandardFields(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := standardFields.LintMessage(message); len(problems) != test.problemCount {
+			if problems := standardFields.Lint(message.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, standardFields.Name, problems)
 			}
 		})
@@ -95,7 +95,7 @@ func TestUnknownFields(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := unknownFields.LintMessage(message); len(problems) != test.problemCount {
+			if problems := unknownFields.Lint(message.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, unknownFields.Name, problems)
 			}
 		})
diff --git a/rules/aip0131/method_rules_test.go b/rules/aip0131/method_rules_test.go
index c5eb095..a79904c 100644
--- a/rules/aip0131/method_rules_test.go
+++ b/rules/aip0131/method_rules_test.go
@@ -54,7 +54,7 @@ func TestRequestMessageName(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := requestMessageName.LintMethod(service.GetMethods()[0]); len(problems) != test.problemCount {
+			if problems := requestMessageName.Lint(service.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, requestMessageName.Name, problems)
 			}
 		})
@@ -90,7 +90,7 @@ func TestResponseMessageName(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := responseMessageName.LintMethod(service.GetMethods()[0]); len(problems) != test.problemCount {
+			if problems := responseMessageName.Lint(service.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, responseMessageName.Name, problems)
 			}
 		})
diff --git a/rules/aip0132/message_rules_test.go b/rules/aip0132/message_rules_test.go
index 02e8e12..a9d0fb0 100644
--- a/rules/aip0132/message_rules_test.go
+++ b/rules/aip0132/message_rules_test.go
@@ -51,7 +51,7 @@ func TestStandardFields(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := standardFields.LintMessage(message); len(problems) != test.problemCount {
+			if problems := standardFields.Lint(message.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, standardFields.Name, problems)
 			}
 		})
@@ -104,7 +104,7 @@ func TestUnknownFields(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := unknownFields.LintMessage(message); len(problems) != test.problemCount {
+			if problems := unknownFields.Lint(message.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, unknownFields.Name, problems)
 			}
 		})
diff --git a/rules/aip0132/method_rules_test.go b/rules/aip0132/method_rules_test.go
index e7e21e0..a8563ae 100644
--- a/rules/aip0132/method_rules_test.go
+++ b/rules/aip0132/method_rules_test.go
@@ -49,7 +49,7 @@ func TestRequestMessageName(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := requestMessageName.LintMethod(service.GetMethods()[0]); len(problems) != test.problemCount {
+			if problems := requestMessageName.Lint(service.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, requestMessageName.Name, problems)
 			}
 		})
@@ -85,7 +85,7 @@ func TestResponseMessageName(t *testing.T) {
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			if problems := responseMessageName.LintMethod(service.GetMethods()[0]); len(problems) != test.problemCount {
+			if problems := responseMessageName.Lint(service.GetFile()); len(problems) != test.problemCount {
 				t.Errorf("%s on rule %s: %#v", test.errPrefix, responseMessageName.Name, problems)
 			}
 		})