From 4f5c7acdc5af815cacb5db1eb994ee9a058750c9 Mon Sep 17 00:00:00 2001 From: Mira Leung Date: Tue, 27 Apr 2021 10:35:42 -0700 Subject: [PATCH] feat: Add linter for AIP-192 deprecated service and methods (#836) --- docs/rules/0192/deprecated-comment.md | 71 +++++++++++++++++ rules/aip0192/aip0192.go | 14 ++++ rules/aip0192/deprecated_comment.go | 37 +++++++++ rules/aip0192/deprecated_test.go | 106 ++++++++++++++++++++++++++ 4 files changed, 228 insertions(+) create mode 100644 docs/rules/0192/deprecated-comment.md create mode 100644 rules/aip0192/deprecated_comment.go create mode 100644 rules/aip0192/deprecated_test.go diff --git a/docs/rules/0192/deprecated-comment.md b/docs/rules/0192/deprecated-comment.md new file mode 100644 index 000000000..87bd38ee6 --- /dev/null +++ b/docs/rules/0192/deprecated-comment.md @@ -0,0 +1,71 @@ +--- +rule: + aip: 192 + name: [core, '0192', deprecated-comment] + summary: Deprecated methods must have a corresponding comment. +permalink: /192/deprecated-comment +redirect_from: + - /0192/deprecated-comment +--- + +# Deprecated comments + +This rule enforces that every RPC marked with the protobuf `deprecated` option +has `"Deprecated: "` as the first line in the public leading comment, as +mandated in [AIP-192][]. + +## Details + +This rule looks at each method descriptor in each proto file, and complains if +the protobuf `deprecated` option is set to `true`, but the first line of the public +comment does not begin with "Deprecated: ". + +## Examples + +**Incorrect** code for this rule: + +```proto +// A library service. +service Library { + // Incorrect. + // Retrieves a book. + rpc GetBook(GetBookRequest) returns (Book) { + option deprecated = true; + } +} +``` + +**Correct** code for this rule: + +```proto +// A library service. +service Library { + // Deprecated: Please borrow a physical book instead. + // Correct. + // Retrieves a book. + rpc GetBook(GetBookRequest) returns (Book) { + option deprecated = true; + } +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the descriptor. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// A library service. +service Library { + // (-- api-linter: core::0192::deprecated-comment=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + // Incorrect. + // Retrieves a book. + rpc GetBook(GetBookRequest) returns (Book) { + option deprecated = true; + } +} +``` + +[aip-192]: https://aip.dev/192 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0192/aip0192.go b/rules/aip0192/aip0192.go index 82c4c93f0..d58f88858 100644 --- a/rules/aip0192/aip0192.go +++ b/rules/aip0192/aip0192.go @@ -19,6 +19,7 @@ import ( "strings" "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" ) // AddRules adds all of the AIP-192 rules to the provided registry. @@ -26,6 +27,7 @@ func AddRules(r lint.RuleRegistry) error { return r.Register( 192, absoluteLinks, + deprecatedComment, hasComments, noHTML, noMarkdownHeadings, @@ -35,6 +37,18 @@ func AddRules(r lint.RuleRegistry) error { ) } +// Returns true if this is a deprecated method or service, false otherwise. +func isDeprecated(d desc.Descriptor) bool { + switch d := d.(type) { + case *desc.MethodDescriptor: + return d.GetMethodOptions().GetDeprecated() + case *desc.ServiceDescriptor: + return d.GetServiceOptions().GetDeprecated() + default: + return false + } +} + func separateInternalComments(comments ...string) struct { Internal []string External []string diff --git a/rules/aip0192/deprecated_comment.go b/rules/aip0192/deprecated_comment.go new file mode 100644 index 000000000..4e1de3f82 --- /dev/null +++ b/rules/aip0192/deprecated_comment.go @@ -0,0 +1,37 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0192 + +import ( + "strings" + + "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" +) + +var deprecatedComment = &lint.DescriptorRule{ + Name: lint.NewRuleName(192, "deprecated-comment"), + OnlyIf: isDeprecated, + LintDescriptor: func(d desc.Descriptor) []lint.Problem { + comment := separateInternalComments(d.GetSourceInfo().GetLeadingComments()) + if len(comment.External) > 0 && strings.HasPrefix(comment.External[0], "Deprecated:") { + return nil + } + return []lint.Problem{{ + Message: `Use "Deprecated: " as the first line in the comment.`, + Descriptor: d, + }} + }, +} diff --git a/rules/aip0192/deprecated_test.go b/rules/aip0192/deprecated_test.go new file mode 100644 index 000000000..a69c1783c --- /dev/null +++ b/rules/aip0192/deprecated_test.go @@ -0,0 +1,106 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0192 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +// These are split up since templating doesn't play nicely with inserting protobuf options. +func TestValidDescriptor(t *testing.T) { + file := testutils.ParseProto3String(t, ` + // A library service. + service Library { + // Retrieves a book. + rpc GetBook(GetBookRequest) returns (Book); + } + message GetBookRequest {} + message Book {} + `) + + serviceProblems := testutils.Problems{} + if diff := serviceProblems.Diff(deprecatedComment.Lint(file)); diff != "" { + t.Error(diff) + } + + methodProblems := testutils.Problems{} + if diff := methodProblems.Diff(deprecatedComment.Lint(file)); diff != "" { + t.Error(diff) + } +} + +func TestDeprecatedMethod(t *testing.T) { + tests := []struct { + testName string + MethodComment string + problems testutils.Problems + }{ + {"ValidMethodDeprecated", "// Deprecated: Don't use this.\n// Method comment.", nil}, + {"InvalidMethodDeprecated", "// Method comment.", testutils.Problems{{Message: `Use "Deprecated: "`}}}, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + service Library { + + {{.MethodComment}} + rpc GetBook(GetBookRequest) returns (Book) { + option deprecated = true; + } + } + message GetBookRequest {} + message Book {} + `, test) + + problems := deprecatedComment.Lint(file) + if diff := test.problems.SetDescriptor(file.GetServices()[0].GetMethods()[0]).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } +} + +func TestDeprecatedService(t *testing.T) { + tests := []struct { + testName string + ServiceComment string + problems testutils.Problems + }{ + {"ValidServiceDeprecated", "// Deprecated: Don't use this.\n// Service comment.", nil}, + {"InvalidServiceDeprecated", "// Service comment.", testutils.Problems{{Message: `Use "Deprecated: "`}}}, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + {{.ServiceComment}} + service Library { + option deprecated = true; + rpc GetBook(GetBookRequest) returns (Book); + } + message GetBookRequest {} + message Book {} + `, test) + + problems := deprecatedComment.Lint(file) + if diff := test.problems.SetDescriptor(file.GetServices()[0]).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } +}