Merge pull request #455 from alexzielenski/required-bugfix

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

pkg/generators/enum.go

@@ -121,15 +121,23 @@ func parseEnums(c *generator.Context) enumMap {
 					Value:   *c.ConstValue,
 					Comment: strings.Join(c.CommentLines, " "),
 				}
-				enumTypes[enumType.Name].appendValue(value)
+				enumTypes[enumType.Name].addIfNotPresent(value)
 			}
 		}
 	}
 
 	return enumTypes
 }
 
-func (et *enumType) appendValue(value *enumValue) {
+func (et *enumType) addIfNotPresent(value *enumValue) {
+	// If we already have an enum case with the same value, then ignore this new
+	// one. This can happen if an enum aliases one from another package and
+	// re-exports the cases.
+	for _, existing := range et.Values {
+		if existing.Value == value.Value {
+			return
+		}
+	}
 	et.Values = append(et.Values, value)
 }
 

pkg/generators/enum_test.go

@@ -167,6 +167,91 @@ func TestParseEnums(t *testing.T) {
 				"foo.Foo": {"different", "same"},
 			},
 		},
+		{
+			name: "aliasing and re-exporting enum from different package",
+			universe: types.Universe{
+				"foo": &types.Package{
+					Name: "foo",
+					Types: map[string]*types.Type{
+						"Foo": {
+							Name: types.Name{
+								Package: "foo",
+								Name:    "Foo",
+							},
+							Kind:         types.Alias,
+							Underlying:   types.String,
+							CommentLines: []string{"+enum"},
+						},
+					},
+					Constants: map[string]*types.Type{
+						"FooCase1": {
+							Name: types.Name{
+								Package: "foo",
+								Name:    "FooCase1",
+							},
+							Kind: types.DeclarationOf,
+							Underlying: &types.Type{
+								Name: types.Name{
+									Package: "foo",
+									Name:    "Foo",
+								},
+							},
+							ConstValue: &[]string{"case1"}[0],
+						},
+						"FooCase2": {
+							Name: types.Name{
+								Package: "foo",
+								Name:    "FooCase2",
+							},
+							Kind: types.DeclarationOf,
+							Underlying: &types.Type{
+								Name: types.Name{
+									Package: "foo",
+									Name:    "Foo",
+								},
+							},
+							ConstValue: &[]string{"case2"}[0],
+						},
+					},
+				},
+				"bar": &types.Package{
+					Name: "bar",
+					Constants: map[string]*types.Type{
+						"FooCase1": {
+							Name: types.Name{
+								Package: "foo",
+								Name:    "FooCase1",
+							},
+							Kind: types.DeclarationOf,
+							Underlying: &types.Type{
+								Name: types.Name{
+									Package: "foo",
+									Name:    "Foo",
+								},
+							},
+							ConstValue: &[]string{"case1"}[0],
+						},
+						"FooCase2": {
+							Name: types.Name{
+								Package: "foo",
+								Name:    "FooCase2",
+							},
+							Kind: types.DeclarationOf,
+							Underlying: &types.Type{
+								Name: types.Name{
+									Package: "foo",
+									Name:    "Foo",
+								},
+							},
+							ConstValue: &[]string{"case2"}[0],
+						},
+					},
+				},
+			},
+			expected: map[string][]string{
+				"foo.Foo": {"case1", "case2"},
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			enums := parseEnums(&generator.Context{Universe: tc.universe})

pkg/generators/openapi.go

@@ -81,19 +81,25 @@ 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 {
+// isOptional returns error if the member has +optional and +required in
+// its comments. If +optional is present it returns true. If +required is present
+// it returns false. Otherwise, it returns true if `omitempty` JSON tag is present
+func isOptional(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 +329,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 := isOptional(&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 {

pkg/generators/openapi_test.go

@@ -49,10 +49,15 @@ func construct(t *testing.T, files map[string]string, testNamer namer.Namer) (*p
 }
 
 func testOpenAPITypeWriter(t *testing.T, code string) (error, error, *assert.Assertions, *bytes.Buffer, *bytes.Buffer, []string) {
+	return testOpenAPITypeWriterWithFiles(t, code, nil)
+}
+
+func testOpenAPITypeWriterWithFiles(t *testing.T, code string, testFiles map[string]string) (error, error, *assert.Assertions, *bytes.Buffer, *bytes.Buffer, []string) {
 	assert := assert.New(t)
-	var testFiles = map[string]string{
-		"base/foo/bar.go": code,
+	if testFiles == nil {
+		testFiles = map[string]string{}
 	}
+	testFiles["base/foo/bar.go"] = code
 	outputPackage := "base/output"
 	imports := generator.NewImportTrackerForPackage(outputPackage)
 	rawNamer := namer.NewRawNamer(outputPackage, imports)
@@ -1618,6 +1623,75 @@ map[string]interface{}{
 `, funcBuffer.String())
 }
 
+func TestEnumAlias(t *testing.T) {
+	callErr, funcErr, assert, _, funcBuffer, _ := testOpenAPITypeWriterWithFiles(t, `
+	package foo
+
+	import "base/bar"
+
+	// EnumType is the enumType.
+	// +enum
+	type EnumType = bar.EnumType
+	
+	// EnumA is a.
+	const EnumA EnumType = bar.EnumA
+	// EnumB is b.
+	const EnumB EnumType = bar.EnumB
+	
+	// Blah is a test.
+	// +k8s:openapi-gen=true
+	type Blah struct {
+		// Value is the value.
+		Value EnumType
+	}
+
+	`, map[string]string{"base/bar/foo.go": `
+	package bar
+
+	// EnumType is the enumType.
+	// +enum
+	type EnumType string
+	
+	// EnumA is a.
+	const EnumA EnumType = "a"
+	// EnumB is b.
+	const EnumB EnumType = "b"
+	`})
+
+	if callErr != nil {
+		t.Fatal(callErr)
+	}
+	if funcErr != nil {
+		t.Fatal(funcErr)
+	}
+	_ = assert
+	assert.Equal(`func schema_base_foo_Blah(ref common.ReferenceCallback) common.OpenAPIDefinition {
+return common.OpenAPIDefinition{
+Schema: spec.Schema{
+SchemaProps: spec.SchemaProps{
+Description: "Blah is a test.",
+Type: []string{"object"},
+Properties: map[string]spec.Schema{
+"Value": {
+SchemaProps: spec.SchemaProps{`+"\n"+
+		"Description: \"Value is the value.\\n\\nPossible enum values:\\n - `\\\"a\\\"` is a.\\n - `\\\"b\\\"` is b.\","+`
+Default: "",
+Type: []string{"string"},
+Format: "",
+Enum: []interface{}{"a", "b"},
+},
+},
+},
+Required: []string{"Value"},
+},
+},
+}
+}
+
+`, funcBuffer.String())
+
+}
+
 func TestEnum(t *testing.T) {
 	callErr, funcErr, assert, _, funcBuffer, _ := testOpenAPITypeWriter(t, `
 package foo
@@ -2187,6 +2261,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

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 := isOptional(&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 := isOptional(&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)