Repository: oapi-codegen/oapi-codegen

Issue:
regression in v2.5.0: marshalling optional array property emits null instead of omitting the array
The changes in https://github.com/oapi-codegen/oapi-codegen/pull/1981 causes nil optional array properties to be marshaled as `null` (and thus not valid according to the schema) in some cases due to the changes in `pkg/codegen/templates/additional-properties.tmpl`, `pkg/codegen/templates/union-and-additional-properties.tmpl`, and `pkg/codegen/templates/union.tmpl`.

What seems to be required to trigger this regression is:
- The array property is part of a union or other structure to trigger one of the above templates
- The array property is optional (not in the `required` list)
- The array property has `x-go-type-skip-optional-pointer: true`
  - This is common (at least for me) from prior versions because slices already can store `nil`, so an extra pointer layer is unnecessary

The change in the generated code looks something like this:
```diff
-       if t.Foo != nil {
-               object["foo"], err = json.Marshal(t.Foo)
-               if err != nil {
-                       return nil, fmt.Errorf("error marshaling 'foo': %w", err)
-               }
+       object["foo"], err = json.Marshal(t.Foo)
+       if err != nil {
+               return nil, fmt.Errorf("error marshaling 'foo': %w", err)
        }
```

I consider this a regression because it causes it to emit a schema-invalid JSON document after parsing a valid one.

Thinking through the logic, I find:
- v2.4.0:
  - Skip emitting a property if it is not required and is nil
- v2.5.0:
  - Skip emitting a property if it has a pointer layer and is nil
- What I _think_ it should do:
  - Skip emitting a property if **either**:
  - It has an optional pointer and is nil (implies it is not required)
  - It is not required and is an inherently nillable type (slice, map) and is nil

Hints from issue comments:
@magnetikonline: Change in this behavior is causing exactly the same regression here.

To provide more context, I'm seeing a problem related to #2021, where the setting of `SkipOptionalPointer: globalState.options.OutputOptions.PreferSkipOptionalPointer,` for referenced schema types is using the _global_ setting, not that of the reference.
@magnetikonline: 👋 @mgabeler-lee-6rs - I dug more into my issue/regression and have landed what I think is a fix.

#2042

I'd be curious to see if this does indeed fix the problem for you and if it's related.
@mgabeler-lee-6rs: @magnetikonline While related, I think your issue is different from mine. In my case, many of the properties in question already had an explicit `x-go-type-skip-optional-pointer: true`, since the extra pointer layer was not needed to represent `nil` for a map or slice type.

My problem is that the codegen now _always_ emits those fields, even if nil, instead of obeying their "not required ⇒ don't emit if nil" behavior from before.

I very much do _not_ want to make them all pointers again, as that will be a messy refactor of the consuming code, and make that code a lot more janky.@magnetikonline: > My problem is that the codegen now _always_ emits those fields, even if nil, instead of obeying their "not required ⇒ don't emit if nil" behavior from before.

thanks @mgabeler-lee-6rs for the clarification. 👍@jamietanna: Sorry for the delay, and regression, taking a look hopefully today 👀 @jamietanna: @mgabeler-lee-6rs can you provide a minimal OpenAPI spec (snippet) for what this is triggering for?@mgabeler-lee-6rs: @jamietanna Here's a failing test for the first of the three variants noted in the issue description: https://github.com/mgabeler-lee-6rs/oapi-codegen/tree/issue-2031-failing-test/internal/test/issues/issue-2031

The other two variants exhibit the same basic behavior, they are just going through other templates where unions (`anyOf`) are involved to force generation of a custom `MarshalJSON` instead of using `additionalProperties` to trigger that. I can expand the failing tests to encompass those if it helps.@jamietanna: Apologies for the delay - taking a look now. Thanks for the failing test + minimal example, that's super helpful 🙇 @jamietanna: I generally stand by the changes we made in https://github.com/oapi-codegen/oapi-codegen/pull/1981, but absolutely see that we should be taking into account _the type we're operating on_ and whether that's a type that can be nullable i.e. whether it's an array/slice - I'm working on the fix now@jamietanna: https://github.com/oapi-codegen/oapi-codegen/pull/2118 will be the fix 👍 

Solution patch:
diff --git a/pkg/codegen/schema.go b/pkg/codegen/schema.go
index 1d03fbd..c099752 100644
--- a/pkg/codegen/schema.go
+++ b/pkg/codegen/schema.go
@@ -128,12 +128,29 @@ 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 {
 	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/templates/additional-properties.tmpl b/pkg/codegen/templates/additional-properties.tmpl
index 2f45058..a103216 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 1c48092..6ec69f5 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 c0385f8..b27a2ac 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 -}}



Test patch:
diff --git a/internal/test/issues/issue-2031/config.yaml b/internal/test/issues/issue-2031/config.yaml
new file mode 100644
index 0000000..495396d
--- /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 0000000..1e29681
--- /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 0000000..1ae12da
--- /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 0000000..fe631a8
--- /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 0000000..54aa5eb
--- /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/schema_test.go b/pkg/codegen/schema_test.go
index c4b3f8b..6075594 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())
+			}
+		})
+	}
+}