fix(AIP-203): avoid panic if name field is missing

[radhus]

This proto reproduces the following crash:

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];
}

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/[email protected]/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.

[noahdietz]

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:

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.

[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:

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 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!

[noahdietz]

This will be in v1.59.1 #1283