From b40c90d9b1a30d50c08e1373dd9c7b468dd651c2 Mon Sep 17 00:00:00 2001 From: Andrei Scripniciuc <46186670+andrei-scripniciuc@users.noreply.github.com> Date: Wed, 20 Dec 2023 13:48:15 -0800 Subject: [PATCH] feat: undelete should not be required for declarative-friendly interfaces (#1304) according to https://google.aip.dev/164#declarative-friendly-resources: > Soft-deletable resources have a poorer experience than hard-deleted resources in declarative clients this contradicts the requirement that declarative-friendly interfaces implement soft-delete. --- .../0164/declarative-friendly-required.md | 93 ------------------- rules/aip0164/aip0164.go | 1 - .../aip0164/declarative_friendly_required.go | 32 ------- .../declarative_friendly_required_test.go | 63 ------------- 4 files changed, 189 deletions(-) delete mode 100644 docs/rules/0164/declarative-friendly-required.md delete mode 100644 rules/aip0164/declarative_friendly_required.go delete mode 100644 rules/aip0164/declarative_friendly_required_test.go diff --git a/docs/rules/0164/declarative-friendly-required.md b/docs/rules/0164/declarative-friendly-required.md deleted file mode 100644 index b6182c11f..000000000 --- a/docs/rules/0164/declarative-friendly-required.md +++ /dev/null @@ -1,93 +0,0 @@ ---- -rule: - aip: 164 - name: [core, '0164', declarative-friendly-required] - summary: Declarative-friendly resources should have an Undelete method. -permalink: /164/declarative-friendly-required -redirect_from: - - /0164/declarative-friendly-required ---- - -# Declarative-friendly resources: Undelete method required - -This rule enforces that all declarative-friendly resources have an Undelete -method, as mandated in [AIP-164][]. - -## Details - -This rule looks at any resource with a `google.api.resource` annotation that -includes `style: DECLARATIVE_FRIENDLY`, and complains if it does not have a -corresponding Undelete method. - -## Examples - -**Incorrect** code for this rule: - -```proto -// Incorrect. -// Should have an `UndeleteBook` method. -service Library { -} - -message Book { - option (google.api.resource) = { - type: "library.googleapis.com/Book" - pattern: "publishers/{publisher}/books/{book}" - style: DECLARATIVE_FRIENDLY - }; - - string name = 1; -} -``` - -**Correct** code for this rule: - -```proto -// Correct. -service Library { - rpc UndeleteBook(UndeleteBookRequest) returns (Book) { - option (google.api.http) = { - post: "/v1/{name=publishers/*/books/*}:undelete" - body: "*" - }; - }; -} - -message Book { - option (google.api.resource) = { - type: "library.googleapis.com/Book" - pattern: "publishers/{publisher}/books/{book}" - style: DECLARATIVE_FRIENDLY - }; - - string name = 1; -} -``` - -## Disabling - -If you need to violate this rule, use a leading comment above the resource. -Remember to also include an [aip.dev/not-precedent][] comment explaining why. - -```proto -service Library { -} - -// (-- api-linter: core::0164::declarative-friendly-required=disabled -// aip.dev/not-precedent: We need to do this because reasons. --) -message Book { - option (google.api.resource) = { - type: "library.googleapis.com/Book" - pattern: "publishers/{publisher}/books/{book}" - style: DECLARATIVE_FRIENDLY - }; - - string name = 1; -} -``` - -If you need to violate this rule for an entire file, place the comment at the -top of the file. - -[aip-164]: https://aip.dev/164 -[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0164/aip0164.go b/rules/aip0164/aip0164.go index a737634bf..3cc198ae8 100644 --- a/rules/aip0164/aip0164.go +++ b/rules/aip0164/aip0164.go @@ -26,7 +26,6 @@ import ( func AddRules(r lint.RuleRegistry) error { return r.Register( 164, - declarativeFriendlyRequired, httpBody, httpMethod, httpURISuffix, diff --git a/rules/aip0164/declarative_friendly_required.go b/rules/aip0164/declarative_friendly_required.go deleted file mode 100644 index 73ba59e5d..000000000 --- a/rules/aip0164/declarative_friendly_required.go +++ /dev/null @@ -1,32 +0,0 @@ -package aip0164 - -import ( - "fmt" - - "github.com/googleapis/api-linter/lint" - "github.com/googleapis/api-linter/rules/internal/utils" - "github.com/jhump/protoreflect/desc" -) - -// Declarative-friendly resources should have an Undelete method. -var declarativeFriendlyRequired = &lint.MessageRule{ - Name: lint.NewRuleName(164, "declarative-friendly-required"), - OnlyIf: func(m *desc.MessageDescriptor) bool { - if resource := utils.DeclarativeFriendlyResource(m); resource == m { - return true - } - return false - }, - LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { - resource := m.GetName() - want := fmt.Sprintf("Undelete%s", resource) - delete := fmt.Sprintf("Delete%s", resource) - if utils.FindMethod(m.GetFile(), want) == nil && utils.FindMethod(m.GetFile(), delete) != nil { - return []lint.Problem{{ - Message: fmt.Sprintf("Declarative-friendly %s should have an Undelete method.", resource), - Descriptor: m, - }} - } - return nil - }, -} diff --git a/rules/aip0164/declarative_friendly_required_test.go b/rules/aip0164/declarative_friendly_required_test.go deleted file mode 100644 index 84c71e61c..000000000 --- a/rules/aip0164/declarative_friendly_required_test.go +++ /dev/null @@ -1,63 +0,0 @@ -package aip0164 - -import ( - "testing" - - "github.com/googleapis/api-linter/rules/internal/testutils" -) - -func TestDeclarativeFriendlyRequired(t *testing.T) { - for _, test := range []struct { - testName string - Style string - MethodName string - problems testutils.Problems - }{ - {"ValidDF", `style: DECLARATIVE_FRIENDLY`, `UndeleteBook`, nil}, - {"ValidNotDF", ``, `GetBook`, nil}, - {"InvalidDF", `style: DECLARATIVE_FRIENDLY`, `GetBook`, testutils.Problems{{Message: "Book should have an Undelete"}}}, - } { - t.Run(test.testName, func(t *testing.T) { - file := testutils.ParseProto3Tmpl(t, ` - import "google/api/resource.proto"; - import "google/protobuf/empty.proto"; - service Library { - rpc {{.MethodName}}({{.MethodName}}Request) returns (Book); - rpc DeleteBook(DeleteBookRequest) returns (google.protobuf.Empty); - } - message Book { - option (google.api.resource) = { - {{.Style}} - }; - } - message {{.MethodName}}Request {} - message DeleteBookRequest {} - `, test) - message := file.GetMessageTypes()[0] - problems := declarativeFriendlyRequired.Lint(file) - if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" { - t.Error(diff) - } - }) - } - - // Also test that undelete is not required if delete is not present. - t.Run("ValidNoDelete", func(t *testing.T) { - file := testutils.ParseProto3String(t, ` - import "google/api/resource.proto"; - service Library { - rpc GetBook(GetBookRequest) returns (Book); - } - message Book { - option (google.api.resource) = { - style: DECLARATIVE_FRIENDLY - }; - } - message GetBookRequest {} - `) - problems := declarativeFriendlyRequired.Lint(file) - if len(problems) > 0 { - t.Errorf("Got %v, expected no problems.", problems) - } - }) -}