Skip to content
Merged
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
184 changes: 71 additions & 113 deletions internal/generate/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -704,149 +704,107 @@ func createAllOf(s *openapi3.Schema, stringEnums map[string][]string, name, type
}

func createOneOf(s *openapi3.Schema, name, typeName string) ([]TypeTemplate, []EnumTemplate) {
var parsedProperties []string
var properties []string
var genericTypes []string
enumTpls := make([]EnumTemplate, 0)
typeTpls := make([]TypeTemplate, 0)
fields := make([]TypeField, 0)
for _, v := range s.OneOf {
// Iterate over all the schema components in the spec and write the types.
keys := sortedKeys(v.Value.Properties)

for _, prop := range keys {
p := v.Value.Properties[prop]
// We want to collect all the unique properties to create our global oneOf type.
propertyType := convertToValidGoType(prop, typeName, p)
properties = append(properties, prop+"="+propertyType)
}
}

// When dealing with oneOf sometimes property types will not be the same, we want to
// catch these to set them as "any" when we generate the type.
typeKeys := []string{}
// First we gather all unique properties
for _, v := range properties {
parts := strings.Split(v, "=")
key := parts[0]
if !slices.Contains(typeKeys, key) {
typeKeys = append(typeKeys, key)
// Loop over variants, creating types and enums for nested types, and gathering metadata about the oneOf overall.

// Set of candidate discriminator keys. There must be exactly zero or one discriminator key.
discriminatorKeys := map[string]struct{}{}
// Map of properties to sets of variant types. We use this to identify fields with multiple types across variants.
propToVariantTypes := map[string]map[string]struct{}{}

for _, variantRef := range s.OneOf {
enumField := ""
for _, propName := range sortedKeys(variantRef.Value.Properties) {
propRef := variantRef.Value.Properties[propName]
propField := strcase.ToCamel(propName)

if len(propRef.Value.Enum) == 1 {
discriminatorKeys[propName] = struct{}{}
enumField = strcase.ToCamel(propRef.Value.Enum[0].(string))
} else if len(propRef.Value.Enum) > 1 {
fmt.Printf("[WARN] TODO: oneOf for %q -> %q enum %#v\n", name, propName, propRef.Value.Enum)
Comment on lines +726 to +727
Copy link
Member

Choose a reason for hiding this comment

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

I squinted at this for a while and I think there's a potential bug if a property has enum with more than one value, but I'm not sure if a spec like this would even make sense. I think, in general, you would create more variants?

The bug I saw by playing around with the tests is that convertToValidGoType uses propName for the struct name when enumField is empty, and so we end up with two structs with the same name (ImageSource in this case):

@@ -331,8 +332,9 @@ func Test_createOneOf(t *testing.T) {
                                                Value: &openapi3.Schema{
                                                        Type: &openapi3.Types{"object"},
                                                        Properties: map[string]*openapi3.SchemaRef{
-                                                               "type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url"}}},
+                                                               "type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url", "abc"}}},
                                                                "url":  {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
+                                                               "abc":  {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
                                                        },
                                                        Required: []string{"type", "url"},
// ImageSourceType is the type definition for a ImageSourceType.
type ImageSourceType string
// ImageSource is the type definition for a ImageSource.
//
// Required fields:
// - Type
// - Url
type ImageSource struct {
        Abc string `json:"abc,omitempty" yaml:"abc,omitempty"`
        Type ImageSourceType `json:"type" yaml:"type"`
        Url string `json:"url" yaml:"url"`
}

// ImageSourceSnapshot is the type definition for a ImageSourceSnapshot.
//
// Required fields:
// - Id
// - Type
type ImageSourceSnapshot struct {
        Id string `json:"id" yaml:"id"`
        Type ImageSourceType `json:"type" yaml:"type"`
}

// ImageSource is the source of the underlying image.
type ImageSource struct {
        // Abc is the type definition for a Abc.
        Abc string `json:"abc,omitempty" yaml:"abc,omitempty"`
        // Type is the type definition for a Type.
        Type ImageSourceType `json:"type,omitempty" yaml:"type,omitempty"`
        // Url is the type definition for a Url.
        Url string `json:"url,omitempty" yaml:"url,omitempty"`
        // Id is the type definition for a Id.
        Id string `json:"id,omitempty" yaml:"id,omitempty"`
}

// ImageSourceTypeUrl represents the ImageSourceType `"url"`.
const ImageSourceTypeUrl ImageSourceType = "url"

// ImageSourceTypeAbc represents the ImageSourceType `"abc"`.
const ImageSourceTypeAbc ImageSourceType = "abc"

// ImageSourceTypeSnapshot represents the ImageSourceType `"snapshot"`.
const ImageSourceTypeSnapshot ImageSourceType = "snapshot"

But again, I think this is an unrealistic scenario, so probably safe to ignore.

The other thing I noticed is that this path is currently triggered once:

[WARN] TODO: oneOf for "DiskSource" -> "block_size" enum []interface {}{512, 2048, 4096}

But that sounds like a false positive? The generated code looks correct. This is just a bit of noise, so I think safe to ignore if it's hard to avoid it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was also looking at duplicate struct generation when a pathological OpenAPI specification is created with oneOf variants. I updated the AddressLotKind scheme like so.

{
  "description": "The kind associated with an address lot.",
  "oneOf": [
    {
      "description": "Infrastructure address lots are used for network infrastructure like addresses assigned to rack switches.",
      "type": "string",
      "enum": [
        "infra"
      ]
    },
    {
      "description": "Pool address lots are used by IP pools.",
      "type": "string",
      "enum": [
        "pool"
      ]
    },
    {
      "description": "Testing things.",
      "type": "object",
      "properties": {
        "type": {
          "type": "string",
          "enum": [
            "foo"
          ]
        },
        "foo": {
          "type": "string"
        }
      },
      "required": [
        "allow",
        "type"
      ]
    }
  ]
}

Both the changes on this pull request and the current main changes generated the following duplicate types.

type AddressLotKind string
type AddressLotKindFoo struct {
    // ...
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other thing I noticed is that this path is currently triggered once:

[WARN] TODO: oneOf for "DiskSource" -> "block_size" enum []interface {}{512, 2048, 4096}

But that sounds like a false positive? The generated code looks correct. This is just a bit of noise, so I think safe to ignore if it's hard to avoid it.

This was an interesting one since it only occurs for this variant of DiskSource even though another variant uses block_size as well.

          {
            "description": "Create a blank disk that will accept bulk writes or pull blocks from an external source.",
            "type": "object",
            "properties": {
              "block_size": {
                "$ref": "#/components/schemas/BlockSize"
              },
              "type": {
                "type": "string",
                "enum": [
                  "importing_blocks"
                ]
              }
            },
            "required": [
              "block_size",
              "type"
            ]
          }

It's because the block_size here directly references the BlockSize schema whereas the other variant uses block_size behind an allOf, like so.

              "block_size": {
                "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096",
                "allOf": [
                  {
                    "$ref": "#/components/schemas/BlockSize"
                  }
                ]
              },

} else if propRef.Value.Enum == nil && len(variantRef.Value.Properties) == 1 {
enumField = propField
}
if _, ok := propToVariantTypes[propName]; !ok {
propToVariantTypes[propName] = map[string]struct{}{}
}
goType := convertToValidGoType(propName, typeName, propRef)
propToVariantTypes[propName][goType] = struct{}{}
}
tt, et := populateTypeTemplates(name, variantRef.Value, enumField)
typeTpls = append(typeTpls, tt...)
enumTpls = append(enumTpls, et...)
}

// For each of the properties above we gather all possible types
// and gather all of those that are not. We will be setting those
// as a generic type
for _, k := range typeKeys {
values := []string{}
for _, v := range properties {
parts := strings.Split(v, "=")
key := parts[0]
value := parts[1]
if key == k {
values = append(values, value)
}
}
// Check invariant: there must be exactly zero or one discriminator field.
if len(discriminatorKeys) > 1 {
panic(fmt.Sprintf("[ERROR] Found multiple discriminator properties for type %s: %+v", name, discriminatorKeys))
}

if !allItemsAreSame(values) {
genericTypes = append(genericTypes, k)
// Find properties that have different types across variants.
multiTypeProps := map[string]struct{}{}
for propName, variantTypes := range propToVariantTypes {
if len(variantTypes) > 1 {
multiTypeProps[propName] = struct{}{}
}
}

for _, v := range s.OneOf {
// We want to iterate over the properties of the embedded object
// and find the type that is a string.
var enumFieldName string

// Iterate over all the schema components in the spec and write the types.
keys := sortedKeys(v.Value.Properties)
for _, prop := range keys {
p := v.Value.Properties[prop]
// We want to collect all the unique properties to create our global oneOf type.
propertyType := convertToValidGoType(prop, typeName, p)

// Check if we have an enum in order to use the corresponding type instead of
// "string"
if propertyType == "string" && len(p.Value.Enum) != 0 {
propertyType = typeName + strcase.ToCamel(prop)
// Build the struct type for the oneOf field, if defined.
oneOfFields := []TypeField{}
seenFields := map[string]struct{}{}
for _, variantRef := range s.OneOf {
for _, propName := range sortedKeys(variantRef.Value.Properties) {
if _, ok := seenFields[propName]; ok {
continue
}
seenFields[propName] = struct{}{}

propertyName := strcase.ToCamel(prop)

// Avoids duplication for every enum
if !containsMatchFirstWord(parsedProperties, propertyName) {
// We set the type of a field as "any" if every element of the oneOf property isn't the same
if slices.Contains(genericTypes, prop) {
propertyType = "any"
}

// Determine omit directive: nullable fields in oneOf use omitzero.
var omitDirective string
if p.Value != nil && p.Value.Nullable {
omitDirective = "omitzero"
}

field := TypeField{
Name: propertyName,
Type: propertyType,
MarshalKey: prop,
Schema: p,
FallbackDescription: true,
OmitDirective: omitDirective,
}
propRef := variantRef.Value.Properties[propName]
propField := strcase.ToCamel(propName)
propType := convertToValidGoType(propName, typeName, propRef)

fields = append(fields, field)

parsedProperties = append(parsedProperties, propertyName)
// Use the enum type name instead of "string" when the property has an enum.
if propType == "string" && len(propRef.Value.Enum) != 0 {
propType = typeName + strcase.ToCamel(propName)
}

if p.Value.Enum != nil {
// We want to get the enum value.
// Make sure there is only one.
if len(p.Value.Enum) != 1 {
fmt.Printf("[WARN] TODO: oneOf for %q -> %q enum %#v\n", name, prop, p.Value.Enum)
continue
}

enumFieldName = strcase.ToCamel(p.Value.Enum[0].(string))
// Use "any" if this property has different types across variants.
if _, ok := multiTypeProps[propName]; ok {
propType = "any"
}

// Enums can appear in a valid OpenAPI spec as a OneOf without necessarily
// being identified as such. If we find an object with a single property
// nested inside a OneOf we will assume this is an enum and modify the name of
// the struct that will be created out of this object.
// e.g. https://github.com/oxidecomputer/omicron/blob/158c0b205f23772dc6c4c97633fd1769cc0e00d4/openapi/nexus.json#L18637-L18682
if len(keys) == 1 && p.Value.Enum == nil {
enumFieldName = propertyName
// Determine omit directive: nullable fields in oneOf use omitzero.
var omitDirective string
if propRef.Value != nil && propRef.Value.Nullable {
omitDirective = "omitzero"
}
}

// TODO: This is the only place that has an "additional name" at the end
// TODO: This is where the "allOf" is being detected
tt, et := populateTypeTemplates(name, v.Value, enumFieldName)
typeTpls = append(typeTpls, tt...)
enumTpls = append(enumTpls, et...)
}
field := TypeField{
Name: propField,
Type: propType,
MarshalKey: propName,
Schema: propRef,
FallbackDescription: true,
OmitDirective: omitDirective,
}

// TODO: For now AllOf values within a OneOf are treated as enums
// because that's how they are being used. Keep an eye out if this
// changes
for _, v := range s.OneOf {
if v.Value.AllOf != nil {
return typeTpls, enumTpls
oneOfFields = append(oneOfFields, field)
}
}

// Make sure to only create structs if the oneOf is not a replacement for enums on the API spec
if len(fields) > 0 {
if len(oneOfFields) > 0 {
typeTpl := TypeTemplate{
Description: formatTypeDescription(typeName, s),
Name: typeName,
Type: "struct",
Fields: fields,
Fields: oneOfFields,
}
typeTpls = append(typeTpls, typeTpl)
}

return typeTpls, enumTpls
}

Expand Down
150 changes: 122 additions & 28 deletions internal/generate/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -315,32 +315,6 @@ func Test_createStringEnum(t *testing.T) {
}

func Test_createOneOf(t *testing.T) {
schema := &openapi3.Schema{
Description: "The source of the underlying image.",
OneOf: openapi3.SchemaRefs{
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url"}}},
"url": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
},
Required: []string{"type", "url"},
},
},
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"id": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Format: "uuid"}},
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"snapshot"}}},
},
Required: []string{"id", "type"},
},
},
},
}

tests := []struct {
name string
schema *openapi3.Schema
Expand All @@ -349,8 +323,32 @@ func Test_createOneOf(t *testing.T) {
wantEnums []EnumTemplate
}{
{
name: "all variants of same type",
schema: schema,
name: "all variants of same type",
schema: &openapi3.Schema{
Description: "The source of the underlying image.",
OneOf: openapi3.SchemaRefs{
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url"}}},
"url": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
},
Required: []string{"type", "url"},
},
},
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"id": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Format: "uuid"}},
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"snapshot"}}},
},
Required: []string{"id", "type"},
},
},
},
},
typeName: "ImageSource",
wantTypes: []TypeTemplate{
{Description: "// ImageSourceType is the type definition for a ImageSourceType.", Name: "ImageSourceType", Type: "string"},
Expand Down Expand Up @@ -388,6 +386,69 @@ func Test_createOneOf(t *testing.T) {
{Description: "// ImageSourceTypeSnapshot represents the ImageSourceType `\"snapshot\"`.", Name: "ImageSourceTypeSnapshot", ValueType: "const", Value: "ImageSourceType = \"snapshot\""},
},
},
{
name: "variants with different value types",
schema: &openapi3.Schema{
Description: "A value that can be an int or a string.",
OneOf: openapi3.SchemaRefs{
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"int"}}},
"value": {Value: &openapi3.Schema{Type: &openapi3.Types{"integer"}}},
},
Required: []string{"type", "value"},
},
},
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"string"}}},
"value": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
},
Required: []string{"type", "value"},
},
},
},
},
typeName: "IntOrString",
wantTypes: []TypeTemplate{
{Description: "// IntOrStringType is the type definition for a IntOrStringType.", Name: "IntOrStringType", Type: "string"},
{
Description: "// IntOrStringInt is the type definition for a IntOrStringInt.\n//\n// Required fields:\n// - Type\n// - Value",
Name: "IntOrStringInt",
Type: "struct",
Fields: []TypeField{
{Name: "Type", Type: "IntOrStringType", MarshalKey: "type", Required: true},
{Name: "Value", Type: "*int", MarshalKey: "value", Required: true},
},
},
{
Description: "// IntOrStringString is the type definition for a IntOrStringString.\n//\n// Required fields:\n// - Type\n// - Value",
Name: "IntOrStringString",
Type: "struct",
Fields: []TypeField{
{Name: "Type", Type: "IntOrStringType", MarshalKey: "type", Required: true},
{Name: "Value", Type: "string", MarshalKey: "value", Required: true},
},
},
{
Description: "// IntOrString is a value that can be an int or a string.",
Name: "IntOrString",
Type: "struct",
Fields: []TypeField{
{Name: "Type", Type: "IntOrStringType", MarshalKey: "type", FallbackDescription: true},
{Name: "Value", Type: "any", MarshalKey: "value", FallbackDescription: true},
},
},
},
wantEnums: []EnumTemplate{
{Description: "// IntOrStringTypeInt represents the IntOrStringType `\"int\"`.", Name: "IntOrStringTypeInt", ValueType: "const", Value: "IntOrStringType = \"int\""},
{Description: "// IntOrStringTypeString represents the IntOrStringType `\"string\"`.", Name: "IntOrStringTypeString", ValueType: "const", Value: "IntOrStringType = \"string\""},
},
},
}

for _, tc := range tests {
Expand All @@ -400,6 +461,39 @@ func Test_createOneOf(t *testing.T) {
assert.Equal(t, tc.wantEnums, gotEnums)
})
}

t.Run("multiple discriminator keys panics", func(t *testing.T) {
schema := &openapi3.Schema{
Description: "Schema with multiple discriminator keys.",
OneOf: openapi3.SchemaRefs{
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"a"}}},
"kind": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"x"}}},
"value": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
},
},
},
&openapi3.SchemaRef{
Value: &openapi3.Schema{
Type: &openapi3.Types{"object"},
Properties: map[string]*openapi3.SchemaRef{
"type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"b"}}},
"kind": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"y"}}},
"value": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}},
},
},
},
},
}

assert.PanicsWithValue(t,
"[ERROR] Found multiple discriminator properties for type MultiDiscriminator: map[kind:{} type:{}]",
func() { createOneOf(schema, "MultiDiscriminator", "MultiDiscriminator") },
)
})
}

func Test_createAllOf(t *testing.T) {
Expand Down
Loading