From ecb5d55d153605e84211c19493c28942dd3f4dc6 Mon Sep 17 00:00:00 2001 From: Jamie Tanna Date: Sat, 1 Nov 2025 10:59:30 +0000 Subject: [PATCH 1/3] chore(templates): add `Property.ZeroValueIsNil` helper method As part of #2031, we need to correct a subtle breakage in #1981, where we would not correctly perform the nil check on arrays, breaking marshalling functionality. As a first step, we add a new helper method to inform as to whether the zero value of a Property could be `nil`, and therefore needs a `nil` check. --- pkg/codegen/schema.go | 11 +++++++ pkg/codegen/schema_test.go | 65 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/pkg/codegen/schema.go b/pkg/codegen/schema.go index 1d03fbdd0e..8b9b38a0e8 100644 --- a/pkg/codegen/schema.go +++ b/pkg/codegen/schema.go @@ -134,6 +134,17 @@ func (p Property) HasOptionalPointer() bool { return p.Required == false && p.Schema.SkipOptionalPointer == false //nolint:staticcheck } +// ZeroValueIsNil is a helper function to determine if the given Go type used for this property +// Will return true if the OpenAPI `type` is: +// - `array` +func (p Property) ZeroValueIsNil() bool { + if p.Schema.OAPISchema == nil { + return false + } + + return p.Schema.OAPISchema.Type.Is("array") +} + // EnumDefinition holds type information for enum type EnumDefinition struct { // Schema is the scheme of a type which has a list of enum values, eg, the diff --git a/pkg/codegen/schema_test.go b/pkg/codegen/schema_test.go index c4b3f8bd7b..6075594a32 100644 --- a/pkg/codegen/schema_test.go +++ b/pkg/codegen/schema_test.go @@ -3,7 +3,9 @@ package codegen import ( "testing" + "github.com/getkin/kin-openapi/openapi3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestProperty_GoTypeDef(t *testing.T) { @@ -454,3 +456,66 @@ func TestProperty_GoTypeDef_nullable(t *testing.T) { }) } } + +func TestProperty_ZeroValueIsNil(t *testing.T) { + newType := func(typ string) *openapi3.Types { + return &openapi3.Types{typ} + } + + tests := []struct { + name string + oapiSchema *openapi3.Schema + expectIsNil bool + }{ + { + name: "when an array, returns true", + oapiSchema: &openapi3.Schema{Type: newType("array")}, + expectIsNil: true, + }, + { + name: "when an object, returns false", + oapiSchema: &openapi3.Schema{Type: newType("object")}, + expectIsNil: false, + }, + { + name: "when a string, returns false", + oapiSchema: &openapi3.Schema{Type: newType("string")}, + expectIsNil: false, + }, + { + name: "when an integer, returns false", + oapiSchema: &openapi3.Schema{Type: newType("integer")}, + expectIsNil: false, + }, + { + name: "when a number, returns false", + oapiSchema: &openapi3.Schema{Type: newType("number")}, + expectIsNil: false, + }, + { + name: "when OAPISchema is nil, returns false", + oapiSchema: nil, + expectIsNil: false, + }, + { + name: "when OAPISchema is zero value, returns false", + oapiSchema: &openapi3.Schema{}, + expectIsNil: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prop := Property{ + Schema: Schema{ + OAPISchema: tt.oapiSchema, + }, + } + if tt.expectIsNil { + require.True(t, prop.ZeroValueIsNil()) + } else { + require.False(t, prop.ZeroValueIsNil()) + } + }) + } +} From 0eea28bd5523194bee386e640d24820a710d0ee4 Mon Sep 17 00:00:00 2001 From: Jamie Tanna Date: Sat, 1 Nov 2025 11:10:10 +0000 Subject: [PATCH 2/3] chore(templates): add `Property.RequiresPointerCheck` helper method As part of #2031, we need to correct a subtle breakage in #1981, where we would not correctly perform the nil check on arrays, breaking marshalling functionality. As a first step, we add a new helper method to be used to validate whether a `nil` check is required on a given type, which is used for either types with a zero value of `nil`, or where we have an optional pointer set. --- pkg/codegen/schema.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/codegen/schema.go b/pkg/codegen/schema.go index 8b9b38a0e8..c099752d88 100644 --- a/pkg/codegen/schema.go +++ b/pkg/codegen/schema.go @@ -128,6 +128,12 @@ func (p Property) GoTypeDef() string { return typeDef } +// RequiresNilCheck indicates whether the generated property should have a nil check performed on it before other checks. +// This should be used in templates when performing `nil` checks, but NOT when i.e. determining if there should be an optional pointer given to the type - in that case, use `HasOptionalPointer` +func (p Property) RequiresNilCheck() bool { + return p.ZeroValueIsNil() || p.HasOptionalPointer() +} + // HasOptionalPointer indicates whether the generated property has an optional pointer associated with it. // This takes into account the `x-go-type-skip-optional-pointer` extension, allowing a parameter definition to control whether the pointer should be skipped. func (p Property) HasOptionalPointer() bool { From 71829e99fad053c7c9cd36a76af57a0eee8489db Mon Sep 17 00:00:00 2001 From: Jamie Tanna Date: Sat, 1 Nov 2025 10:17:27 +0000 Subject: [PATCH 3/3] fix(templates): correctly nil check arrays As part of #2031, we need to correct a subtle breakage in #1981, where we would not correctly perform the nil check on arrays, breaking marshalling functionality. To do this we can modify our templates to call the newly added `RequiresNilCheck` when performing nil checks Closes #2031. Co-authored-by: Matthew Gabeler-Lee --- internal/test/issues/issue-2031/config.yaml | 4 + internal/test/issues/issue-2031/generate.go | 3 + .../test/issues/issue-2031/issue2031.gen.go | 83 +++++++++++++++++++ .../test/issues/issue-2031/issue2031_test.go | 18 ++++ internal/test/issues/issue-2031/openapi.yaml | 27 ++++++ .../templates/additional-properties.tmpl | 4 +- .../union-and-additional-properties.tmpl | 4 +- pkg/codegen/templates/union.tmpl | 4 +- 8 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 internal/test/issues/issue-2031/config.yaml create mode 100644 internal/test/issues/issue-2031/generate.go create mode 100644 internal/test/issues/issue-2031/issue2031.gen.go create mode 100644 internal/test/issues/issue-2031/issue2031_test.go create mode 100644 internal/test/issues/issue-2031/openapi.yaml diff --git a/internal/test/issues/issue-2031/config.yaml b/internal/test/issues/issue-2031/config.yaml new file mode 100644 index 0000000000..495396d560 --- /dev/null +++ b/internal/test/issues/issue-2031/config.yaml @@ -0,0 +1,4 @@ +package: issue2031 +generate: + models: true +output: issue2031.gen.go diff --git a/internal/test/issues/issue-2031/generate.go b/internal/test/issues/issue-2031/generate.go new file mode 100644 index 0000000000..1e29681627 --- /dev/null +++ b/internal/test/issues/issue-2031/generate.go @@ -0,0 +1,3 @@ +package issue2031 + +//go:generate go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=config.yaml openapi.yaml diff --git a/internal/test/issues/issue-2031/issue2031.gen.go b/internal/test/issues/issue-2031/issue2031.gen.go new file mode 100644 index 0000000000..1ae12da963 --- /dev/null +++ b/internal/test/issues/issue-2031/issue2031.gen.go @@ -0,0 +1,83 @@ +// Package issue2031 provides primitives to interact with the openapi HTTP API. +// +// Code generated by github.com/oapi-codegen/oapi-codegen/v2 version v2.0.0-00010101000000-000000000000 DO NOT EDIT. +package issue2031 + +import ( + "encoding/json" + "fmt" +) + +// ArrayContainer defines model for ArrayContainer. +type ArrayContainer struct { + Values []string `json:"values,omitempty"` + AdditionalProperties map[string]interface{} `json:"-"` +} + +// Getter for additional properties for ArrayContainer. Returns the specified +// element and whether it was found +func (a ArrayContainer) Get(fieldName string) (value interface{}, found bool) { + if a.AdditionalProperties != nil { + value, found = a.AdditionalProperties[fieldName] + } + return +} + +// Setter for additional properties for ArrayContainer +func (a *ArrayContainer) Set(fieldName string, value interface{}) { + if a.AdditionalProperties == nil { + a.AdditionalProperties = make(map[string]interface{}) + } + a.AdditionalProperties[fieldName] = value +} + +// Override default JSON handling for ArrayContainer to handle AdditionalProperties +func (a *ArrayContainer) UnmarshalJSON(b []byte) error { + object := make(map[string]json.RawMessage) + err := json.Unmarshal(b, &object) + if err != nil { + return err + } + + if raw, found := object["values"]; found { + err = json.Unmarshal(raw, &a.Values) + if err != nil { + return fmt.Errorf("error reading 'values': %w", err) + } + delete(object, "values") + } + + if len(object) != 0 { + a.AdditionalProperties = make(map[string]interface{}) + for fieldName, fieldBuf := range object { + var fieldVal interface{} + err := json.Unmarshal(fieldBuf, &fieldVal) + if err != nil { + return fmt.Errorf("error unmarshaling field %s: %w", fieldName, err) + } + a.AdditionalProperties[fieldName] = fieldVal + } + } + return nil +} + +// Override default JSON handling for ArrayContainer to handle AdditionalProperties +func (a ArrayContainer) MarshalJSON() ([]byte, error) { + var err error + object := make(map[string]json.RawMessage) + + if a.Values != nil { + object["values"], err = json.Marshal(a.Values) + if err != nil { + return nil, fmt.Errorf("error marshaling 'values': %w", err) + } + } + + for fieldName, field := range a.AdditionalProperties { + object[fieldName], err = json.Marshal(field) + if err != nil { + return nil, fmt.Errorf("error marshaling '%s': %w", fieldName, err) + } + } + return json.Marshal(object) +} diff --git a/internal/test/issues/issue-2031/issue2031_test.go b/internal/test/issues/issue-2031/issue2031_test.go new file mode 100644 index 0000000000..fe631a8bcf --- /dev/null +++ b/internal/test/issues/issue-2031/issue2031_test.go @@ -0,0 +1,18 @@ +package issue2031 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMarshal(t *testing.T) { + value := ArrayContainer{} + content, err := json.Marshal(value) + require.NoError(t, err) + // the _optional array_ should be _omitted_ when null, not marshaled as null + // (which is not valid per the schema) + assert.Equal(t, "{}", string(content)) +} diff --git a/internal/test/issues/issue-2031/openapi.yaml b/internal/test/issues/issue-2031/openapi.yaml new file mode 100644 index 0000000000..54aa5eb404 --- /dev/null +++ b/internal/test/issues/issue-2031/openapi.yaml @@ -0,0 +1,27 @@ +openapi: "3.0.0" +info: + version: 1.0.0 + title: Issue 2031 +paths: + /test: + get: + responses: + "200": + description: A list of strings + content: + application/json: + schema: + $ref: "#/components/schemas/ArrayContainer" +components: + schemas: + ArrayContainer: + type: object + # enabling additionalProperties is required to expose one variant of the bug + additionalProperties: true + properties: + # NOTE: the array property is NOT required and NOT nullable + values: + x-go-type-skip-optional-pointer: true + type: array + items: + type: string diff --git a/pkg/codegen/templates/additional-properties.tmpl b/pkg/codegen/templates/additional-properties.tmpl index 2f450582d1..a103216c94 100644 --- a/pkg/codegen/templates/additional-properties.tmpl +++ b/pkg/codegen/templates/additional-properties.tmpl @@ -53,12 +53,12 @@ func (a {{.TypeName}}) MarshalJSON() ([]byte, error) { var err error object := make(map[string]json.RawMessage) {{range .Schema.Properties}} -{{if .HasOptionalPointer}}if a.{{.GoFieldName}} != nil { {{end}} +{{if .RequiresNilCheck}}if a.{{.GoFieldName}} != nil { {{end}} object["{{.JsonFieldName}}"], err = json.Marshal(a.{{.GoFieldName}}) if err != nil { return nil, fmt.Errorf("error marshaling '{{.JsonFieldName}}': %w", err) } -{{if .HasOptionalPointer}} }{{end}} +{{if .RequiresNilCheck}} }{{end}} {{end}} for fieldName, field := range a.AdditionalProperties { object[fieldName], err = json.Marshal(field) diff --git a/pkg/codegen/templates/union-and-additional-properties.tmpl b/pkg/codegen/templates/union-and-additional-properties.tmpl index 1c48092ccf..6ec69f5e85 100644 --- a/pkg/codegen/templates/union-and-additional-properties.tmpl +++ b/pkg/codegen/templates/union-and-additional-properties.tmpl @@ -54,12 +54,12 @@ func (a {{.TypeName}}) MarshalJSON() ([]byte, error) { } } {{range .Schema.Properties}} -{{if .HasOptionalPointer}}if a.{{.GoFieldName}} != nil { {{end}} +{{if .RequiresNilCheck}}if a.{{.GoFieldName}} != nil { {{end}} object["{{.JsonFieldName}}"], err = json.Marshal(a.{{.GoFieldName}}) if err != nil { return nil, fmt.Errorf("error marshaling '{{.JsonFieldName}}': %w", err) } -{{if .HasOptionalPointer}} }{{end}} +{{if .RequiresNilCheck}} }{{end}} {{end}} for fieldName, field := range a.AdditionalProperties { object[fieldName], err = json.Marshal(field) diff --git a/pkg/codegen/templates/union.tmpl b/pkg/codegen/templates/union.tmpl index c0385f82e3..b27a2aca3f 100644 --- a/pkg/codegen/templates/union.tmpl +++ b/pkg/codegen/templates/union.tmpl @@ -102,12 +102,12 @@ } } {{range .Schema.Properties}} - {{if .HasOptionalPointer}}if t.{{.GoFieldName}} != nil { {{end}} + {{if .RequiresNilCheck}}if t.{{.GoFieldName}} != nil { {{end}} object["{{.JsonFieldName}}"], err = json.Marshal(t.{{.GoFieldName}}) if err != nil { return nil, fmt.Errorf("error marshaling '{{.JsonFieldName}}': %w", err) } - {{if .HasOptionalPointer}} }{{end}} + {{if .RequiresNilCheck}} }{{end}} {{end -}} b, err = json.Marshal(object) {{end -}}