From 3533b7f029965905a46539565a5c9ca9e9a13d90 Mon Sep 17 00:00:00 2001 From: Marcin Romaszewicz Date: Tue, 19 May 2026 08:24:45 -0700 Subject: [PATCH] Fix form binding of Nullables Closes: #129 `nullable.Nullable[T]` is defined as `map[bool]T`, but neither `bindFormImpl` nor `BindStringToObjectWithOptions` had a `reflect.Map` branch, so any Nullable field in a form body failed with "error binding string parameter: can not bind to destination of type: map". This change adds Map handling at both layers: - `BindStringToObjectWithOptions` (bindstring.go): a bool-keyed map destination is treated as a Nullable wrapper -- bind `src` into a fresh value of the map's element type, then store it under `map[bool]T{true: value}`. This covers query, path, and header parameters in addition to forms. - `bindFormImpl` (bindform.go): a bool-keyed map field recurses through `bindFormImpl` on the element type and wraps the result, so `Nullable[ComplexStruct]` works alongside scalars. A non-bool-keyed map field routes to a new `bindFormMap` helper that binds entries of the form `name[key]=value` into a generic `map[K]V`. The structural `map[bool]T` check keeps `runtime` decoupled from `github.com/oapi-codegen/nullable`; the data shape is the whole type, so detecting it by reflection is equivalent to importing the package. An absent form field still produces an unspecified (zero) Nullable, matching the pre-fix behavior. Form payloads have no way to encode an explicit null, so `name=` binds as the inner type's zero value (specified), symmetric with non-nullable form binding. Tests cover scalar Nullable, generic `map[K]V`, and the absent-field-stays-unspecified path. Co-Authored-By: Claude Opus 4.7 (1M context) --- bindform.go | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ bindform_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++++++++ bindstring.go | 15 +++++++++ go.mod | 1 + go.sum | 2 ++ 5 files changed, 178 insertions(+) diff --git a/bindform.go b/bindform.go index 121f5d6c..b5885187 100644 --- a/bindform.go +++ b/bindform.go @@ -194,6 +194,26 @@ func bindFormImpl(v reflect.Value, form map[string][]string, files map[string][] hasData = hasData || fieldHasData } return hasData, nil + case reflect.Map: + // A bool-keyed map (such as nullable.Nullable[T], which is + // map[bool]T) is treated as a nullable wrapper: bind the inner type + // and store the result under map[true]. An absent field stays as + // the zero (unspecified) map. + if v.Type().Key().Kind() == reflect.Bool { + valuePtr := reflect.New(v.Type().Elem()) + valueHasData, err := bindFormImpl(valuePtr.Elem(), form, files, name) + if err != nil { + return false, err + } + if valueHasData { + newMap := reflect.MakeMap(v.Type()) + newMap.SetMapIndex(reflect.ValueOf(true), valuePtr.Elem()) + v.Set(newMap) + return true, nil + } + return false, nil + } + return bindFormMap(v, form, files, name) default: value := form[name] if len(value) != 0 { @@ -286,6 +306,66 @@ func bindAdditionalProperties(additionalProperties reflect.Value, parentStruct r return hasData, nil } +// bindFormMap binds form (and file) entries of the form `name[key]=value` +// into a generic map[K]V destination. It is reached from bindFormImpl for +// non-bool-keyed maps; the bool-keyed case is handled separately as a +// nullable wrapper. +func bindFormMap(v reflect.Value, form map[string][]string, files map[string][]*multipart.FileHeader, name string) (bool, error) { + keyType := v.Type().Key() + valueType := v.Type().Elem() + result := reflect.MakeMap(v.Type()) + hasData := false + prefix := name + "[" + seen := map[string]struct{}{} + + process := func(formKey string) error { + if !strings.HasPrefix(formKey, prefix) { + return nil + } + inner := strings.TrimPrefix(formKey, prefix) + end := strings.Index(inner, "]") + if end < 0 { + return nil + } + innerKey := inner[:end] + if _, ok := seen[innerKey]; ok { + return nil + } + seen[innerKey] = struct{}{} + + keyPtr := reflect.New(keyType) + if err := BindStringToObject(innerKey, keyPtr.Interface()); err != nil { + return err + } + valuePtr := reflect.New(valueType) + innerHasData, err := bindFormImpl(valuePtr.Elem(), form, files, fmt.Sprintf("%s[%s]", name, innerKey)) + if err != nil { + return err + } + if innerHasData { + result.SetMapIndex(keyPtr.Elem(), valuePtr.Elem()) + hasData = true + } + return nil + } + + for k := range form { + if err := process(k); err != nil { + return false, err + } + } + for k := range files { + if err := process(k); err != nil { + return false, err + } + } + + if hasData { + v.Set(result) + } + return hasData, nil +} + func marshalFormImpl(v reflect.Value, result url.Values, name string) { switch v.Kind() { case reflect.Ptr: diff --git a/bindform_test.go b/bindform_test.go index 92180a73..df5134b4 100644 --- a/bindform_test.go +++ b/bindform_test.go @@ -6,8 +6,10 @@ import ( "net/url" "testing" + "github.com/oapi-codegen/nullable" "github.com/oapi-codegen/runtime/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestBindURLForm(t *testing.T) { @@ -65,6 +67,84 @@ func TestBindURLForm(t *testing.T) { } } +// TestBindURLFormNullable reproduces oapi-codegen/runtime#129: BindForm fails on +// a nullable.Nullable[T] field because Nullable's underlying type is map[bool]T +// and neither bindFormImpl nor BindStringToObjectWithOptions handles maps. +func TestBindURLFormNullable(t *testing.T) { + type testStruct struct { + Name nullable.Nullable[string] `json:"name"` + Age nullable.Nullable[int] `json:"age"` + } + + t.Run("string value is bound", func(t *testing.T) { + values, err := url.ParseQuery("name=alice") + require.NoError(t, err) + var result testStruct + err = BindForm(&result, values, nil, nil) + require.NoError(t, err) + got, err := result.Name.Get() + require.NoError(t, err) + assert.Equal(t, "alice", got) + }) + + t.Run("int value is bound", func(t *testing.T) { + values, err := url.ParseQuery("age=42") + require.NoError(t, err) + var result testStruct + err = BindForm(&result, values, nil, nil) + require.NoError(t, err) + got, err := result.Age.Get() + require.NoError(t, err) + assert.Equal(t, 42, got) + }) + + t.Run("missing field stays unspecified", func(t *testing.T) { + var result testStruct + err := BindForm(&result, url.Values{}, nil, nil) + require.NoError(t, err) + assert.False(t, result.Name.IsSpecified()) + assert.False(t, result.Age.IsSpecified()) + }) +} + +// TestBindURLFormGenericMap covers binding into a non-Nullable map[K]V field +// using the `name[key]=value` form encoding. +func TestBindURLFormGenericMap(t *testing.T) { + t.Run("string keys, int values", func(t *testing.T) { + type testStruct struct { + Attrs map[string]int `json:"attrs"` + } + values, err := url.ParseQuery("attrs[a]=1&attrs[b]=2") + require.NoError(t, err) + var result testStruct + err = BindForm(&result, values, nil, nil) + require.NoError(t, err) + assert.Equal(t, map[string]int{"a": 1, "b": 2}, result.Attrs) + }) + + t.Run("int keys, string values", func(t *testing.T) { + type testStruct struct { + Labels map[int]string `json:"labels"` + } + values, err := url.ParseQuery("labels[1]=one&labels[2]=two") + require.NoError(t, err) + var result testStruct + err = BindForm(&result, values, nil, nil) + require.NoError(t, err) + assert.Equal(t, map[int]string{1: "one", 2: "two"}, result.Labels) + }) + + t.Run("absent map stays nil", func(t *testing.T) { + type testStruct struct { + Attrs map[string]int `json:"attrs"` + } + var result testStruct + err := BindForm(&result, url.Values{}, nil, nil) + require.NoError(t, err) + assert.Nil(t, result.Attrs) + }) +} + func TestBindMultipartForm(t *testing.T) { var testStruct struct { File types.File `json:"file"` diff --git a/bindstring.go b/bindstring.go index 7aa8f7e7..1a2d0522 100644 --- a/bindstring.go +++ b/bindstring.go @@ -190,6 +190,21 @@ func BindStringToObjectWithOptions(src string, dst interface{}, opts BindStringT // We fall through to the error case below if we haven't handled the // destination type above. fallthrough + case reflect.Map: + // A bool-keyed map (such as nullable.Nullable[T], which is + // map[bool]T) is treated as a nullable wrapper: bind src into a + // fresh value of the inner type and store it under map[true]. + if t.Kind() == reflect.Map && t.Key().Kind() == reflect.Bool { + elemPtr := reflect.New(t.Elem()) + if bindErr := BindStringToObjectWithOptions(src, elemPtr.Interface(), opts); bindErr != nil { + return bindErr + } + newMap := reflect.MakeMap(t) + newMap.SetMapIndex(reflect.ValueOf(true), elemPtr.Elem()) + v.Set(newMap) + return nil + } + fallthrough default: // We've got a bunch of types unimplemented, don't fail silently. err = fmt.Errorf("can not bind to destination of type: %s", t.Kind()) diff --git a/go.mod b/go.mod index bd025ee6..ea3b55c8 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/google/uuid v1.6.0 github.com/kataras/iris/v12 v12.2.11 github.com/labstack/echo/v4 v4.15.1 + github.com/oapi-codegen/nullable v1.1.0 github.com/stretchr/testify v1.11.1 ) diff --git a/go.sum b/go.sum index d746a50d..d838ba67 100644 --- a/go.sum +++ b/go.sum @@ -127,6 +127,8 @@ github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9G github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= +github.com/oapi-codegen/nullable v1.1.0 h1:eAh8JVc5430VtYVnq00Hrbpag9PFRGWLjxR1/3KntMs= +github.com/oapi-codegen/nullable v1.1.0/go.mod h1:KUZ3vUzkmEKY90ksAmit2+5juDIhIZhfDl+0PwOQlFY= github.com/pelletier/go-toml/v2 v2.2.2 h1:aYUidT7k73Pcl9nb2gScu7NSrKCSHIDE89b3+6Wq+LM= github.com/pelletier/go-toml/v2 v2.2.2/go.mod h1:1t835xjRzz80PqgE6HHgN2JOsmgYu/h4qDAS4n929Rs= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=