bugfix: dont throw error if +required is set with omitempty

previous PR had a bug
kubernetes · Feb 23, 2024 · 5b141d2 · 5b141d2
1 parent d73fd71
commit 5b141d2
Show file tree

Hide file tree

Showing 3 changed files with 123 additions and 14 deletions.
diff --git a/pkg/generators/openapi.go b/pkg/generators/openapi.go
@@ -81,19 +81,24 @@ func hasOpenAPITagValue(comments []string, value string) bool {
 	return false
 }
 
-func hasRequiredTag(m *types.Member) bool {
-	return types.ExtractCommentTags(
-		"+", m.CommentLines)[tagRequired] != nil
-}
-
 // hasOptionalTag returns true if the member has +optional in its comments or
 // omitempty in its json tags.
-func hasOptionalTag(m *types.Member) bool {
+func hasOptionalTag(m *types.Member) (bool, error) {
 	hasOptionalCommentTag := types.ExtractCommentTags(
 		"+", m.CommentLines)[tagOptional] != nil
-	hasOptionalJsonTag := strings.Contains(
-		reflect.StructTag(m.Tags).Get("json"), "omitempty")
-	return hasOptionalCommentTag || hasOptionalJsonTag
+	hasRequiredCommentTag := types.ExtractCommentTags(
+		"+", m.CommentLines)[tagRequired] != nil
+	if hasOptionalCommentTag && hasRequiredCommentTag {
+		return false, fmt.Errorf("member %s cannot be both optional and required", m.Name)
+	} else if hasRequiredCommentTag {
+		return false, nil
+	} else if hasOptionalCommentTag {
+		return true, nil
+	}
+
+	// If neither +optional nor +required is present in the comments,
+	// infer optional from the json tags.
+	return strings.Contains(reflect.StructTag(m.Tags).Get("json"), "omitempty"), nil
 }
 
 func apiTypeFilterFunc(c *generator.Context, t *types.Type) bool {
@@ -323,10 +328,10 @@ func (g openAPITypeWriter) generateMembers(t *types.Type, required []string) ([]
 		if name == "" {
 			continue
 		}
-		if isOptional, isRequired := hasOptionalTag(&m), hasRequiredTag(&m); isOptional && isRequired {
+		if isOptional, err := hasOptionalTag(&m); err != nil {
 			klog.Errorf("Error when generating: %v, %v\n", name, m)
-			return required, fmt.Errorf("member %s of type %s cannot be both optional and required", m.Name, t.Name)
-		} else if !isOptional || isRequired {
+			return required, err
+		} else if !isOptional {
 			required = append(required, name)
 		}
 		if err = g.generateProperty(&m, t); err != nil {

diff --git a/pkg/generators/openapi_test.go b/pkg/generators/openapi_test.go
@@ -2187,6 +2187,110 @@ func TestMultilineCELMarkerComments(t *testing.T) {
 	}
 }
 
+func TestRequired(t *testing.T) {
+	callErr, funcErr, assert, _, funcBuffer, imports := testOpenAPITypeWriter(t, `
+		package foo
+
+		// +k8s:openapi-gen=true
+		type Blah struct {
+			// +optional
+			OptionalField string
+
+			// +required
+			RequiredField string
+
+			// +required
+			RequiredPointerField *string `+"`json:\"requiredPointerField,omitempty\"`"+`
+
+			// +optional
+			OptionalPointerField *string `+"`json:\"optionalPointerField,omitempty\"`"+`
+
+			ImplicitlyRequiredField string
+			ImplicitlyOptionalField string `+"`json:\"implicitlyOptionalField,omitempty\"`"+`
+		}
+	`)
+
+	assert.NoError(funcErr)
+	assert.NoError(callErr)
+	assert.ElementsMatch(imports, []string{`foo "base/foo"`, `common "k8s.io/kube-openapi/pkg/common"`, `spec "k8s.io/kube-openapi/pkg/validation/spec"`})
+
+	if formatted, err := format.Source(funcBuffer.Bytes()); err != nil {
+		t.Fatalf("%v\n%v", err, string(funcBuffer.Bytes()))
+	} else {
+		formatted_expected, ree := format.Source([]byte(`func schema_base_foo_Blah(ref common.ReferenceCallback) common.OpenAPIDefinition {
+		return common.OpenAPIDefinition{
+			Schema: spec.Schema{
+				SchemaProps: spec.SchemaProps{
+					Type: 			  []string{"object"},
+					Properties: map[string]spec.Schema{
+						"OptionalField": {
+							SchemaProps: spec.SchemaProps{
+								Default: "",
+								Type:    []string{"string"},
+								Format:  "",
+							},
+						},
+   						"RequiredField": {
+   							SchemaProps: spec.SchemaProps{
+   								Default: "",
+   								Type:    []string{"string"},
+   								Format:  "",
+   							},
+   						},
+   						"requiredPointerField": {
+   							SchemaProps: spec.SchemaProps{
+   								Type:   []string{"string"},
+   								Format: "",
+   							},
+   						},
+   						"optionalPointerField": {
+   							SchemaProps: spec.SchemaProps{
+   								Type:   []string{"string"},
+   								Format: "",
+   							},
+   						},
+   						"ImplicitlyRequiredField": {
+   							SchemaProps: spec.SchemaProps{
+   								Default: "",
+   								Type:    []string{"string"},
+   								Format:  "",
+   							},
+   						},
+   						"implicitlyOptionalField": {
+   							SchemaProps: spec.SchemaProps{
+   								Type:   []string{"string"},
+   								Format: "",
+   							},
+   						},
+					},
+					Required: []string{"RequiredField", "requiredPointerField", "ImplicitlyRequiredField"},
+				},
+			},
+		}
+	}
+
+`))
+		if ree != nil {
+			t.Fatal(ree)
+		}
+		assert.Equal(string(formatted_expected), string(formatted))
+	}
+
+	// Show specifying both is an error
+	callErr, funcErr, assert, _, _, _ = testOpenAPITypeWriter(t, `
+	package foo
+
+	// +k8s:openapi-gen=true
+	type Blah struct {
+		// +optional
+		// +required
+		ConfusingField string
+	}
+`)
+	assert.NoError(callErr)
+	assert.ErrorContains(funcErr, "cannot be both optional and required")
+}
+
 func TestMarkerCommentsCustomDefsV3(t *testing.T) {
 	callErr, funcErr, assert, callBuffer, funcBuffer, _ := testOpenAPITypeWriter(t, `
 package foo

diff --git a/pkg/generators/union.go b/pkg/generators/union.go
@@ -163,7 +163,7 @@ func parseUnionStruct(t *types.Type) (*union, []error) {
 		if types.ExtractCommentTags("+", m.CommentLines)[tagUnionDiscriminator] != nil {
 			errors = append(errors, u.setDiscriminator(jsonName)...)
 		} else {
-			if !hasOptionalTag(&m) {
+			if optional, err := hasOptionalTag(&m); !optional || err != nil {
 				errors = append(errors, fmt.Errorf("union members must be optional: %v.%v", t.Name, m.Name))
 			}
 			u.addMember(jsonName, m.Name)
@@ -194,7 +194,7 @@ func parseUnionMembers(t *types.Type) (*union, []error) {
 			continue
 		}
 		if types.ExtractCommentTags("+", m.CommentLines)[tagUnionDeprecated] != nil {
-			if !hasOptionalTag(&m) {
+			if optional, err := hasOptionalTag(&m); !optional || err != nil {
 				errors = append(errors, fmt.Errorf("union members must be optional: %v.%v", t.Name, m.Name))
 			}
 			u.addMember(jsonName, m.Name)