Skip to content

Commit 918ff57

Browse files
jamietannajamietanna
committed
fix(templates/client): correctly nil check query parameters
As a fix similar to c4ba545, we had cases where the global `prefer-skip-optional-pointer` could lead to not nil checking when we should have been. The previous behaviour would lead to incorrectly converting i.e. `user_ids=nil` to `user_ids[]=`, which is not correct. To do this, we can add the same helper functions that we have elsewhere.
1 parent a14bfbb commit 918ff57

File tree

7 files changed

+361
-3
lines changed

7 files changed

+361
-3
lines changed

‎internal/test/issues/issue-2031/prefer/config.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package: issue2031
2+
generate:
3+
models: true
4+
client: true
5+
output-options:
6+
prefer-skip-optional-pointer: true
7+
output: issue2031.gen.go

‎internal/test/issues/issue-2031/prefer/generate.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
package issue2031
2+
3+
//go:generate go run github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen --config=config.yaml openapi.yaml

‎internal/test/issues/issue-2031/prefer/issue2031.gen.go‎

Lines changed: 251 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/test/issues/issue-2031/prefer/issue2031_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package issue2031
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
func TestNewGetTestRequest(t *testing.T) {
11+
t.Run("does not add the user_ids[] parameter if zero value", func(t *testing.T) {
12+
params := GetTestParams{}
13+
14+
req, err := NewGetTestRequest("https://localhost", &params)
15+
require.NoError(t, err)
16+
17+
assert.Equal(t, "https://localhost/test", req.URL.String())
18+
})
19+
20+
t.Run("does not add the user_ids[] parameter if nil", func(t *testing.T) {
21+
params := GetTestParams{
22+
UserIds: nil,
23+
}
24+
25+
req, err := NewGetTestRequest("https://localhost", &params)
26+
require.NoError(t, err)
27+
28+
assert.Equal(t, "https://localhost/test", req.URL.String())
29+
})
30+
31+
t.Run("adds the user_ids[] parameter if an explicitly initialised empty array", func(t *testing.T) {
32+
params := GetTestParams{
33+
UserIds: []int{},
34+
}
35+
36+
req, err := NewGetTestRequest("https://localhost", &params)
37+
require.NoError(t, err)
38+
39+
assert.Equal(t, "https://localhost/test?user_ids%5B%5D=", req.URL.String())
40+
})
41+
42+
t.Run("adds the user_ids[] parameter if array contains a value", func(t *testing.T) {
43+
params := GetTestParams{
44+
UserIds: []int{1},
45+
}
46+
47+
req, err := NewGetTestRequest("https://localhost", &params)
48+
require.NoError(t, err)
49+
50+
assert.Equal(t, "https://localhost/test?user_ids%5B%5D=1", req.URL.String())
51+
})
52+
53+
t.Run("handles multiple user_ids[] parameters", func(t *testing.T) {
54+
params := GetTestParams{
55+
UserIds: []int{1, 100},
56+
}
57+
58+
req, err := NewGetTestRequest("https://localhost", &params)
59+
require.NoError(t, err)
60+
61+
assert.Equal(t, "https://localhost/test?user_ids%5B%5D=1&user_ids%5B%5D=100", req.URL.String())
62+
})
63+
}

‎internal/test/issues/issue-2031/prefer/openapi.yaml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
openapi: "3.0.0"
2+
info:
3+
version: 1.0.0
4+
title: Issue 2031
5+
paths:
6+
/test:
7+
get:
8+
parameters:
9+
- name: "user_ids[]"
10+
in: query
11+
schema:
12+
type: array
13+
items:
14+
type: integer
15+
style: form
16+
explode: true
17+
required: false

‎pkg/codegen/operations.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,23 @@ func (pd ParameterDefinition) TypeDef() string {
4444
return typeDecl
4545
}
4646

47+
// RequiresNilCheck indicates whether the generated property should have a nil check performed on it before other checks.
48+
// 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`
49+
func (pd ParameterDefinition) RequiresNilCheck() bool {
50+
return pd.ZeroValueIsNil() || pd.HasOptionalPointer()
51+
}
52+
53+
// ZeroValueIsNil is a helper function to determine if the given Go type used for this property
54+
// Will return true if the OpenAPI `type` is:
55+
// - `array`
56+
func (pd ParameterDefinition) ZeroValueIsNil() bool {
57+
if pd.Schema.OAPISchema == nil {
58+
return false
59+
}
60+
61+
return pd.Schema.OAPISchema.Type.Is("array")
62+
}
63+
4764
// JsonTag generates the JSON annotation to map GoType to json type name. If Parameter
4865
// Foo is marshaled to json as "foo", this will create the annotation
4966
// 'json:"foo"'

‎pkg/codegen/templates/client.tmpl‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr
197197
if params != nil {
198198
queryValues := queryURL.Query()
199199
{{range $paramIdx, $param := .QueryParams}}
200-
{{if .HasOptionalPointer}} if params.{{.GoName}} != nil { {{end}}
200+
{{if .RequiresNilCheck}} if params.{{.GoName}} != nil { {{end}}
201201
{{if .IsPassThrough}}
202202
queryValues.Add("{{.ParamName}}", {{if .HasOptionalPointer}}*{{end}}params.{{.GoName}})
203203
{{end}}
@@ -210,7 +210,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr
210210

211211
{{end}}
212212
{{if .IsStyled}}
213-
if queryFrag, err := runtime.StyleParamWithLocation("{{.Style}}", {{.Explode}}, "{{.ParamName}}", runtime.ParamLocationQuery, {{if .HasOptionalPointer}}*{{end}}params.{{.GoName}}); err != nil {
213+
if queryFrag, err := runtime.StyleParamWithLocation("{{.Style}}", {{.Explode}}, "{{.ParamName}}", runtime.ParamLocationQuery, {{if and .RequiresNilCheck .HasOptionalPointer}}*{{end}}params.{{.GoName}}); err != nil {
214214
return nil, err
215215
} else if parsed, err := url.ParseQuery(queryFrag); err != nil {
216216
return nil, err
@@ -222,7 +222,7 @@ func New{{$opid}}Request{{if .HasBody}}WithBody{{end}}(server string{{genParamAr
222222
}
223223
}
224224
{{end}}
225-
{{if .HasOptionalPointer}}}{{end}}
225+
{{if .RequiresNilCheck}}}{{end}}
226226
{{end}}
227227
queryURL.RawQuery = queryValues.Encode()
228228
}

0 commit comments

Comments
 (0)