Skip to content

Commit

Permalink
feat: test across multiple files, ignore name on resources (#1172)
Browse files Browse the repository at this point in the history
  • Loading branch information
@shwoodard
shwoodard authored Jun 2, 2023
1 parent ddd5572 commit c17a973
Show file tree
Hide file tree
Showing 2 changed files with 199 additions and 36 deletions.
61 changes: 38 additions & 23 deletions rules/aip0203/field_behavior_required.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,40 +30,55 @@ var minimumRequiredFieldBehavior = stringset.New(
var fieldBehaviorRequired = &lint.MethodRule{
Name: lint.NewRuleName(203, "field-behavior-required"),
LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
// we only check requests, as OutputTypes are always
// OUTPUT_ONLY
it := m.GetInputType()
return checkFields(it)
req := m.GetInputType()
ps := problems(req)
if len(ps) == 0 {
return nil
}

return ps
},
}

func checkFields(m *desc.MessageDescriptor) []lint.Problem {
problems := []lint.Problem{}
func problems(m *desc.MessageDescriptor) []lint.Problem {
var ps []lint.Problem

for _, f := range m.GetFields() {
asMessage := f.GetMessageType()
if asMessage != nil {
problems = append(problems, checkFields(asMessage)...)
if utils.IsResource(m) && f.GetName() == "name" {
continue
}

p := checkFieldBehavior(f)
if p != nil {
ps = append(ps, *p)
}

if mt := f.GetMessageType(); mt != nil {
ps = append(ps, problems(mt)...)
}
problems = append(problems, checkFieldBehavior(f)...)
}
return problems

return ps
}

func checkFieldBehavior(f *desc.FieldDescriptor) []lint.Problem {
problems := []lint.Problem{}
fieldBehavior := utils.GetFieldBehavior(f)
if len(fieldBehavior) == 0 {
problems = append(problems, lint.Problem{
Message: fmt.Sprintf("google.api.field_behavior annotation must be set, and have one of %v", minimumRequiredFieldBehavior),
func checkFieldBehavior(f *desc.FieldDescriptor) *lint.Problem {
fb := utils.GetFieldBehavior(f)

if len(fb) == 0 {
return &lint.Problem{
Message: fmt.Sprintf("google.api.field_behavior annotation must be set on %q and contain one of, \"%v\"", f.GetName(), minimumRequiredFieldBehavior),
Descriptor: f,
})
}
}

if !minimumRequiredFieldBehavior.Intersects(fb) {
// check for at least one valid annotation
} else if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) {
problems = append(problems, lint.Problem{
return &lint.Problem{
Message: fmt.Sprintf(
"google.api.field_behavior must have at least one of the following behaviors set: %v", minimumRequiredFieldBehavior),
"google.api.field_behavior on field %q must contain at least one, \"%v\"", f.GetName(), minimumRequiredFieldBehavior),
Descriptor: f,
})
}
}
return problems

return nil
}
174 changes: 161 additions & 13 deletions rules/aip0203/field_behavior_required_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import (

const ()

func TestFieldBehaviorRequired(t *testing.T) {
for _, test := range []struct {
func TestFieldBehaviorRequired_SingleFile_SingleMessage(t *testing.T) {
testCases := []struct {
name string
Fields string
problems testutils.Problems
Expand Down Expand Up @@ -51,7 +51,7 @@ func TestFieldBehaviorRequired(t *testing.T) {
"ValidOptionalImmutable",
`int32 page_count = 1 [
(google.api.field_behavior) = OUTPUT_ONLY,
(google.api.field_behavior) = OPTIONAL
(google.api.field_behavior) = OPTIONAL
];`,
nil,
},
Expand All @@ -60,10 +60,12 @@ func TestFieldBehaviorRequired(t *testing.T) {
"int32 page_count = 1;",
testutils.Problems{{Message: "annotation must be set"}},
},
} {
t.Run(test.name, func(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/api/field_behavior.proto";
import "google/api/resource.proto";
service Library {
rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) {
Expand All @@ -72,24 +74,91 @@ func TestFieldBehaviorRequired(t *testing.T) {
message UpdateBookRequest {
{{.Fields}}
Book book = 2 [(google.api.field_behavior) = REQUIRED];
}
message UpdateBookResponse {
// verifies that no error was raised on lack
// of field behavior in the response.
string name = 1;
}
`, test)
message Book {
option (google.api.resource) = {
type: "library.googleapis.com/Book"
pattern: "books/{book}"
};
string name = 1;
}
`, tc)

field := f.GetMessageTypes()[0].GetFields()[0]
if diff := test.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {

if diff := tc.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
}
}

func TestFieldBehaviorRequired_Resource_SingleFile(t *testing.T) {
testCases := []struct {
name string
FieldBehavior string
problems testutils.Problems
}{
{
name: "valid with field behavior",
FieldBehavior: "[(google.api.field_behavior) = OUTPUT_ONLY]",
problems: nil,
},
{
name: "valid without field behavior",
FieldBehavior: "",
problems: nil,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/api/field_behavior.proto";
import "google/api/resource.proto";
service Library {
rpc GetBook(GetBookRequest) returns (Book) {
}
}
message GetBookRequest {
string name = 1 [(google.api.field_behavior) = REQUIRED];
}
message Book {
option (google.api.resource) = {
type: "library.googleapis.com/Book"
pattern: "books/{book}"
};
string name = 1;
string title = 2 {{.FieldBehavior}};
}
`, tc)

field := f.GetMessageTypes()[1].GetFields()[1]

if diff := tc.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
}

}

func TestFieldBehaviorRequiredNested(t *testing.T) {
for _, test := range []struct {
func TestFieldBehaviorRequired_NestedMessages_SingleFile(t *testing.T) {
testCases := []struct {
name string
Fields string
problems testutils.Problems
Expand All @@ -104,8 +173,9 @@ func TestFieldBehaviorRequiredNested(t *testing.T) {
"NonAnnotated non_annotated = 1 [(google.api.field_behavior) = REQUIRED];",
testutils.Problems{{Message: "must be set"}},
},
} {
t.Run(test.name, func(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/api/field_behavior.proto";
Expand All @@ -131,12 +201,90 @@ func TestFieldBehaviorRequiredNested(t *testing.T) {
// of field behavior in the response.
string name = 1;
}
`, test)
`, tc)

it := f.GetServices()[0].GetMethods()[0].GetInputType()
nestedField := it.GetFields()[0].GetMessageType().GetFields()[0]
if diff := test.problems.SetDescriptor(nestedField).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {

if diff := tc.problems.SetDescriptor(nestedField).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
}
}

func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
testCases := []struct {
name string
MessageType string
MessageFieldName string
problems testutils.Problems
}{
{
"ValidAnnotatedAndChildAnnotated",
"Annotated",
"annotated",
nil,
},
{
"InvalidChildNotAnnotated",
"NonAnnotated",
"non_annotated",
testutils.Problems{{Message: "must be set"}},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
f1 := `
import "google/api/field_behavior.proto";
import "resource.proto";
service Library {
rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) {
}
}
message UpdateBookRequest {
{{.MessageType}} {{.MessageFieldName}} = 1 [(google.api.field_behavior) = REQUIRED];
}
message UpdateBookResponse {
// verifies that no error was raised on lack
// of field behavior in the response.
string name = 1;
}
`

f2 := `
import "google/api/field_behavior.proto";
message NonAnnotated {
string nested = 1;
}
message Annotated {
string nested = 1 [(google.api.field_behavior) = REQUIRED];
}
`

srcs := map[string]string{
"service.proto": f1,
"resource.proto": f2,
}

ds := testutils.ParseProto3Tmpls(t, srcs, tc)
f := ds["service.proto"]
it := f.GetServices()[0].GetMethods()[0].GetInputType()
fd := it.GetFields()[0].GetMessageType().GetFields()[0]

if diff := tc.problems.SetDescriptor(fd).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {
t.Errorf(diff)
}

want := "resource.proto"
if got := fd.GetFile().GetName(); got != want {
t.Fatalf("got file name %q for location of field but wanted %q", got, want)
}
})
}
}

0 comments on commit c17a973

Please sign in to comment.