Repository: googleapis/api-linter
Pull request #846 (2021-05-15 23:33:28)

feat: Support 'conditionally' required fields.
google.aip.dev issue: https://github.com/aip-dev/google.aip.dev/issues/411

Given a field:

```proto
message ExportBookRequest {
  bool enable_encryption = 1;
  // Required if enable_encryption is set to true.
  string encryption_key = 2;
}
```

`encryption_key` should not be annotated with `REQUIRED` since it is
only necessary if `enable_encryption` is set. This change ensures that
if a field comment contains 'required if', it will not trigger a linter
error.

Note there are other phrases that could logically apply (e.g. 'Only
required when'), but the resulting regex would be difficult to maintain.

RocketDonkey: Happy to adjust if this approach isn't correct (or drop it if you'd rather hold off on supporting this).
noahdietz: @RocketDonkey would it be sufficient to just disable the api-linter rule for the fields in your API that are conditionally required? https://linter.aip.dev/203/required#disabling. 

IMO, your suggestion is an OK one and I've seen this case before in API reviews, but we'd have to codify this and start over at https://github.com/aip-dev/google.aip.dev suggesting an amendment to the AIP before adding it to the linter. I'd say though that simply disabling the linter rule in these cases should be sufficient, WDYT?

Patch:
diff --git a/docs/rules/0203/required.md b/docs/rules/0203/required.md
index d3c20d4..583882b 100644
--- a/docs/rules/0203/required.md
+++ b/docs/rules/0203/required.md
@@ -18,6 +18,10 @@ machine-readable annotation, as mandated by [AIP-203][].
 This rule looks at any field with "required" (or similar forms) in the comment,
 and complains if it does not have a `google.api.field_behavior` annotation.
 
+**Note:** if a field is conditionally required (i.e. it is only required if
+another field is set), the phrases 'Required if' or 'Required when' in the
+comment will disable this rule.
+
 ## Examples
 
 **Incorrect** code for this rule:
@@ -55,6 +59,18 @@ message Book {
 }
 ```
 
+Conditionally required fields do not need to be annotated.
+
+```proto
+// Correct.
+message Book {
+  string name = 1;
+
+  // The title of the book. Required if the book has been published.
+  string title = 2;
+}
+```
+
 ## Disabling
 
 If you need to violate this rule, use a leading comment above the field.
diff --git a/rules/aip0203/required.go b/rules/aip0203/required.go
index ff4038c..1ed6852 100644
--- a/rules/aip0203/required.go
+++ b/rules/aip0203/required.go
@@ -23,7 +23,10 @@ import (
 var required = &lint.FieldRule{
 	Name:      lint.NewRuleName(203, "required"),
 	OnlyIf:    withoutFieldBehavior,
-	LintField: checkLeadingComments(requiredRegexp, "REQUIRED", optionalRegexp),
+	LintField: checkLeadingComments(requiredRegexp, "REQUIRED", optionalRegexp, conditionallyRequiredRegexp),
 }
 
 var requiredRegexp = regexp.MustCompile("(?i).*required.*")
+
+// 'Conditionally required' fields (denoted by 'required if/when') do not require an annotation.
+var conditionallyRequiredRegexp = regexp.MustCompile(`(?i).*required\s+(when|if)\s.*`)


Test patch:
diff --git a/rules/aip0203/required_test.go b/rules/aip0203/required_test.go
index fbd8b76..d6c0e1d 100644
--- a/rules/aip0203/required_test.go
+++ b/rules/aip0203/required_test.go
@@ -42,6 +42,47 @@ func TestRequired(t *testing.T) {
 			field:    titleWithRequiredBehavior,
 			problems: nil,
 		},
+		{
+			name:     "Valid-Required-if",
+			comment:  "Required if other condition",
+			field:    title,
+			problems: nil,
+		},
+		{
+			name:     "Valid-Required-when",
+			comment:  "Only required when other condition",
+			field:    title,
+			problems: nil,
+		},
+		{
+			name:     "Valid-Required-if-whitespace",
+			comment:  "Required   if other condition",
+			field:    title,
+			problems: nil,
+		},
+		{
+			name:     "Valid-Required-if-free-text",
+			comment:  "Only required if other condition",
+			field:    title,
+			problems: nil,
+		},
+		{
+			name:    "Valid-Required-starts-with-if",
+			comment: "Required iframe name",
+			field:   title,
+			problems: testutils.Problems{{
+				Message: "google.api.field_behavior",
+			}},
+		},
+		{
+			// Note this explicitly adds a comment marker on the second line in order
+			// to leverage the existing test setup.
+			name: "Valid-Required-if-multiline",
+			comment: `This field is only required
+		            // if condition is true`,
+			field:    title,
+			problems: nil,
+		},
 		{
 			name:    "Invalid-required",
 			comment: "required",