From 235b20f2180c3a4fb1e1d0aee9b1e3bd1ac83d2c Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 2 Dec 2025 16:07:23 -0500 Subject: [PATCH 1/3] Refactor: consolidate field logic in struct. Currently, we define field descriptions and serialization behavior in several places, with repetitive but also sometime different logic. To make the field generation logic easier to understand and modify, this patch consolidates field descriptions and serialization logic into the TypeFields struct, and derives them from the schema dynamically. We also add tests for these new methods. --- go.mod | 1 + go.sum | 2 + internal/generate/templates/type.go.tpl | 4 +- internal/generate/types.go | 147 ++++++--- internal/generate/types_test.go | 411 +++++++++--------------- 5 files changed, 257 insertions(+), 308 deletions(-) diff --git a/go.mod b/go.mod index cf13c25..6455e9f 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.24.3 require ( github.com/getkin/kin-openapi v0.133.0 + github.com/google/go-cmp v0.7.0 github.com/iancoleman/strcase v0.3.0 github.com/pelletier/go-toml v1.9.5 github.com/stretchr/testify v1.11.1 diff --git a/go.sum b/go.sum index 121db86..ba1d78c 100644 --- a/go.sum +++ b/go.sum @@ -8,6 +8,8 @@ github.com/go-openapi/swag v0.23.0 h1:vsEVJDUo2hPJ2tu0/Xc+4noaxyEffXNIs3cOULZ+Gr github.com/go-openapi/swag v0.23.0/go.mod h1:esZ8ITTYEsH1V2trKHjAN8Ai7xHb8RV+YSZ577vPjgQ= github.com/go-test/deep v1.0.8 h1:TDsG77qcSprGbC6vTN8OuXp5g+J+b5Pcguhf7Zt61VM= github.com/go-test/deep v1.0.8/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/iancoleman/strcase v0.3.0 h1:nTXanmYxhfFAMjZL34Ov6gkzEsSJZ5DbhxWjvSASxEI= github.com/iancoleman/strcase v0.3.0/go.mod h1:iwCmte+B7n89clKwxIoIXy/HfoL7AsD47ZCWhYzw7ho= github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY= diff --git a/internal/generate/templates/type.go.tpl b/internal/generate/templates/type.go.tpl index 4b3e104..6a42aa3 100644 --- a/internal/generate/templates/type.go.tpl +++ b/internal/generate/templates/type.go.tpl @@ -3,9 +3,9 @@ type {{.Name}} {{.Type}} { {{- range .Fields}} {{- if .Description}} - {{splitDocString .Description}} + {{.Description}} {{- end}} - {{.Name}} {{.Type}} {{.SerializationInfo}} + {{.Name}} {{.Type}} {{.StructTag}} {{- end}} } diff --git a/internal/generate/types.go b/internal/generate/types.go index e620fcb..9d1415b 100644 --- a/internal/generate/types.go +++ b/internal/generate/types.go @@ -22,6 +22,10 @@ import ( // are not duplicated in createStringEnum() var collectEnumStringTypes = enumStringTypes() +func enumStringTypes() map[string][]string { + return map[string][]string{} +} + var ( typeTemplate = template.Must( template.New("type.go.tpl"). @@ -38,10 +42,6 @@ var ( ) ) -func enumStringTypes() map[string][]string { - return map[string][]string{} -} - // renderTemplate executes a template with the given data and returns the result. func renderTemplate(tmpl *template.Template, data any) string { var buf bytes.Buffer @@ -70,10 +70,65 @@ func (t TypeTemplate) Render() string { // TypeField holds the information for each type field. type TypeField struct { - Description string - Name string - Type string - SerializationInfo string + Schema *openapi3.SchemaRef + Name string + Type string + MarshalKey string + Required bool + + // FallbackDescription generates a generic description for the field when the Schema doesn't have one. + // TODO: Drop this, since generated descriptions don't contain useful information. + FallbackDescription bool + + // OmitDirective overrides the derived omit directive for the field. + // TODO: Drop this; we should set omit directives consistently rather than using overrides. + OmitDirective string +} + +// Description returns the formatted description comment for this field. +func (f TypeField) Description() string { + if f.Schema == nil { + return "" + } + if f.Schema.Value.Description != "" { + desc := fmt.Sprintf("// %s is %s", f.Name, toLowerFirstLetter( + strings.ReplaceAll(f.Schema.Value.Description, "\n", "\n// "))) + return splitDocString(desc) + } + if f.FallbackDescription { + return splitDocString(fmt.Sprintf("// %s is the type definition for a %s.", f.Name, f.Name)) + } + return "" +} + +// StructTag returns the JSON/YAML struct tags for this field. +func (f TypeField) StructTag() string { + // Derive the omit directive. + var omitDirective string + switch { + case f.OmitDirective != "": + // Explicit override. + omitDirective = f.OmitDirective + case f.Schema == nil: + // Special case: no schema (e.g., Body field). + omitDirective = "omitempty" + case f.Required || isNullableArray(f.Schema): + // Required or nullable array: no directive (always serialize). + omitDirective = "" + case slices.Contains(omitzeroTypes(), f.Type): + // Special types: omitzero. + omitDirective = "omitzero" + default: + omitDirective = "omitempty" + } + + // Build the tag value. + tagValue := f.MarshalKey + if omitDirective != "" { + tagValue = f.MarshalKey + "," + omitDirective + } + + return fmt.Sprintf("`json:\"%s\" yaml:\"%s\"`", tagValue, tagValue) } // EnumTemplate holds the information for enum types. @@ -171,18 +226,18 @@ func constructParamTypes(paths map[string]*openapi3.PathItem) []TypeTemplate { } paramName := strcase.ToCamel(p.Value.Name) + paramType := convertToValidGoType("", "", p.Value.Schema) field := TypeField{ - Name: paramName, - Type: convertToValidGoType("", "", p.Value.Schema), + Name: paramName, + Type: paramType, + MarshalKey: p.Value.Name, + Schema: nil, // nil so StructTag always uses omitempty } if p.Value.Required { requiredFields = requiredFields + fmt.Sprintf("\n// - %s", paramName) } - serInfo := fmt.Sprintf("`json:\"%s,omitempty\" yaml:\"%s,omitempty\"`", p.Value.Name, p.Value.Name) - field.SerializationInfo = serInfo - fields = append(fields, field) } if o.RequestBody != nil { @@ -193,17 +248,19 @@ func constructParamTypes(paths map[string]*openapi3.PathItem) []TypeTemplate { // TODO: Handle other mime types in a more idiomatic way if mt != "application/json" { field = TypeField{ - Name: "Body", - Type: "io.Reader", - SerializationInfo: "`json:\"body,omitempty\" yaml:\"body,omitempty\"`", + Name: "Body", + Type: "io.Reader", + MarshalKey: "body", + Schema: nil, // no schema for non-JSON body } break } field = TypeField{ - Name: "Body", - Type: "*" + convertToValidGoType("", "", r.Schema), - SerializationInfo: "`json:\"body,omitempty\" yaml:\"body,omitempty\"`", + Name: "Body", + Type: "*" + convertToValidGoType("", "", r.Schema), + MarshalKey: "body", + Schema: nil, // Body uses special serialization } } // Body is always a required field @@ -355,7 +412,7 @@ func constructEnums(enumStrCollection map[string][]string) []EnumTemplate { return enumCollection } -// writeTypes iterates over the templates, constructs the different types and writes to file +// writeTypes iterates over the templates, constructs the different types and writes to file. func writeTypes(f *os.File, typeCollection []TypeTemplate, typeValidationCollection []ValidationTemplate, enumCollection []EnumTemplate) { for _, tt := range typeCollection { fmt.Fprint(f, tt.Render()) @@ -521,29 +578,14 @@ func createTypeObject(schema *openapi3.Schema, name, typeName, description strin } } - field := TypeField{} - if v.Value.Description != "" { - desc := fmt.Sprintf("// %s is %s", strcase.ToCamel(k), toLowerFirstLetter(strings.ReplaceAll(v.Value.Description, "\n", "\n// "))) - field.Description = desc - } - - field.Name = strcase.ToCamel(k) - field.Type = typeName - - // Configure json/yaml struct tags. By default, omit empty/zero - // values, but retain them for required fields. - // - // TODO: Use `omitzero` rather than `omitempty` on all relevant - // fields: https://github.com/oxidecomputer/oxide.go/issues/290 - serInfo := fmt.Sprintf("`json:\"%s,omitempty\" yaml:\"%s,omitempty\"`", k, k) - if isNullableArray(v) || isRequired { - serInfo = fmt.Sprintf("`json:\"%s\" yaml:\"%s\"`", k, k) - } else if slices.Contains(omitzeroTypes(), typeName) { - serInfo = fmt.Sprintf("`json:\"%s,omitzero\" yaml:\"%s,omitzero\"`", k, k) + field := TypeField{ + Name: strcase.ToCamel(k), + Type: typeName, + MarshalKey: k, + Schema: v, + Required: isRequired, } - field.SerializationInfo = serInfo - fields = append(fields, field) } @@ -712,21 +754,24 @@ func createOneOf(s *openapi3.Schema, name, typeName string) ([]TypeTemplate, []E // Avoids duplication for every enum if !containsMatchFirstWord(parsedProperties, propertyName) { - field := TypeField{ - Description: formatTypeDescription(propertyName, p.Value), - Name: propertyName, - Type: propertyType, - SerializationInfo: fmt.Sprintf("`json:\"%s,omitempty\" yaml:\"%s,omitempty\"`", prop, prop), - } - // 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) { - field.Type = "any" + propertyType = "any" } - // Check if the field is nullable and use omitzero instead of omitempty. + // Determine omit directive: nullable fields in oneOf use omitzero. + var omitDirective string if p.Value != nil && p.Value.Nullable { - field.SerializationInfo = fmt.Sprintf("`json:\"%s,omitzero\" yaml:\"%s,omitzero\"`", prop, prop) + omitDirective = "omitzero" + } + + field := TypeField{ + Name: propertyName, + Type: propertyType, + MarshalKey: prop, + Schema: p, + FallbackDescription: true, + OmitDirective: omitDirective, } fields = append(fields, field) diff --git a/internal/generate/types_test.go b/internal/generate/types_test.go index a364f42..07701fe 100644 --- a/internal/generate/types_test.go +++ b/internal/generate/types_test.go @@ -5,13 +5,17 @@ package main import ( - "fmt" "testing" "github.com/getkin/kin-openapi/openapi3" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" ) +// cmpIgnoreSchema is a go-cmp option that ignores the Schema field when comparing TypeField. +var cmpIgnoreSchema = cmpopts.IgnoreFields(TypeField{}, "Schema") + func Test_generateTypes(t *testing.T) { typesSpec := &openapi3.T{ Components: &openapi3.Components{ @@ -103,6 +107,87 @@ func Test_generateTypes(t *testing.T) { } } +func TestTypeField_Description(t *testing.T) { + t.Run("nil schema returns empty", func(t *testing.T) { + f := TypeField{Name: "Foo", Schema: nil} + assert.Equal(t, "", f.Description()) + }) + + t.Run("empty", func(t *testing.T) { + f := TypeField{ + Name: "Foo", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Description: ""}}, + } + assert.Equal(t, "", f.Description()) + }) + + t.Run("not empty", func(t *testing.T) { + f := TypeField{ + Name: "Foo", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Description: "A foo field"}}, + } + assert.Equal(t, "// Foo is a foo field", f.Description()) + }) +} + +func TestTypeField_StructTag(t *testing.T) { + t.Run("nil schema", func(t *testing.T) { + f := TypeField{Name: "Body", MarshalKey: "body", Schema: nil} + assert.Equal(t, "`json:\"body,omitempty\" yaml:\"body,omitempty\"`", f.StructTag()) + }) + + t.Run("required", func(t *testing.T) { + f := TypeField{ + Name: "Id", + MarshalKey: "id", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}}, + Required: true, + } + assert.Equal(t, "`json:\"id\" yaml:\"id\"`", f.StructTag()) + }) + + t.Run("nullable array", func(t *testing.T) { + f := TypeField{ + Name: "Items", + MarshalKey: "items", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"array"}, Nullable: true}}, + Required: false, + } + assert.Equal(t, "`json:\"items\" yaml:\"items\"`", f.StructTag()) + }) + + t.Run("nullable", func(t *testing.T) { + f := TypeField{ + Name: "Value", + MarshalKey: "value", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Nullable: true}}, + Required: false, + } + assert.Equal(t, "`json:\"value,omitempty\" yaml:\"value,omitempty\"`", f.StructTag()) + }) + + t.Run("omitdirective", func(t *testing.T) { + f := TypeField{ + Name: "Value", + MarshalKey: "value", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}}, + OmitDirective: "omitzero", + } + assert.Equal(t, "`json:\"value,omitzero\" yaml:\"value,omitzero\"`", f.StructTag()) + }) + + t.Run("default", func(t *testing.T) { + f := TypeField{ + Name: "Count", + Type: "int", + MarshalKey: "count", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"integer"}}}, + Required: false, + } + assert.Equal(t, "`json:\"count,omitempty\" yaml:\"count,omitempty\"`", f.StructTag()) + }) +} + func Test_createTypeObject(t *testing.T) { typesSpec := openapi3.Schema{ Required: []string{"type"}, @@ -111,49 +196,32 @@ func Test_createTypeObject(t *testing.T) { Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Format: "uuid"}, }, "type": { - Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []interface{}{"snapshot"}}, + Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"snapshot"}}, }, }} - type args struct { - s openapi3.Schema - name string - typeName string - } - tests := []struct { - name string - args args - want TypeTemplate - }{ - { - name: "success", - args: args{typesSpec, "DiskSource", "DiskSourceSnapshot"}, - want: TypeTemplate{ - Description: "Create a disk from a disk snapshot\n//\n// Required fields:\n// - Type", - Name: "DiskSourceSnapshot", - Type: "struct", Fields: []TypeField{ - { - Description: "", - Name: "SnapshotId", - Type: "string", - SerializationInfo: "`json:\"snapshot_id,omitempty\" yaml:\"snapshot_id,omitempty\"`", - }, - { - Description: "", - Name: "Type", - Type: "DiskSourceType", - SerializationInfo: "`json:\"type\" yaml:\"type\"`", - }, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := createTypeObject(&tt.args.s, tt.args.name, tt.args.typeName, "Create a disk from a disk snapshot") - assert.Equal(t, tt.want, got) - }) - } + got := createTypeObject(&typesSpec, "DiskSource", "DiskSourceSnapshot", "Create a disk from a disk snapshot") + + assert.Equal(t, "DiskSourceSnapshot", got.Name) + assert.Equal(t, "struct", got.Type) + assert.Equal(t, "Create a disk from a disk snapshot\n//\n// Required fields:\n// - Type", got.Description) + assert.Len(t, got.Fields, 2) + + // Check first field (snapshot_id) + assert.Equal(t, "SnapshotId", got.Fields[0].Name) + assert.Equal(t, "string", got.Fields[0].Type) + assert.Equal(t, "snapshot_id", got.Fields[0].MarshalKey) + assert.False(t, got.Fields[0].Required) + assert.Equal(t, "", got.Fields[0].Description()) + assert.Equal(t, "`json:\"snapshot_id,omitempty\" yaml:\"snapshot_id,omitempty\"`", got.Fields[0].StructTag()) + + // Check second field (type) + assert.Equal(t, "Type", got.Fields[1].Name) + assert.Equal(t, "DiskSourceType", got.Fields[1].Type) + assert.Equal(t, "type", got.Fields[1].MarshalKey) + assert.True(t, got.Fields[1].Required) + assert.Equal(t, "", got.Fields[1].Description()) + assert.Equal(t, "`json:\"type\" yaml:\"type\"`", got.Fields[1].StructTag()) } func Test_createStringEnum(t *testing.T) { @@ -184,30 +252,26 @@ func Test_createStringEnum(t *testing.T) { }, }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got, got1, got2 := createStringEnum(tc.args.s, tc.args.stringEnums, tc.args.name, tc.args.typeName) - assert.Equal(t, tc.want, got) - assert.Equal(t, tc.want1, got1) - assert.Equal(t, tc.want2, got2) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, got1, got2 := createStringEnum(tt.args.s, tt.args.stringEnums, tt.args.name, tt.args.typeName) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.want1, got1) + assert.Equal(t, tt.want2, got2) }) } } func Test_createOneOf(t *testing.T) { - typeSpec := &openapi3.Schema{ + 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: []interface{}{"url"}}, - }, - "url": { - Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}, - }, + "type": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}, Enum: []any{"url"}}}, + "url": {Value: &openapi3.Schema{Type: &openapi3.Types{"string"}}}, }, Required: []string{"type", "url"}, }, @@ -216,12 +280,8 @@ func Test_createOneOf(t *testing.T) { 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: []interface{}{"snapshot"}}, - }, + "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"}, }, @@ -229,35 +289,26 @@ func Test_createOneOf(t *testing.T) { }, } - type args struct { - s *openapi3.Schema - name string - typeName string - } tests := []struct { - name string - args args - want []TypeTemplate - want1 []EnumTemplate + name string + schema *openapi3.Schema + typeName string + wantTypes []TypeTemplate + wantEnums []EnumTemplate }{ { - name: "success", - args: args{typeSpec, "ImageSource", "ImageSource"}, - want: []TypeTemplate{ - { - Description: "// ImageSourceType is the type definition for a ImageSourceType.", Name: "ImageSourceType", Type: "string", - }, + name: "all variants of same type", + schema: schema, + typeName: "ImageSource", + wantTypes: []TypeTemplate{ + {Description: "// ImageSourceType is the type definition for a ImageSourceType.", Name: "ImageSourceType", Type: "string"}, { Description: "// ImageSourceUrl is the type definition for a ImageSourceUrl.\n//\n// Required fields:\n// - Type\n// - Url", Name: "ImageSourceUrl", Type: "struct", Fields: []TypeField{ - { - Description: "", Name: "Type", Type: "ImageSourceType", SerializationInfo: "`json:\"type\" yaml:\"type\"`", - }, - { - Description: "", Name: "Url", Type: "string", SerializationInfo: "`json:\"url\" yaml:\"url\"`", - }, + {Name: "Type", Type: "ImageSourceType", MarshalKey: "type", Required: true}, + {Name: "Url", Type: "string", MarshalKey: "url", Required: true}, }, }, { @@ -265,39 +316,36 @@ func Test_createOneOf(t *testing.T) { Name: "ImageSourceSnapshot", Type: "struct", Fields: []TypeField{ - { - Description: "", Name: "Id", Type: "string", SerializationInfo: "`json:\"id\" yaml:\"id\"`", - }, - { - Description: "", Name: "Type", Type: "ImageSourceType", SerializationInfo: "`json:\"type\" yaml:\"type\"`", - }, + {Name: "Id", Type: "string", MarshalKey: "id", Required: true}, + {Name: "Type", Type: "ImageSourceType", MarshalKey: "type", Required: true}, }, }, { - Description: "// ImageSource is the source of the underlying image.", Name: "ImageSource", Type: "struct", Fields: []TypeField{ - { - Description: "// Type is the type definition for a Type.", Name: "Type", Type: "ImageSourceType", SerializationInfo: "`json:\"type,omitempty\" yaml:\"type,omitempty\"`", - }, - { - Description: "// Url is the type definition for a Url.", Name: "Url", Type: "string", SerializationInfo: "`json:\"url,omitempty\" yaml:\"url,omitempty\"`", - }, - { - Description: "// Id is the type definition for a Id.", Name: "Id", Type: "string", SerializationInfo: "`json:\"id,omitempty\" yaml:\"id,omitempty\"`", - }, + Description: "// ImageSource is the source of the underlying image.", + Name: "ImageSource", + Type: "struct", + Fields: []TypeField{ + {Name: "Type", Type: "ImageSourceType", MarshalKey: "type", FallbackDescription: true}, + {Name: "Url", Type: "string", MarshalKey: "url", FallbackDescription: true}, + {Name: "Id", Type: "string", MarshalKey: "id", FallbackDescription: true}, }, }, }, - want1: []EnumTemplate{ + wantEnums: []EnumTemplate{ {Description: "// ImageSourceTypeUrl represents the ImageSourceType `\"url\"`.", Name: "ImageSourceTypeUrl", ValueType: "const", Value: "ImageSourceType = \"url\""}, {Description: "// ImageSourceTypeSnapshot represents the ImageSourceType `\"snapshot\"`.", Name: "ImageSourceTypeSnapshot", ValueType: "const", Value: "ImageSourceType = \"snapshot\""}, }, }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got, got1 := createOneOf(tc.args.s, tc.args.name, tc.args.typeName) - assert.Equal(t, tc.want, got) - assert.Equal(t, tc.want1, got1) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotTypes, gotEnums := createOneOf(tt.schema, tt.typeName, tt.typeName) + + if diff := cmp.Diff(tt.wantTypes, gotTypes, cmpIgnoreSchema); diff != "" { + t.Errorf("types mismatch (-want +got):\n%s", diff) + } + assert.Equal(t, tt.wantEnums, gotEnums) }) } } @@ -336,159 +384,12 @@ func Test_createAllOf(t *testing.T) { }, }, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Run(tc.name, func(t *testing.T) { - got := createAllOf(tc.args.s, tc.args.stringEnums, tc.args.name, tc.args.typeName) - assert.Equal(t, tc.want, got) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { + got := createAllOf(tt.args.s, tt.args.stringEnums, tt.args.name, tt.args.typeName) + assert.Equal(t, tt.want, got) }) }) } } - -func Test_ValidationTemplate_Render(t *testing.T) { - tests := []struct { - name string - template ValidationTemplate - want string - }{ - { - name: "with all field types", - template: ValidationTemplate{ - AssociatedType: "CreateUserParams", - RequiredObjects: []string{"Body"}, - RequiredStrings: []string{"Name", "Email"}, - RequiredNums: []string{"Age"}, - }, - want: `// Validate verifies all required fields for CreateUserParams are set -func (p *CreateUserParams) Validate() error { - v := new(Validator) - v.HasRequiredObj(p.Body, "Body") - v.HasRequiredStr(string(p.Name), "Name") - v.HasRequiredStr(string(p.Email), "Email") - v.HasRequiredNum(p.Age, "Age") - if !v.IsValid() { - return fmt.Errorf("validation error:\n%v", v.Error()) - } - return nil -} -`, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := tc.template.Render() - assert.Equal(t, tc.want, got) - }) - } -} - -func Test_EnumTemplate_Render(t *testing.T) { - tests := []struct { - name string - template EnumTemplate - want string - }{ - { - name: "const enum", - template: EnumTemplate{ - Description: "// FleetRoleAdmin represents the FleetRole `\"admin\"`.", - Name: "FleetRoleAdmin", - ValueType: "const", - Value: "FleetRole = \"admin\"", - }, - want: `// FleetRoleAdmin represents the FleetRole ` + "`" + `"admin"` + "`" + `. -const FleetRoleAdmin FleetRole = "admin" - -`, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := tc.template.Render() - assert.Equal(t, tc.want, got) - }) - } -} - -func Test_TypeTemplate_Render(t *testing.T) { - nameTag := "`json:\"name,omitempty\" yaml:\"name,omitempty\"`" - streetTag := "`json:\"street\" yaml:\"street\"`" - cityTag := "`json:\"city\" yaml:\"city\"`" - - tests := []struct { - name string - template TypeTemplate - want string - }{ - { - name: "primitive type without fields", - template: TypeTemplate{ - Description: "// FleetRole is the type definition for a FleetRole.", - Name: "FleetRole", - Type: "string", - }, - want: `// FleetRole is the type definition for a FleetRole. -type FleetRole string -`, - }, - { - name: "struct type with fields", - template: TypeTemplate{ - Description: "// DiskIdentifier is the identifier for a disk.", - Name: "DiskIdentifier", - Type: "struct", - Fields: []TypeField{ - { - Name: "Name", - Type: "string", - SerializationInfo: nameTag, - }, - }, - }, - want: fmt.Sprintf(`// DiskIdentifier is the identifier for a disk. -type DiskIdentifier struct { - Name string %s -} - -`, nameTag), - }, - { - name: "struct type with field descriptions", - template: TypeTemplate{ - Description: "// Address is an address.", - Name: "Address", - Type: "struct", - Fields: []TypeField{ - { - Description: "// Street is the street name", - Name: "Street", - Type: "string", - SerializationInfo: streetTag, - }, - { - Description: "// City is the city name", - Name: "City", - Type: "string", - SerializationInfo: cityTag, - }, - }, - }, - want: fmt.Sprintf(`// Address is an address. -type Address struct { - // Street is the street name - Street string %s - // City is the city name - City string %s -} - -`, streetTag, cityTag), - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := tc.template.Render() - assert.Equal(t, tc.want, got) - }) - } -} From 96f0b5c0b37f3dd877768d7fcaab82afaf9fee86 Mon Sep 17 00:00:00 2001 From: Josh Date: Thu, 4 Dec 2025 12:02:16 -0500 Subject: [PATCH 2/3] Move IsPointer logic to FieldTypes struct. Rather than deciding which generated fields are represented as pointers in different places in the code, this patch adds an IsPointer() method to TypeFields to consolidate that logic. --- internal/generate/templates/type.go.tpl | 2 +- internal/generate/types.go | 63 ++++++++++++++++--------- internal/generate/types_test.go | 60 +++++++++++++++++++++-- internal/generate/utils.go | 18 ++----- 4 files changed, 102 insertions(+), 41 deletions(-) diff --git a/internal/generate/templates/type.go.tpl b/internal/generate/templates/type.go.tpl index 6a42aa3..3a18b2e 100644 --- a/internal/generate/templates/type.go.tpl +++ b/internal/generate/templates/type.go.tpl @@ -5,7 +5,7 @@ type {{.Name}} {{.Type}} { {{- if .Description}} {{.Description}} {{- end}} - {{.Name}} {{.Type}} {{.StructTag}} + {{.Name}} {{.GoType}} {{.StructTag}} {{- end}} } diff --git a/internal/generate/types.go b/internal/generate/types.go index 9d1415b..dfa4bc1 100644 --- a/internal/generate/types.go +++ b/internal/generate/types.go @@ -102,27 +102,26 @@ func (f TypeField) Description() string { } // StructTag returns the JSON/YAML struct tags for this field. +// Configure json/yaml struct tags. By default, omit empty/zero +// values, but retain them for required fields. +// +// TODO: Use `omitzero` rather than `omitempty` on all relevant +// fields: https://github.com/oxidecomputer/oxide.go/issues/290 func (f TypeField) StructTag() string { - // Derive the omit directive. var omitDirective string switch { case f.OmitDirective != "": - // Explicit override. omitDirective = f.OmitDirective case f.Schema == nil: - // Special case: no schema (e.g., Body field). omitDirective = "omitempty" case f.Required || isNullableArray(f.Schema): - // Required or nullable array: no directive (always serialize). omitDirective = "" case slices.Contains(omitzeroTypes(), f.Type): - // Special types: omitzero. omitDirective = "omitzero" default: omitDirective = "omitempty" } - // Build the tag value. tagValue := f.MarshalKey if omitDirective != "" { tagValue = f.MarshalKey + "," + omitDirective @@ -131,6 +130,42 @@ func (f TypeField) StructTag() string { return fmt.Sprintf("`json:\"%s\" yaml:\"%s\"`", tagValue, tagValue) } +// IsPointer returns whether this field should be a pointer type. +// +// Note: Primitive type pointer logic (int, bool, time) is handled in +// schemaValueToGoType() because those types can appear in nested contexts +// (map values, array items) that don't go through TypeField. +func (f TypeField) IsPointer() bool { + if f.Schema == nil { + return false + } + + v := f.Schema.Value + + // Required + nullable fields should be pointers (Omicron API pattern): + // they can be set to a null value, but they must not be omitted. + // The SDK presents these fields as optional and serializes them to + // `null` if not provided. + if f.Required && v.Nullable { + return true + } + + // Check hardcoded nullable exceptions (upstream API workarounds) + if slices.Contains(nullable(), f.Type) { + return true + } + + return false +} + +// GoType returns the Go type for this field, with pointer prefix if needed. +func (f TypeField) GoType() string { + if f.IsPointer() && !strings.HasPrefix(f.Type, "*") { + return "*" + f.Type + } + return f.Type +} + // EnumTemplate holds the information for enum types. type EnumTemplate struct { Description string @@ -562,22 +597,7 @@ func createTypeObject(schema *openapi3.Schema, name, typeName, description strin } } - // Omicron includes fields that are both required and nullable: - // they can be set to a null value, but they must not be - // omitted. The sdk should present these fields to the user as - // optional, and serialize them to `null` if not provided. isRequired := slices.Contains(required, k) - isRequiredNullable := v.Value.Nullable && isRequired - if slices.Contains(nullable(), typeName) || isRequiredNullable { - // We may have already decided to use a pointer. For - // example, convertToValidGoType always uses pointers - // for ints and bools. Prefix the type with "*", unless - // we've already made it a pointer upstream. - if !strings.HasPrefix(typeName, "*") { - typeName = fmt.Sprintf("*%s", typeName) - } - } - field := TypeField{ Name: strcase.ToCamel(k), Type: typeName, @@ -585,6 +605,7 @@ func createTypeObject(schema *openapi3.Schema, name, typeName, description strin Schema: v, Required: isRequired, } + // Note: pointer prefix is applied by TypeField.GoType() based on IsPointer() fields = append(fields, field) diff --git a/internal/generate/types_test.go b/internal/generate/types_test.go index 07701fe..a5835b8 100644 --- a/internal/generate/types_test.go +++ b/internal/generate/types_test.go @@ -188,6 +188,56 @@ func TestTypeField_StructTag(t *testing.T) { }) } +func TestTypeField_IsPointer(t *testing.T) { + tests := []struct { + name string + field TypeField + expected bool + }{ + { + name: "nil schema", + field: TypeField{Name: "Body", Schema: nil}, + expected: false, + }, + { + name: "nullable required", + field: TypeField{ + Name: "Config", + Type: "SomeConfig", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"object"}, Nullable: true}}, + Required: true, + }, + expected: true, + }, + { + name: "nullable not required", + field: TypeField{ + Name: "Config", + Type: "SomeConfig", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"object"}, Nullable: true}}, + Required: false, + }, + expected: false, + }, + { + name: "not nullable required", + field: TypeField{ + Name: "Config", + Type: "SomeConfig", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Type: &openapi3.Types{"object"}, Nullable: false}}, + Required: true, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.field.IsPointer()) + }) + } +} + func Test_createTypeObject(t *testing.T) { typesSpec := openapi3.Schema{ Required: []string{"type"}, @@ -338,14 +388,14 @@ func Test_createOneOf(t *testing.T) { }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotTypes, gotEnums := createOneOf(tt.schema, tt.typeName, tt.typeName) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotTypes, gotEnums := createOneOf(tc.schema, tc.typeName, tc.typeName) - if diff := cmp.Diff(tt.wantTypes, gotTypes, cmpIgnoreSchema); diff != "" { + if diff := cmp.Diff(tc.wantTypes, gotTypes, cmpIgnoreSchema); diff != "" { t.Errorf("types mismatch (-want +got):\n%s", diff) } - assert.Equal(t, tt.wantEnums, gotEnums) + assert.Equal(t, tc.wantEnums, gotEnums) }) } } diff --git a/internal/generate/utils.go b/internal/generate/utils.go index 8285fa8..356aff1 100644 --- a/internal/generate/utils.go +++ b/internal/generate/utils.go @@ -59,19 +59,13 @@ func isNullableArray(v *openapi3.SchemaRef) bool { // formatStringType converts a string schema to a valid Go type. func formatStringType(t *openapi3.Schema) string { - var format string switch t.Format { - case "date-time": - format = "*time.Time" - case "date": - format = "*time.Time" - case "time": - format = "*time.Time" + case "date-time", "date", "time": + // Time types need pointers for JSON marshaling + return "*time.Time" default: - format = "string" + return "string" } - - return format } // toLowerFirstLetter returns the given string with the first letter converted to lower case. @@ -201,10 +195,6 @@ func schemaValueToGoType(schemaValue *openapi3.Schema, property string) string { return fmt.Sprintf("[]%v", schemaType) } - if schemaValue.Type.Is("object") { - return "object" - } - fmt.Printf("[WARN] TODO: handle type %q for %q, marking as any for now\n", schemaValue.Type, property) return "any" } From c4a9a4f14ed56c97b61fe7c2dc7d149ff9a962bc Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 9 Dec 2025 12:55:19 -0500 Subject: [PATCH 3/3] Clean up createTypeField tests. --- internal/generate/types.go | 8 +++---- internal/generate/types_test.go | 40 +++++++++++++++++---------------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/internal/generate/types.go b/internal/generate/types.go index dfa4bc1..a433889 100644 --- a/internal/generate/types.go +++ b/internal/generate/types.go @@ -263,10 +263,10 @@ func constructParamTypes(paths map[string]*openapi3.PathItem) []TypeTemplate { paramName := strcase.ToCamel(p.Value.Name) paramType := convertToValidGoType("", "", p.Value.Schema) field := TypeField{ - Name: paramName, - Type: paramType, - MarshalKey: p.Value.Name, - Schema: nil, // nil so StructTag always uses omitempty + Name: paramName, + Type: paramType, + MarshalKey: p.Value.Name, + OmitDirective: "omitempty", } if p.Value.Required { diff --git a/internal/generate/types_test.go b/internal/generate/types_test.go index a5835b8..2233412 100644 --- a/internal/generate/types_test.go +++ b/internal/generate/types_test.go @@ -128,6 +128,15 @@ func TestTypeField_Description(t *testing.T) { } assert.Equal(t, "// Foo is a foo field", f.Description()) }) + + t.Run("fallback", func(t *testing.T) { + f := TypeField{ + Name: "Foo", + Schema: &openapi3.SchemaRef{Value: &openapi3.Schema{Description: ""}}, + FallbackDescription: true, + } + assert.Equal(t, "// Foo is the type definition for a Foo.", f.Description()) + }) } func TestTypeField_StructTag(t *testing.T) { @@ -252,26 +261,19 @@ func Test_createTypeObject(t *testing.T) { got := createTypeObject(&typesSpec, "DiskSource", "DiskSourceSnapshot", "Create a disk from a disk snapshot") - assert.Equal(t, "DiskSourceSnapshot", got.Name) - assert.Equal(t, "struct", got.Type) - assert.Equal(t, "Create a disk from a disk snapshot\n//\n// Required fields:\n// - Type", got.Description) - assert.Len(t, got.Fields, 2) - - // Check first field (snapshot_id) - assert.Equal(t, "SnapshotId", got.Fields[0].Name) - assert.Equal(t, "string", got.Fields[0].Type) - assert.Equal(t, "snapshot_id", got.Fields[0].MarshalKey) - assert.False(t, got.Fields[0].Required) - assert.Equal(t, "", got.Fields[0].Description()) - assert.Equal(t, "`json:\"snapshot_id,omitempty\" yaml:\"snapshot_id,omitempty\"`", got.Fields[0].StructTag()) + want := TypeTemplate{ + Name: "DiskSourceSnapshot", + Type: "struct", + Description: "Create a disk from a disk snapshot\n//\n// Required fields:\n// - Type", + Fields: []TypeField{ + {Name: "SnapshotId", Type: "string", MarshalKey: "snapshot_id", Required: false}, + {Name: "Type", Type: "DiskSourceType", MarshalKey: "type", Required: true}, + }, + } - // Check second field (type) - assert.Equal(t, "Type", got.Fields[1].Name) - assert.Equal(t, "DiskSourceType", got.Fields[1].Type) - assert.Equal(t, "type", got.Fields[1].MarshalKey) - assert.True(t, got.Fields[1].Required) - assert.Equal(t, "", got.Fields[1].Description()) - assert.Equal(t, "`json:\"type\" yaml:\"type\"`", got.Fields[1].StructTag()) + if diff := cmp.Diff(want, got, cmpIgnoreSchema); diff != "" { + t.Errorf("createTypeObject() mismatch (-want +got):\n%s", diff) + } } func Test_createStringEnum(t *testing.T) {