From b6341478472d83d37baac9a339b2bcfa029d39fa Mon Sep 17 00:00:00 2001 From: Marcin Romaszewicz Date: Sat, 28 Feb 2026 08:07:20 -0800 Subject: [PATCH] fix: use RequiresNilCheck for all params PR #2237 fixed query parameters to use RequiresNilCheck instead of HasOptionalPointer for nil-check guards, but the same bug remained in the header and cookie parameter sections of client.tmpl. This caused optional array params (e.g. []string) to be sent even when nil, because prefer-skip-optional-pointer removes the pointer wrapper and HasOptionalPointer returns false, skipping the nil check entirely. Co-Authored-By: Claude Opus 4.6 --- internal/test/issues/issue-2238/config.yaml | 7 + internal/test/issues/issue-2238/generate.go | 3 + .../test/issues/issue-2238/issue2238.gen.go | 262 ++++++++++++++++++ .../test/issues/issue-2238/issue2238_test.go | 56 ++++ internal/test/issues/issue-2238/openapi.yaml | 22 ++ pkg/codegen/templates/client.tmpl | 8 +- 6 files changed, 354 insertions(+), 4 deletions(-) create mode 100644 internal/test/issues/issue-2238/config.yaml create mode 100644 internal/test/issues/issue-2238/generate.go create mode 100644 internal/test/issues/issue-2238/issue2238.gen.go create mode 100644 internal/test/issues/issue-2238/issue2238_test.go create mode 100644 internal/test/issues/issue-2238/openapi.yaml diff --git a/internal/test/issues/issue-2238/config.yaml b/internal/test/issues/issue-2238/config.yaml new file mode 100644 index 000000000..cbb78b8d3 --- /dev/null +++ b/internal/test/issues/issue-2238/config.yaml @@ -0,0 +1,7 @@ +package: issue2238 +generate: + models: true + client: true +output-options: + prefer-skip-optional-pointer: true +output: issue2238.gen.go diff --git a/internal/test/issues/issue-2238/generate.go b/internal/test/issues/issue-2238/generate.go new file mode 100644 index 000000000..5c1077323 --- /dev/null +++ b/internal/test/issues/issue-2238/generate.go @@ -0,0 +1,3 @@ +package issue2238 + +//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-2238/issue2238.gen.go b/internal/test/issues/issue-2238/issue2238.gen.go new file mode 100644 index 000000000..81f395ddf --- /dev/null +++ b/internal/test/issues/issue-2238/issue2238.gen.go @@ -0,0 +1,262 @@ +// Package issue2238 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 issue2238 + +import ( + "context" + "fmt" + "io" + "net/http" + "net/url" + "strings" + + "github.com/oapi-codegen/runtime" +) + +// GetTestParams defines parameters for GetTest. +type GetTestParams struct { + XTags []string `json:"X-Tags,omitempty"` + Tags []string `form:"tags,omitempty" json:"tags,omitempty"` +} + +// RequestEditorFn is the function signature for the RequestEditor callback function +type RequestEditorFn func(ctx context.Context, req *http.Request) error + +// Doer performs HTTP requests. +// +// The standard http.Client implements this interface. +type HttpRequestDoer interface { + Do(req *http.Request) (*http.Response, error) +} + +// Client which conforms to the OpenAPI3 specification for this service. +type Client struct { + // The endpoint of the server conforming to this interface, with scheme, + // https://api.deepmap.com for example. This can contain a path relative + // to the server, such as https://api.deepmap.com/dev-test, and all the + // paths in the swagger spec will be appended to the server. + Server string + + // Doer for performing requests, typically a *http.Client with any + // customized settings, such as certificate chains. + Client HttpRequestDoer + + // A list of callbacks for modifying requests which are generated before sending over + // the network. + RequestEditors []RequestEditorFn +} + +// ClientOption allows setting custom parameters during construction +type ClientOption func(*Client) error + +// Creates a new Client, with reasonable defaults +func NewClient(server string, opts ...ClientOption) (*Client, error) { + // create a client with sane default values + client := Client{ + Server: server, + } + // mutate client and add all optional params + for _, o := range opts { + if err := o(&client); err != nil { + return nil, err + } + } + // ensure the server URL always has a trailing slash + if !strings.HasSuffix(client.Server, "/") { + client.Server += "/" + } + // create httpClient, if not already present + if client.Client == nil { + client.Client = &http.Client{} + } + return &client, nil +} + +// WithHTTPClient allows overriding the default Doer, which is +// automatically created using http.Client. This is useful for tests. +func WithHTTPClient(doer HttpRequestDoer) ClientOption { + return func(c *Client) error { + c.Client = doer + return nil + } +} + +// WithRequestEditorFn allows setting up a callback function, which will be +// called right before sending the request. This can be used to mutate the request. +func WithRequestEditorFn(fn RequestEditorFn) ClientOption { + return func(c *Client) error { + c.RequestEditors = append(c.RequestEditors, fn) + return nil + } +} + +// The interface specification for the client above. +type ClientInterface interface { + // GetTest request + GetTest(ctx context.Context, params *GetTestParams, reqEditors ...RequestEditorFn) (*http.Response, error) +} + +func (c *Client) GetTest(ctx context.Context, params *GetTestParams, reqEditors ...RequestEditorFn) (*http.Response, error) { + req, err := NewGetTestRequest(c.Server, params) + if err != nil { + return nil, err + } + req = req.WithContext(ctx) + if err := c.applyEditors(ctx, req, reqEditors); err != nil { + return nil, err + } + return c.Client.Do(req) +} + +// NewGetTestRequest generates requests for GetTest +func NewGetTestRequest(server string, params *GetTestParams) (*http.Request, error) { + var err error + + serverURL, err := url.Parse(server) + if err != nil { + return nil, err + } + + operationPath := fmt.Sprintf("/test") + if operationPath[0] == '/' { + operationPath = "." + operationPath + } + + queryURL, err := serverURL.Parse(operationPath) + if err != nil { + return nil, err + } + + req, err := http.NewRequest("GET", queryURL.String(), nil) + if err != nil { + return nil, err + } + + if params != nil { + + if params.XTags != nil { + var headerParam0 string + + headerParam0, err = runtime.StyleParamWithOptions("simple", false, "X-Tags", params.XTags, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationHeader, Type: "array", Format: ""}) + if err != nil { + return nil, err + } + + req.Header.Set("X-Tags", headerParam0) + } + + } + + if params != nil { + + if params.Tags != nil { + var cookieParam0 string + + cookieParam0, err = runtime.StyleParamWithOptions("simple", true, "tags", params.Tags, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationCookie, Type: "array", Format: ""}) + if err != nil { + return nil, err + } + + cookie0 := &http.Cookie{ + Name: "tags", + Value: cookieParam0, + } + req.AddCookie(cookie0) + } + } + return req, nil +} + +func (c *Client) applyEditors(ctx context.Context, req *http.Request, additionalEditors []RequestEditorFn) error { + for _, r := range c.RequestEditors { + if err := r(ctx, req); err != nil { + return err + } + } + for _, r := range additionalEditors { + if err := r(ctx, req); err != nil { + return err + } + } + return nil +} + +// ClientWithResponses builds on ClientInterface to offer response payloads +type ClientWithResponses struct { + ClientInterface +} + +// NewClientWithResponses creates a new ClientWithResponses, which wraps +// Client with return type handling +func NewClientWithResponses(server string, opts ...ClientOption) (*ClientWithResponses, error) { + client, err := NewClient(server, opts...) + if err != nil { + return nil, err + } + return &ClientWithResponses{client}, nil +} + +// WithBaseURL overrides the baseURL. +func WithBaseURL(baseURL string) ClientOption { + return func(c *Client) error { + newBaseURL, err := url.Parse(baseURL) + if err != nil { + return err + } + c.Server = newBaseURL.String() + return nil + } +} + +// ClientWithResponsesInterface is the interface specification for the client with responses above. +type ClientWithResponsesInterface interface { + // GetTestWithResponse request + GetTestWithResponse(ctx context.Context, params *GetTestParams, reqEditors ...RequestEditorFn) (*GetTestResponse, error) +} + +type GetTestResponse struct { + Body []byte + HTTPResponse *http.Response +} + +// Status returns HTTPResponse.Status +func (r GetTestResponse) Status() string { + if r.HTTPResponse != nil { + return r.HTTPResponse.Status + } + return http.StatusText(0) +} + +// StatusCode returns HTTPResponse.StatusCode +func (r GetTestResponse) StatusCode() int { + if r.HTTPResponse != nil { + return r.HTTPResponse.StatusCode + } + return 0 +} + +// GetTestWithResponse request returning *GetTestResponse +func (c *ClientWithResponses) GetTestWithResponse(ctx context.Context, params *GetTestParams, reqEditors ...RequestEditorFn) (*GetTestResponse, error) { + rsp, err := c.GetTest(ctx, params, reqEditors...) + if err != nil { + return nil, err + } + return ParseGetTestResponse(rsp) +} + +// ParseGetTestResponse parses an HTTP response from a GetTestWithResponse call +func ParseGetTestResponse(rsp *http.Response) (*GetTestResponse, error) { + bodyBytes, err := io.ReadAll(rsp.Body) + defer func() { _ = rsp.Body.Close() }() + if err != nil { + return nil, err + } + + response := &GetTestResponse{ + Body: bodyBytes, + HTTPResponse: rsp, + } + + return response, nil +} diff --git a/internal/test/issues/issue-2238/issue2238_test.go b/internal/test/issues/issue-2238/issue2238_test.go new file mode 100644 index 000000000..8c399efa5 --- /dev/null +++ b/internal/test/issues/issue-2238/issue2238_test.go @@ -0,0 +1,56 @@ +package issue2238 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewGetTestRequest(t *testing.T) { + t.Run("nil header array param is not sent", func(t *testing.T) { + params := GetTestParams{ + XTags: nil, + } + + req, err := NewGetTestRequest("https://localhost", ¶ms) + require.NoError(t, err) + + assert.Empty(t, req.Header.Values("X-Tags")) + }) + + t.Run("non-nil header array param is sent", func(t *testing.T) { + params := GetTestParams{ + XTags: []string{"a", "b"}, + } + + req, err := NewGetTestRequest("https://localhost", ¶ms) + require.NoError(t, err) + + assert.NotEmpty(t, req.Header.Values("X-Tags")) + }) + + t.Run("nil cookie array param is not sent", func(t *testing.T) { + params := GetTestParams{ + Tags: nil, + } + + req, err := NewGetTestRequest("https://localhost", ¶ms) + require.NoError(t, err) + + assert.Empty(t, req.Cookies()) + }) + + t.Run("non-nil cookie array param is sent", func(t *testing.T) { + params := GetTestParams{ + Tags: []string{"a", "b"}, + } + + req, err := NewGetTestRequest("https://localhost", ¶ms) + require.NoError(t, err) + + cookies := req.Cookies() + require.Len(t, cookies, 1) + assert.Equal(t, "tags", cookies[0].Name) + }) +} diff --git a/internal/test/issues/issue-2238/openapi.yaml b/internal/test/issues/issue-2238/openapi.yaml new file mode 100644 index 000000000..4e026542a --- /dev/null +++ b/internal/test/issues/issue-2238/openapi.yaml @@ -0,0 +1,22 @@ +openapi: "3.0.0" +info: + version: 1.0.0 + title: Issue 2238 +paths: + /test: + get: + parameters: + - name: X-Tags + in: header + schema: + type: array + items: + type: string + required: false + - name: tags + in: cookie + schema: + type: array + items: + type: string + required: false diff --git a/pkg/codegen/templates/client.tmpl b/pkg/codegen/templates/client.tmpl index 27085038d..24410dfae 100644 --- a/pkg/codegen/templates/client.tmpl +++ b/pkg/codegen/templates/client.tmpl @@ -236,7 +236,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr {{ if .HeaderParams }} if params != nil { {{range $paramIdx, $param := .HeaderParams}} - {{if .HasOptionalPointer}} if params.{{.GoName}} != nil { {{end}} + {{if .RequiresNilCheck}} if params.{{.GoName}} != nil { {{end}} var headerParam{{$paramIdx}} string {{if .IsPassThrough}} headerParam{{$paramIdx}} = {{if .HasOptionalPointer}}*{{end}}params.{{.GoName}} @@ -256,7 +256,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr } {{end}} req.Header.Set("{{.ParamName}}", headerParam{{$paramIdx}}) - {{if .HasOptionalPointer}}}{{end}} + {{if .RequiresNilCheck}}}{{end}} {{end}} } {{- end }}{{/* if .HeaderParams */}} @@ -264,7 +264,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr {{ if .CookieParams }} if params != nil { {{range $paramIdx, $param := .CookieParams}} - {{if .HasOptionalPointer}} if params.{{.GoName}} != nil { {{end}} + {{if .RequiresNilCheck}} if params.{{.GoName}} != nil { {{end}} var cookieParam{{$paramIdx}} string {{if .IsPassThrough}} cookieParam{{$paramIdx}} = {{if .HasOptionalPointer}}*{{end}}params.{{.GoName}} @@ -288,7 +288,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr Value:cookieParam{{$paramIdx}}, } req.AddCookie(cookie{{$paramIdx}}) - {{if .HasOptionalPointer}}}{{end}} + {{if .RequiresNilCheck}}}{{end}} {{ end -}} } {{- end }}{{/* if .CookieParams */}}