Skip to content

Commit

Permalink
feat: Require show_deleted List request field for resources supportin…
Browse files Browse the repository at this point in the history
…g soft delete (#833)
  • Loading branch information
apasel422 authored Apr 21, 2021
1 parent 26a1c82 commit 199432d
Show file tree
Hide file tree
Showing 4 changed files with 166 additions and 0 deletions.
80 changes: 80 additions & 0 deletions docs/rules/0132/request-show-deleted-required.md
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions rules/aip0132/aip0132.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func AddRules(r lint.RuleRegistry) error {
requestParentReference,
requestParentValidReference,
requestParentRequired,
requestShowDeletedRequired,
responseMessageName,
responseUnknownFields,
unknownFields,
Expand Down
41 changes: 41 additions & 0 deletions rules/aip0132/request_show_deleted_required.go
Original file line number Diff line number Diff line change
@@ -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,
}}
},
}
44 changes: 44 additions & 0 deletions rules/aip0132/request_show_deleted_required_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}

0 comments on commit 199432d

Please sign in to comment.