Repository: googleapis/api-linter
Pull request #1282 (2023-11-01 11:28:36)

fix(AIP-203): avoid panic if name field is missing
This proto reproduces the following crash:
```proto
syntax = "proto3";

package dummy;

import "google/api/field_behavior.proto";
import "google/api/resource.proto";

message Dummy {
  option (google.api.resource) = {
    type: "dummy/Dummy"
    pattern: "dummies/{dummy}"
  };
  string other = 1 [(google.api.field_behavior) = REQUIRED];
}
```

```sh
goroutine 1 [running]:
runtime/debug.Stack()
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/debug/stack.go:24 +0x64
runtime/debug.PrintStack()
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/debug/stack.go:16 +0x1c
github.com/googleapis/api-linter/lint.(*Linter).runAndRecoverFromPanics.func1()
        /Users/william/dev/oss/api-linter/lint/lint.go:125 +0x50
panic({0x102971740?, 0x102e6e5f0?})
        /opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/panic.go:914 +0x218
github.com/jhump/protoreflect/desc.(*FieldDescriptor).GetFieldOptions(...)
        /Users/william/go/pkg/mod/github.com/jhump/protoreflect@v1.15.3/desc/descriptor.go:954
github.com/googleapis/api-linter/rules/internal/utils.GetFieldBehavior(0x14000331180?)
        /Users/william/dev/oss/api-linter/rules/internal/utils/extension.go:30 +0x1c
github.com/googleapis/api-linter/rules/aip0203.glob..func4(0x1400046e700?)
        /Users/william/dev/oss/api-linter/rules/aip0203/resource_name_identifier.go:30 +0x64
github.com/googleapis/api-linter/lint.(*MessageRule).Lint(0x102e779c0, 0x0?)
        /Users/william/dev/oss/api-linter/lint/rule.go:104 +0xc0
github.com/googleapis/api-linter/lint.(*Linter).runAndRecoverFromPanics(0x140000de840?, {0x102a2edb8?, 0x102e779c0?}, 0x14000045c80?)
        /Users/william/dev/oss/api-linter/lint/lint.go:135 +0x74
github.com/googleapis/api-linter/lint.(*Linter).lintFileDescriptor(0x1400021e510, 0x1400046e700)
        /Users/william/dev/oss/api-linter/lint/lint.go:96 +0x174
github.com/googleapis/api-linter/lint.(*Linter).LintProtos(0x140002d4220?, {0x140004968b8, 0x1, 0x140002d8100?})
        /Users/william/dev/oss/api-linter/lint/lint.go:71 +0x98
main.(*cli).lint(0x140000de780, 0x140002bcf60, {0x102ec1820?, 0x1022f2788?, 0x140000686e8?})
        /Users/william/dev/oss/api-linter/cmd/api-linter/cli.go:190 +0x73c
main.runCLI({0x14000032100?, 0x0?, 0x140001e9f28?})
        /Users/william/dev/oss/api-linter/cmd/api-linter/main.go:46 +0x44
main.main()
        /Users/william/dev/oss/api-linter/cmd/api-linter/main.go:39 +0x54
2023/11/01 11:34:08 runtime error: invalid memory address or nil pointer dereference
```

With the fix, the lint rule will skip checking when there is no `name` field - this is covered by [`AIP-123`](https://github.com/googleapis/api-linter/blob/main/rules/aip0123/resource_name_field.go).

radhus: > Thanks @radhus this is a great catch. I think we need to adjust the solution. Can we instead just skip the rule if there the resource name field does not exist? Meaning, put this check into the `OnlyIf`:
> 
> ```go
> OnlyIf: func(m *desc.MessageDescriptor) bool {
>   return utils.IsResource(m) && m.FindFieldByName(utils.GetResourceNameField(utils.GetResource(m))) != nil
> }
> ```
> 
> Could even put that name field existence check into a helper if you'd like.
> 
> The core functionality of this rule is **not** to enforce that the resource name field must exist - that is/should be a different rule. This rule must only lint the one thing it is meant to lint for. This makes the fine grained configuration more accurate.

@noahdietz you're right, this is even covered by [AIP-123](https://github.com/googleapis/api-linter/blob/main/rules/aip0123/resource_name_field.go) already. Took your suggestion, much cleaner, and I like the pattern of keeping the rule of only linting one thing!
noahdietz: @radhus thank you very much!

Patch:
diff --git a/rules/aip0203/resource_name_identifier.go b/rules/aip0203/resource_name_identifier.go
index 556bc51..b5d664c 100644
--- a/rules/aip0203/resource_name_identifier.go
+++ b/rules/aip0203/resource_name_identifier.go
@@ -23,8 +23,10 @@ import (
 )
 
 var resourceNameIdentifier = &lint.MessageRule{
-	Name:   lint.NewRuleName(203, "resource-name-identifier"),
-	OnlyIf: utils.IsResource,
+	Name: lint.NewRuleName(203, "resource-name-identifier"),
+	OnlyIf: func(m *desc.MessageDescriptor) bool {
+		return utils.IsResource(m) && m.FindFieldByName(utils.GetResourceNameField(utils.GetResource(m))) != nil
+	},
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
 		f := m.FindFieldByName(utils.GetResourceNameField(utils.GetResource(m)))
 		fb := utils.GetFieldBehavior(f)


Test patch:
diff --git a/rules/aip0203/resource_name_identifier_test.go b/rules/aip0203/resource_name_identifier_test.go
index 02c206d..4b8de4f 100644
--- a/rules/aip0203/resource_name_identifier_test.go
+++ b/rules/aip0203/resource_name_identifier_test.go
@@ -37,6 +37,11 @@ func TestResourceNameIdentifier(t *testing.T) {
 			NameField: "resource_name",
 			problems:  nil,
 		},
+		{
+			name:     "SkipMissingNameField",
+			Field:    "string other = 1;",
+			problems: nil,
+		},
 		{
 			name:     "InvalidNoFieldBehavior",
 			Field:    "string name = 1;",