Repository: googleapis/api-linter
Pull request #1184 (2023-06-29 22:05:04)

fix(AIP-203): no check for OneOf fields
OneOf fields should not be checked for annotations, as they are implicitly optional.

However, if a sub-message is used in the OneOf,
it should be verified.


Patch:
diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md
index 29b6322..6091c1a 100644
--- a/docs/rules/0203/field-behavior-required.md
+++ b/docs/rules/0203/field-behavior-required.md
@@ -18,10 +18,10 @@ This rule enforces that each field in a message used in a request has a
 
 ## Details
 
-This rule looks at all fields and ensures they have a
-`google.api.field_behavior` annotation. It also verifies that they have at least
-one of the options `OUTPUT_ONLY`, `REQUIRED`, or `OPTIONAL`: all fields must
-fall into one of these categories.
+This rule looks at all fields except those nested inside a OneOf and ensures
+they have a `google.api.field_behavior` annotation. It also verifies that they
+have at least one of the options `OUTPUT_ONLY`, `REQUIRED`, or `OPTIONAL`: all
+fields must fall into one of these categories.
 
 Although all request messages **must** be annotated, this linter only verifies
 messages that are in the same package as some upstream protos (e.g. common
diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go
index be9f454..bafb050 100644
--- a/rules/aip0203/field_behavior_required.go
+++ b/rules/aip0203/field_behavior_required.go
@@ -49,9 +49,12 @@ func problems(m *desc.MessageDescriptor, pkg string) []lint.Problem {
 			continue
 		}
 
-		p := checkFieldBehavior(f)
-		if p != nil {
-			ps = append(ps, *p)
+		// Ignore a field if it is a OneOf (do not ignore children)
+		if f.AsFieldDescriptorProto().OneofIndex == nil {
+			p := checkFieldBehavior(f)
+			if p != nil {
+				ps = append(ps, *p)
+			}
 		}
 
 		if mt := f.GetMessageType(); mt != nil && !mt.IsMapEntry() && mt.GetFile().GetPackage() == pkg {


Test patch:
diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go
index ff9f9c5..ff81bb9 100644
--- a/rules/aip0203/field_behavior_required_test.go
+++ b/rules/aip0203/field_behavior_required_test.go
@@ -49,6 +49,16 @@ func TestFieldBehaviorRequired_SingleFile_SingleMessage(t *testing.T) {
 			"map<string, string> page_count = 1 [(google.api.field_behavior) = OPTIONAL];",
 			nil,
 		},
+		// OneOfs are not required to have an annotation, as they
+		// are implicitly optional.
+		{
+			"ValidOneOfNoAnnotation",
+			`oneof candy_bar {
+				bool snickers = 1;
+				bool chocolate = 3;
+			}`,
+			nil,
+		},
 		{
 			"ValidOutputOnly",
 			"int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];",
@@ -181,6 +191,14 @@ func TestFieldBehaviorRequired_NestedMessages_SingleFile(t *testing.T) {
 			"NonAnnotated non_annotated = 1 [(google.api.field_behavior) = REQUIRED];",
 			testutils.Problems{{Message: "must be set"}},
 		},
+		// Children of OneOfs should still be validated.
+		{
+			"InvalidOneOfChildNotAnnotated",
+			`oneof candy_bar {
+				NonAnnotated non_annotated = 1;
+			}`,
+			testutils.Problems{{Message: "must be set"}},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {