Repository: googleapis/api-linter
Pull request #1091 (2023-02-10 18:40:56)

feat: add the ability to ignore comment disables
For strict enforcement of AIPs across an organization, it's valuable to disable the ability to have text in the proto disable linter rules.

toumorokoshi: > Add a test in lint_test.go similar to [this](https://github.com/googleapis/api-linter/blob/3890e69e44084e1d2e2f83c3a3be54db9a2f4552/lint/lint_test.go#L156)?

This feels a little redundant to me to be honest (the integration test exercises this path), but I can add it.
toumorokoshi: @shwoodard I think I addresses all of your comments. PTAL.

Patch:
diff --git a/cmd/api-linter/cli.go b/cmd/api-linter/cli.go
index be4daf1..526a6a5 100644
--- a/cmd/api-linter/cli.go
+++ b/cmd/api-linter/cli.go
@@ -32,21 +32,23 @@ import (
 )
 
 type cli struct {
-	ConfigPath              string
-	FormatType              string
-	OutputPath              string
-	ExitStatusOnLintFailure bool
-	VersionFlag             bool
-	ProtoImportPaths        []string
-	ProtoFiles              []string
-	ProtoDescPath           []string
-	EnabledRules            []string
-	DisabledRules           []string
-	ListRulesFlag           bool
-	DebugFlag               bool
+	ConfigPath                string
+	FormatType                string
+	OutputPath                string
+	ExitStatusOnLintFailure   bool
+	VersionFlag               bool
+	ProtoImportPaths          []string
+	ProtoFiles                []string
+	ProtoDescPath             []string
+	EnabledRules              []string
+	DisabledRules             []string
+	ListRulesFlag             bool
+	DebugFlag                 bool
+	IgnoreCommentDisablesFlag bool
 }
 
 // ExitForLintFailure indicates that a problem was found during linting.
+//
 //lint:ignore ST1012 modifying this variable name is a breaking change.
 var ExitForLintFailure = errors.New("found problems during linting")
 
@@ -63,6 +65,7 @@ func newCli(args []string) *cli {
 	var ruleDisableFlag []string
 	var listRulesFlag bool
 	var debugFlag bool
+	var ignoreCommentDisablesFlag bool
 
 	// Register flag variables.
 	fs := pflag.NewFlagSet("api-linter", pflag.ExitOnError)
@@ -76,7 +79,8 @@ func newCli(args []string) *cli {
 	fs.StringArrayVar(&ruleEnableFlag, "enable-rule", nil, "Enable a rule with the given name.\nMay be specified multiple times.")
 	fs.StringArrayVar(&ruleDisableFlag, "disable-rule", nil, "Disable a rule with the given name.\nMay be specified multiple times.")
 	fs.BoolVar(&listRulesFlag, "list-rules", false, "Print the rules and exit.  Honors the output-format flag.")
-	fs.BoolVar(&debugFlag, "debug", false, "Run in deubug mode. Panics will print stack.")
+	fs.BoolVar(&debugFlag, "debug", false, "Run in debug mode. Panics will print stack.")
+	fs.BoolVar(&ignoreCommentDisablesFlag, "ignore-comment-disables", false, "If set to true, disable comments will be ignored.\nThis is helpful when strict enforcement of AIPs are necessary and\nproto definitions should not be able to disable checks.")
 
 	// Parse flags.
 	err := fs.Parse(args)
@@ -85,18 +89,19 @@ func newCli(args []string) *cli {
 	}
 
 	return &cli{
-		ConfigPath:              cfgFlag,
-		FormatType:              fmtFlag,
-		OutputPath:              outFlag,
-		ExitStatusOnLintFailure: setExitStatusOnLintFailure,
-		ProtoImportPaths:        append(protoImportFlag, "."),
-		ProtoDescPath:           protoDescFlag,
-		EnabledRules:            ruleEnableFlag,
-		DisabledRules:           ruleDisableFlag,
-		ProtoFiles:              fs.Args(),
-		VersionFlag:             versionFlag,
-		ListRulesFlag:           listRulesFlag,
-		DebugFlag:               debugFlag,
+		ConfigPath:                cfgFlag,
+		FormatType:                fmtFlag,
+		OutputPath:                outFlag,
+		ExitStatusOnLintFailure:   setExitStatusOnLintFailure,
+		ProtoImportPaths:          append(protoImportFlag, "."),
+		ProtoDescPath:             protoDescFlag,
+		EnabledRules:              ruleEnableFlag,
+		DisabledRules:             ruleDisableFlag,
+		ProtoFiles:                fs.Args(),
+		VersionFlag:               versionFlag,
+		ListRulesFlag:             listRulesFlag,
+		DebugFlag:                 debugFlag,
+		IgnoreCommentDisablesFlag: ignoreCommentDisablesFlag,
 	}
 }
 
@@ -180,7 +185,7 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 	}
 
 	// Create a linter to lint the file descriptors.
-	l := lint.New(rules, configs, lint.Debug(c.DebugFlag))
+	l := lint.New(rules, configs, lint.Debug(c.DebugFlag), lint.IgnoreCommentDisables(c.IgnoreCommentDisablesFlag))
 	results, err := l.LintProtos(fd...)
 	if err != nil {
 		return err
diff --git a/lint/lint.go b/lint/lint.go
index 9d5aba6..61e958a 100644
--- a/lint/lint.go
+++ b/lint/lint.go
@@ -27,9 +27,10 @@ import (
 
 // Linter checks API files and returns a list of detected problems.
 type Linter struct {
-	rules   RuleRegistry
-	configs Configs
-	debug   bool
+	rules                 RuleRegistry
+	configs               Configs
+	debug                 bool
+	ignoreCommentDisables bool
 }
 
 // LinterOption prvoides the ability to configure the Linter.
@@ -42,6 +43,13 @@ func Debug(debug bool) LinterOption {
 	}
 }
 
+// IgnoreCommentDisables sets the flag for ignoring comments which disable rules.
+func IgnoreCommentDisables(ignoreCommentDisables bool) LinterOption {
+	return func(l *Linter) {
+		l.ignoreCommentDisables = ignoreCommentDisables
+	}
+}
+
 // New creates and returns a linter with the given rules and configs.
 func New(rules RuleRegistry, configs Configs, opts ...LinterOption) *Linter {
 	l := &Linter{
@@ -91,7 +99,7 @@ func (l *Linter) lintFileDescriptor(fd *desc.FileDescriptor) (Response, error) {
 						errMessages = append(errMessages, fmt.Sprintf("rule %q missing required Descriptor in returned Problem", rule.GetName()))
 						continue
 					}
-					if ruleIsEnabled(rule, p.Descriptor, p.Location, aliasMap) {
+					if ruleIsEnabled(rule, p.Descriptor, p.Location, aliasMap, l.ignoreCommentDisables) {
 						p.RuleID = rule.GetName()
 						resp.Problems = append(resp.Problems, p)
 					}
diff --git a/lint/rule_enabled.go b/lint/rule_enabled.go
index a21baf0..9f8f85c 100644
--- a/lint/rule_enabled.go
+++ b/lint/rule_enabled.go
@@ -65,7 +65,8 @@ var descriptorDisableChecks = []func(d desc.Descriptor) bool{
 //
 // Note, if the given source code location is not nil, it will be used to
 // augment the set of commentLines.
-func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Location, aliasMap map[string]string) bool {
+func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Location,
+	aliasMap map[string]string, ignoreCommentDisables bool) bool {
 	// If the rule is disabled because of something on the descriptor itself
 	// (e.g. a deprecated annotation), address that.
 	for _, mustDisable := range descriptorDisableChecks {
@@ -76,6 +77,27 @@ func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Loca
 		}
 	}
 
+	if !ignoreCommentDisables {
+		if ruleIsDisabledByComments(rule, d, l, aliasMap) {
+			return false
+		}
+	}
+
+	// The rule may have been disabled on a parent. (For example, a field rule
+	// may be disabled at the message level to cover all fields in the message).
+	//
+	// Do not pass the source code location here, the source location in relation
+	// to the parent is not helpful.
+	if parent := d.GetParent(); parent != nil {
+		return ruleIsEnabled(rule, parent, nil, aliasMap, ignoreCommentDisables)
+	}
+
+	return true
+}
+
+// ruleIsDisabledByComments returns true if the rule has been disabled
+// by comments in the file or leading the element.
+func ruleIsDisabledByComments(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Location, aliasMap map[string]string) bool {
 	// Some rules have a legacy name. We add it to the check list.
 	ruleName := string(rule.GetName())
 	names := []string{ruleName, aliasMap[ruleName]}
@@ -99,18 +121,9 @@ func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Loca
 
 	for _, name := range names {
 		if matchRule(name, disabledRules...) {
-			return false
+			return true
 		}
 	}
 
-	// The rule may have been disabled on a parent. (For example, a field rule
-	// may be disabled at the message level to cover all fields in the message).
-	//
-	// Do not pass the source code location here, the source location in relation
-	// to the parent is not helpful.
-	if parent := d.GetParent(); parent != nil {
-		return ruleIsEnabled(rule, parent, nil, aliasMap)
-	}
-
-	return true
+	return false
 }


Test patch:
diff --git a/cmd/api-linter/integration_test.go b/cmd/api-linter/integration_test.go
index ae737a0..14e936b 100644
--- a/cmd/api-linter/integration_test.go
+++ b/cmd/api-linter/integration_test.go
@@ -77,7 +77,7 @@ func TestRules_EnabledByDefault(t *testing.T) {
 			proto := test.proto
 			result := runLinter(t, proto, "")
 			if !strings.Contains(result, test.rule) {
-				t.Errorf("Rule %q should be enabled by default", test.rule)
+				t.Errorf("rule %q should be enabled by default", test.rule)
 			}
 		})
 	}
@@ -90,22 +90,47 @@ func TestRules_DisabledByFileComments(t *testing.T) {
 			proto := disableInFile + "\n" + test.proto
 			result := runLinter(t, proto, "")
 			if strings.Contains(result, test.rule) {
-				t.Errorf("Rule %q should be disabled by file comments", test.rule)
+				t.Errorf("rule %q should be disabled by file comments", test.rule)
 			}
 		})
 	}
 }
 
 func TestRules_DisabledByInlineComments(t *testing.T) {
+	testConfigurations := []struct {
+		suffix       string
+		appendArgs   []string
+		wantDisabled bool
+	}{
+		{
+			suffix:       "WithIgnoreCommentDisablesTrue",
+			appendArgs:   []string{"--ignore-comment-disables=true"},
+			wantDisabled: false,
+		},
+		{
+			suffix:       "WithIgnoreCommentDisablesFalse",
+			appendArgs:   []string{"--ignore-comment-disables=false"},
+			wantDisabled: true,
+		},
+		{
+			suffix:       "WithNoFlags",
+			appendArgs:   []string{},
+			wantDisabled: true,
+		},
+	}
+
 	for _, test := range testCases {
-		t.Run(test.testName, func(t *testing.T) {
-			disableInline := fmt.Sprintf("(-- api-linter: %s=disabled --)", test.rule)
-			proto := strings.Replace(test.proto, "disable-me-here", disableInline, -1)
-			result := runLinter(t, proto, "")
-			if strings.Contains(result, test.rule) {
-				t.Errorf("Rule %q should be disabled by in-line comments", test.rule)
-			}
-		})
+		for _, testConfig := range testConfigurations {
+			t.Run(test.testName+testConfig.suffix, func(t *testing.T) {
+				disableInline := fmt.Sprintf("(-- api-linter: %s=disabled --)", test.rule)
+				proto := strings.Replace(test.proto, "disable-me-here", disableInline, -1)
+				_, result := runLinterWithFailureStatus(t, proto, "", testConfig.appendArgs)
+				isDisabled := !strings.Contains(result, test.rule)
+				if isDisabled != testConfig.wantDisabled {
+					t.Errorf("want %q disabled by in-line comments to be %v with flags %v, got %v", test.rule, testConfig.wantDisabled, testConfig.appendArgs, isDisabled)
+				}
+			})
+		}
 	}
 }
 
@@ -123,7 +148,7 @@ func TestRules_DisabledByConfig(t *testing.T) {
 			c := strings.Replace(config, "replace-me-here", test.rule, -1)
 			result := runLinter(t, test.proto, c)
 			if strings.Contains(result, test.rule) {
-				t.Errorf("Rule %q should be disabled by the user config: %q", test.rule, c)
+				t.Errorf("rule %q should be disabled by the user config: %q", test.rule, c)
 			}
 		})
 	}
diff --git a/lint/lint_test.go b/lint/lint_test.go
index dd66ccf..114cf55 100644
--- a/lint/lint_test.go
+++ b/lint/lint_test.go
@@ -178,3 +178,29 @@ func TestLinter_debug(t *testing.T) {
 		})
 	}
 }
+
+func TestLinter_IgnoreCommentDisables(t *testing.T) {
+	tests := []struct {
+		name                  string
+		ignoreCommentDisables bool
+	}{
+		{
+			name:                  "ignoreCommentDisables",
+			ignoreCommentDisables: true,
+		},
+		{
+			name:                  "do not ignoreCommentDisables",
+			ignoreCommentDisables: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			l := New(NewRuleRegistry(), nil, IgnoreCommentDisables(test.ignoreCommentDisables))
+
+			if a, e := l.ignoreCommentDisables, test.ignoreCommentDisables; a != e {
+				t.Errorf("got ignoreCommentDisables %v wanted ignoreCommentDisables %v", a, e)
+			}
+		})
+	}
+}
diff --git a/lint/rule_enabled_test.go b/lint/rule_enabled_test.go
index 3c23d78..2355bc2 100644
--- a/lint/rule_enabled_test.go
+++ b/lint/rule_enabled_test.go
@@ -71,9 +71,14 @@ func TestRuleIsEnabled(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Error building test message")
 			}
-			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, aliases), test.enabled; got != want {
+			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, aliases, false), test.enabled; got != want {
 				t.Errorf("Expected the test rule to return %v from ruleIsEnabled, got %v", want, got)
 			}
+			if !test.enabled {
+				if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, aliases, true), true; got != want {
+					t.Errorf("Expected the test rule with ignoreCommentDisables true to return %v from ruleIsEnabled, got %v", want, got)
+				}
+			}
 		})
 	}
 }
@@ -98,10 +103,10 @@ func TestRuleIsEnabledFirstMessage(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error building test file: %q", err)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, nil), false; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, nil, false), false; got != want {
 		t.Errorf("Expected the first message to return %v from ruleIsEnabled, got %v", want, got)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1], nil, nil), true; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1], nil, nil, false), true; got != want {
 		t.Errorf("Expected the second message to return %v from ruleIsEnabled, got %v", want, got)
 	}
 }
@@ -126,10 +131,10 @@ func TestRuleIsEnabledParent(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error building test file: %q", err)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil), false; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil, false), false; got != want {
 		t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1].GetFields()[0], nil, nil), true; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1].GetFields()[0], nil, nil, false), true; got != want {
 		t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 	}
 }
@@ -166,7 +171,7 @@ func TestRuleIsEnabledDeprecated(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Error building test file: %q", err)
 			}
-			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil), test.enabled; got != want {
+			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil, false), test.enabled; got != want {
 				t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 			}
 		})