From 199432d50c952ba22b832a93bc5724b64bc4ed17 Mon Sep 17 00:00:00 2001 From: Andrew Paseltiner Date: Wed, 21 Apr 2021 15:59:38 -0400 Subject: [PATCH] feat: Require show_deleted List request field for resources supporting soft delete (#833) --- .../0132/request-show-deleted-required.md | 80 +++++++++++++++++++ rules/aip0132/aip0132.go | 1 + .../aip0132/request_show_deleted_required.go | 41 ++++++++++ .../request_show_deleted_required_test.go | 44 ++++++++++ 4 files changed, 166 insertions(+) create mode 100644 docs/rules/0132/request-show-deleted-required.md create mode 100644 rules/aip0132/request_show_deleted_required.go create mode 100644 rules/aip0132/request_show_deleted_required_test.go diff --git a/docs/rules/0132/request-show-deleted-required.md b/docs/rules/0132/request-show-deleted-required.md new file mode 100644 index 000000000..541e1ecf2 --- /dev/null +++ b/docs/rules/0132/request-show-deleted-required.md @@ -0,0 +1,80 @@ +--- +rule: + aip: 132 + name: [core, '0132', request-show-deleted-required] + summary: List requests must have a `show-deleted` field for resources + supporting soft delete. +permalink: /132/request-show-deleted-required +redirect_from: + - /0132/request-show-deleted-required +--- + +# List methods: `show_deleted` field + +This rule enforces that all `List` standard methods have a `bool show_deleted` +field in the request message if the resource supports soft delete, as mandated +in [AIP-132](http://aip.dev/132). + +## Details + +This rule looks at any message matching `List*Request` and complains if the +`show_deleted` field is missing and the corresponding resource has an +`Undelete` method. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. + +service Library { + ... + rpc UndeleteBook(UndeleteBookRequest) returns (Book) { ... } +} + +// Missing the `bool show_deleted` field. +message ListBooksRequest { + string parent = 1; + int32 page_size = 2; + string page_token = 3; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. + +service Library { + ... + rpc UndeleteBook(UndeleteBookRequest) returns (Book) { ... } +} + +message ListBooksRequest { + string parent = 1; + int32 page_size = 2; + string page_token = 3; + bool show_deleted = 4; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the message. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// (-- api-linter: core::0132::request-show-deleted-required=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +message ListBooksRequest { + string parent = 1; + int32 page_size = 2; + string page_token = 3; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0132/aip0132.go b/rules/aip0132/aip0132.go index 65aeebd21..9726ff01b 100644 --- a/rules/aip0132/aip0132.go +++ b/rules/aip0132/aip0132.go @@ -38,6 +38,7 @@ func AddRules(r lint.RuleRegistry) error { requestParentReference, requestParentValidReference, requestParentRequired, + requestShowDeletedRequired, responseMessageName, responseUnknownFields, unknownFields, diff --git a/rules/aip0132/request_show_deleted_required.go b/rules/aip0132/request_show_deleted_required.go new file mode 100644 index 000000000..b56ac587f --- /dev/null +++ b/rules/aip0132/request_show_deleted_required.go @@ -0,0 +1,41 @@ +package aip0132 + +import ( + "fmt" + "strings" + + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" + "github.com/stoewer/go-strcase" +) + +// List requests should contain a show_deleted field if the resource supports +// soft delete. +var requestShowDeletedRequired = &lint.MessageRule{ + Name: lint.NewRuleName(132, "request-show-deleted-required"), + OnlyIf: func(m *desc.MessageDescriptor) bool { + if !isListRequestMessage(m) { + return false + } + // Check for soft-delete support by getting the resource name + // from the corresponding response message. + plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "List") + if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("List%sResponse", plural)); resp != nil { + if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil { + singular := paged.GetMessageType().GetName() + return utils.FindMethod(m.GetFile(), "Undelete"+singular) != nil + } + } + return false + }, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + if m.FindFieldByName("show_deleted") != nil { + return nil + } + return []lint.Problem{{ + Message: "List requests for resources supporting soft delete must have a `bool show_deleted` field.", + Descriptor: m, + }} + }, +} diff --git a/rules/aip0132/request_show_deleted_required_test.go b/rules/aip0132/request_show_deleted_required_test.go new file mode 100644 index 000000000..92673cc78 --- /dev/null +++ b/rules/aip0132/request_show_deleted_required_test.go @@ -0,0 +1,44 @@ +package aip0132 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestRequestShowDeletedRequired(t *testing.T) { + tests := []struct { + name string + Message string + Method string + Field string + problems testutils.Problems + }{ + {"Valid", "ListBooksRequest", "UndeleteBook", `bool show_deleted = 1;`, nil}, + {"Invalid", "ListBooksRequest", "UndeleteBook", ``, testutils.Problems{{Message: "show_deleted"}}}, + {"IrrelevantNoSoftDelete", "ListBooksRequest", "GetBook", ``, nil}, + {"IrrelevantMessage", "EnumerateBooksRequest", "UndeleteBook", "", nil}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + service Library { + rpc {{.Method}}({{.Method}}Request) returns (Book); + } + message Book {} + message {{.Message}} { + {{.Field}} + } + message ListBooksResponse { + repeated Book books = 1; + } + message {{.Method}}Request {} + `, test) + problems := requestShowDeletedRequired.Lint(f) + if diff := test.problems.SetDescriptor(f.GetMessageTypes()[1]).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } +}