From 86798a61788ea548ec4e4ac6a4d68d68071e632e Mon Sep 17 00:00:00 2001 From: Ville Vesilehto Date: Sat, 25 Jan 2025 20:48:31 +0200 Subject: [PATCH 1/4] fix: handle string pointer in fromJSON builtin Support *string input in fromJSON builtin function by dereferencing pointer before JSON unmarshaling. Added tests covering both direct string and pointer string use cases, including toJSON, which already supports this. Signed-off-by: Ville Vesilehto --- builtin/builtin.go | 6 ++++- builtin/builtin_test.go | 52 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/builtin/builtin.go b/builtin/builtin.go index 35a9f687e..4375f9944 100644 --- a/builtin/builtin.go +++ b/builtin/builtin.go @@ -443,7 +443,11 @@ var Builtins = []*Function{ Name: "fromJSON", Func: func(args ...any) (any, error) { var v any - err := json.Unmarshal([]byte(args[0].(string)), &v) + jsonStr := args[0] + if strPtr, ok := jsonStr.(*string); ok { + jsonStr = *strPtr + } + err := json.Unmarshal([]byte(jsonStr.(string)), &v) if err != nil { return nil, err } diff --git a/builtin/builtin_test.go b/builtin/builtin_test.go index 499a838b7..5166c225c 100644 --- a/builtin/builtin_test.go +++ b/builtin/builtin_test.go @@ -638,3 +638,55 @@ func Test_int_unwraps_underlying_value(t *testing.T) { require.NoError(t, err) assert.Equal(t, true, out) } + +func TestBuiltin_json(t *testing.T) { + t.Run("fromJSON/string", func(t *testing.T) { + env := map[string]any{ + "json": `{"foo": "bar"}`, + } + program, err := expr.Compile(`fromJSON(json)`, expr.Env(env)) + require.NoError(t, err) + + out, err := expr.Run(program, env) + require.NoError(t, err) + assert.Equal(t, map[string]any{"foo": "bar"}, out) + }) + + t.Run("fromJSON/string pointer", func(t *testing.T) { + jsonString := `{"foo": "bar"}` + env := map[string]any{ + "json": &jsonString, + } + program, err := expr.Compile(`fromJSON(json)`, expr.Env(env)) + require.NoError(t, err) + + out, err := expr.Run(program, env) + require.NoError(t, err) + assert.Equal(t, map[string]any{"foo": "bar"}, out) + }) + + t.Run("toJSON/object", func(t *testing.T) { + env := map[string]any{ + "obj": map[string]any{"foo": "bar"}, + } + program, err := expr.Compile(`toJSON(obj)`, expr.Env(env)) + require.NoError(t, err) + + out, err := expr.Run(program, env) + require.NoError(t, err) + assert.Equal(t, "{\n \"foo\": \"bar\"\n}", out) + }) + + t.Run("toJSON/object pointer", func(t *testing.T) { + obj := map[string]any{"foo": "bar"} + env := map[string]any{ + "obj": &obj, + } + program, err := expr.Compile(`toJSON(obj)`, expr.Env(env)) + require.NoError(t, err) + + out, err := expr.Run(program, env) + require.NoError(t, err) + assert.Equal(t, "{\n \"foo\": \"bar\"\n}", out) + }) +} From 846b31008d956cdbc0aa60385edac84e85eecf07 Mon Sep 17 00:00:00 2001 From: Ville Vesilehto Date: Mon, 27 Jan 2025 21:01:34 +0200 Subject: [PATCH 2/4] fix: add pointer-string support to fromJSON & preserve interfaces This commit enables fromJSON to accept both string and *string arguments by performing a runtime check on the provided argument. It also refactors the deref.Type function to preserve interface types, ensuring methods sets are not lost when unwrapping pointers. Changes: - fromJSON now handles `string` and `*string` distinctly, returning an error if the pointer is nil or if an unsupported type is given. - deref.Type preserves the Kind() == reflect.Interface immediately instead of unwrapping further, which prevents losing interface method sets. - Updated unit tests to confirm that interface-wrapped pointers are unwrapped correctly (e.g., reflect.String) and interface types remain interfaces. Signed-off-by: Ville Vesilehto --- builtin/builtin.go | 20 +++++++++++++++----- internal/deref/deref.go | 19 +++++++++++++++++++ internal/deref/deref_test.go | 17 +++++++++++++++++ 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/builtin/builtin.go b/builtin/builtin.go index 4375f9944..b80b77961 100644 --- a/builtin/builtin.go +++ b/builtin/builtin.go @@ -443,17 +443,27 @@ var Builtins = []*Function{ Name: "fromJSON", Func: func(args ...any) (any, error) { var v any - jsonStr := args[0] - if strPtr, ok := jsonStr.(*string); ok { - jsonStr = *strPtr + var jsonStr string + + switch arg := args[0].(type) { + case string: + jsonStr = arg + case *string: + if arg == nil { + return nil, fmt.Errorf("nil string pointer") + } + jsonStr = *arg + default: + return nil, fmt.Errorf("expected string or *string, got %T", args[0]) } - err := json.Unmarshal([]byte(jsonStr.(string)), &v) + + err := json.Unmarshal([]byte(jsonStr), &v) if err != nil { return nil, err } return v, nil }, - Types: types(new(func(string) any)), + Types: types(new(func(string) any), new(func(*string) any)), }, { Name: "toBase64", diff --git a/internal/deref/deref.go b/internal/deref/deref.go index acdc89811..06c0900bb 100644 --- a/internal/deref/deref.go +++ b/internal/deref/deref.go @@ -30,9 +30,28 @@ func Type(t reflect.Type) reflect.Type { if t == nil { return nil } + + // Preserve interface types immediately to maintain type information + // This handles both empty (interface{}) and non-empty (e.g., io.Reader) interfaces + if t.Kind() == reflect.Interface { + return t + } + + // Iteratively unwrap pointer types until we reach a non-pointer + // or encounter an interface type that needs preservation for t.Kind() == reflect.Ptr { t = t.Elem() + if t == nil { + return nil + } + // Stop unwrapping if we hit an interface type to preserve its type information + // This ensures interface method sets are not lost + if t.Kind() == reflect.Interface { + return t + } } + + // Return the final unwrapped type, which could be any non-pointer, non-interface type return t } diff --git a/internal/deref/deref_test.go b/internal/deref/deref_test.go index 5f812bee5..d9e31da31 100644 --- a/internal/deref/deref_test.go +++ b/internal/deref/deref_test.go @@ -67,6 +67,23 @@ func TestType_nil(t *testing.T) { assert.Nil(t, deref.Type(nil)) } +func TestType_interface_wrapped_pointer(t *testing.T) { + t.Run("one level", func(t *testing.T) { + str := "hello" + var iface any = &str + dt := deref.Type(reflect.TypeOf(iface)) + assert.Equal(t, reflect.String, dt.Kind()) + }) + + t.Run("two levels", func(t *testing.T) { + str := "hello" + strPtr := &str + var iface any = &strPtr + dt := deref.Type(reflect.TypeOf(iface)) + assert.Equal(t, reflect.String, dt.Kind()) + }) +} + func TestValue(t *testing.T) { a := uint(42) b := &a From 9827937b080444711ae9f56ae8d91783e9d15157 Mon Sep 17 00:00:00 2001 From: Ville Vesilehto Date: Mon, 27 Jan 2025 22:18:47 +0200 Subject: [PATCH 3/4] revert: deref changes --- internal/deref/deref.go | 19 ------------------- internal/deref/deref_test.go | 17 ----------------- 2 files changed, 36 deletions(-) diff --git a/internal/deref/deref.go b/internal/deref/deref.go index 06c0900bb..acdc89811 100644 --- a/internal/deref/deref.go +++ b/internal/deref/deref.go @@ -30,28 +30,9 @@ func Type(t reflect.Type) reflect.Type { if t == nil { return nil } - - // Preserve interface types immediately to maintain type information - // This handles both empty (interface{}) and non-empty (e.g., io.Reader) interfaces - if t.Kind() == reflect.Interface { - return t - } - - // Iteratively unwrap pointer types until we reach a non-pointer - // or encounter an interface type that needs preservation for t.Kind() == reflect.Ptr { t = t.Elem() - if t == nil { - return nil - } - // Stop unwrapping if we hit an interface type to preserve its type information - // This ensures interface method sets are not lost - if t.Kind() == reflect.Interface { - return t - } } - - // Return the final unwrapped type, which could be any non-pointer, non-interface type return t } diff --git a/internal/deref/deref_test.go b/internal/deref/deref_test.go index d9e31da31..5f812bee5 100644 --- a/internal/deref/deref_test.go +++ b/internal/deref/deref_test.go @@ -67,23 +67,6 @@ func TestType_nil(t *testing.T) { assert.Nil(t, deref.Type(nil)) } -func TestType_interface_wrapped_pointer(t *testing.T) { - t.Run("one level", func(t *testing.T) { - str := "hello" - var iface any = &str - dt := deref.Type(reflect.TypeOf(iface)) - assert.Equal(t, reflect.String, dt.Kind()) - }) - - t.Run("two levels", func(t *testing.T) { - str := "hello" - strPtr := &str - var iface any = &strPtr - dt := deref.Type(reflect.TypeOf(iface)) - assert.Equal(t, reflect.String, dt.Kind()) - }) -} - func TestValue(t *testing.T) { a := uint(42) b := &a From 5a3ae045a253df4051615042b8880d8af04b1882 Mon Sep 17 00:00:00 2001 From: Ville Vesilehto Date: Mon, 27 Jan 2025 22:56:46 +0200 Subject: [PATCH 4/4] fix: go already deref's the string ptr --- builtin/builtin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/builtin.go b/builtin/builtin.go index b80b77961..8dd37acc3 100644 --- a/builtin/builtin.go +++ b/builtin/builtin.go @@ -463,7 +463,7 @@ var Builtins = []*Function{ } return v, nil }, - Types: types(new(func(string) any), new(func(*string) any)), + Types: types(new(func(string) any)), }, { Name: "toBase64",