Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generators: Default is a valid enum value #407

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/generators/enum.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,17 @@ func (et *enumType) ValueStrings() []string {
return values
}

// IsValue returns true if the given value is one of the possible enum
// values.
func (et *enumType) IsValid(val string) bool {
for _, value := range et.Values {
if val == fmt.Sprintf("%q", value.Value) {
return true
}
}
return false
}

// DescriptionLines returns a description of the enum in this format:
//
// Possible enum values:
Expand Down
34 changes: 25 additions & 9 deletions pkg/generators/openapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -585,26 +585,27 @@ func mustEnforceDefault(t *types.Type, omitEmpty bool) (interface{}, error) {
}
}

func (g openAPITypeWriter) generateDefault(comments []string, t *types.Type, omitEmpty bool) error {
func (g openAPITypeWriter) generateDefault(comments []string, t *types.Type, omitEmpty bool) (*string, error) {
t = resolveAliasAndEmbeddedType(t)
def, err := defaultFromComments(comments)
if err != nil {
return err
return nil, err
}
if enforced, err := mustEnforceDefault(t, omitEmpty); err != nil {
return err
return nil, err
} else if enforced != nil {
if def == nil {
def = enforced
} else if !reflect.DeepEqual(def, enforced) {
enforcedJson, _ := json.Marshal(enforced)
return fmt.Errorf("invalid default value (%#v) for non-pointer/non-omitempty. If specified, must be: %v", def, string(enforcedJson))
return nil, fmt.Errorf("invalid default value (%#v) for non-pointer/non-omitempty. If specified, must be: %v", def, string(enforcedJson))
}
}
if def != nil {
g.Do("Default: $.$,\n", fmt.Sprintf("%#v", def))
val := fmt.Sprintf("%#v", def)
return &val, nil
}
return nil
return nil, nil
}

func (g openAPITypeWriter) generateDescription(CommentLines []string) {
Expand Down Expand Up @@ -676,9 +677,16 @@ func (g openAPITypeWriter) generateProperty(m *types.Member, parent *types.Type)
return nil
}
omitEmpty := strings.Contains(reflect.StructTag(m.Tags).Get("json"), "omitempty")
if err := g.generateDefault(m.CommentLines, m.Type, omitEmpty); err != nil {
def, err := g.generateDefault(m.CommentLines, m.Type, omitEmpty)
if err != nil {
return fmt.Errorf("failed to generate default in %v: %v: %v", parent, m.Name, err)
}
if def != nil {
g.Do("Default: $.$,\n", *def)
if enumType, isEnum := g.enumContext.EnumType(m.Type); isEnum && !enumType.IsValid(*def) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this check be part of generateDefault implementation instead? Just wondering since it is called from multiple places and might be cleaner that way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would mean we would have to push even more parameters to generateDefault, I don't have a strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't see IsValid being called in other places?

return fmt.Errorf("Default value is not a valid enum value: %v not in %v", *def, enumType.ValueStrings())
}
}
t := resolveAliasAndPtrType(m.Type)
// If we can get a openAPI type and format for this type, we consider it to be simple property
typeString, format := openapi.OpenAPITypeFormat(t.String())
Expand Down Expand Up @@ -762,9 +770,13 @@ func (g openAPITypeWriter) generateMapProperty(t *types.Type) error {

g.Do("Type: []string{\"object\"},\n", nil)
g.Do("AdditionalProperties: &spec.SchemaOrBool{\nAllows: true,\nSchema: &spec.Schema{\nSchemaProps: spec.SchemaProps{\n", nil)
if err := g.generateDefault(t.Elem.CommentLines, t.Elem, false); err != nil {
def, err := g.generateDefault(t.Elem.CommentLines, t.Elem, false)
if err != nil {
return err
}
if def != nil {
g.Do("Default: $.$,\n", *def)
}
typeString, format := openapi.OpenAPITypeFormat(elemType.String())
if typeString != "" {
g.generateSimpleProperty(typeString, format)
Expand Down Expand Up @@ -795,9 +807,13 @@ func (g openAPITypeWriter) generateSliceProperty(t *types.Type) error {
elemType := resolveAliasAndPtrType(t.Elem)
g.Do("Type: []string{\"array\"},\n", nil)
g.Do("Items: &spec.SchemaOrArray{\nSchema: &spec.Schema{\nSchemaProps: spec.SchemaProps{\n", nil)
if err := g.generateDefault(t.Elem.CommentLines, t.Elem, false); err != nil {
def, err := g.generateDefault(t.Elem.CommentLines, t.Elem, false)
if err != nil {
return err
}
if def != nil {
g.Do("Default: $.$,\n", *def)
}
typeString, format := openapi.OpenAPITypeFormat(elemType.String())
if typeString != "" {
g.generateSimpleProperty(typeString, format)
Expand Down
29 changes: 10 additions & 19 deletions pkg/generators/openapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1632,10 +1632,10 @@ const EnumB EnumType = "b"
// +k8s:openapi-gen=true
// +k8s:openapi-gen=x-kubernetes-type-tag:type_test
type Blah struct {
// Value is the value.
Value EnumType
NoCommentEnum EnumType
// +optional
// Value is the value.
// +default="b"
Value *EnumType
// +optional
OptionalEnum *EnumType
}`)
if callErr != nil {
Expand All @@ -1653,33 +1653,24 @@ 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"},
},
},
"NoCommentEnum": {
SchemaProps: spec.SchemaProps{`+"\n"+
"Description: \"Possible enum values:\\n - `\\\"a\\\"` is a.\\n - `\\\"b\\\"` is b.\","+`
Default: "",
SchemaProps: spec.SchemaProps{
Description: "Value is the value.\n\nPossible enum values:\n - `+"`"+`\"a\"`+"`"+` is a.\n - `+"`"+`\"b\"`+"`"+` is b.",
Default: "b",
Type: []string{"string"},
Format: "",
Enum: []interface{}{"a", "b"},
},
},
"OptionalEnum": {
SchemaProps: spec.SchemaProps{`+"\n"+
"Description: \"Possible enum values:\\n - `\\\"a\\\"` is a.\\n - `\\\"b\\\"` is b.\","+`
SchemaProps: spec.SchemaProps{
Description: "Possible enum values:\n - `+"`"+`\"a\"`+"`"+` is a.\n - `+"`"+`\"b\"`+"`"+` is b.",
Type: []string{"string"},
Format: "",
Enum: []interface{}{"a", "b"},
},
},
},
Required: []string{"Value","NoCommentEnum"},
Required: []string{"Value"},
},
VendorExtensible: spec.VendorExtensible{
Extensions: spec.Extensions{
Expand Down
6 changes: 3 additions & 3 deletions test/integration/pkg/generated/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion test/integration/testdata/enumtype/enum.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ const FruitRiceBall FruitType = "onigiri"
// FruitsBasket is the type that contains the enum type.
// +k8s:openapi-gen=true
type FruitsBasket struct {
Content FruitType `json:"content"`
// +optional
Content FruitType `json:"content,omitempty"`

// +default=0
Count int `json:"count"`
Expand Down