From e5d8548b34cab786a1b92c4197818914963149b0 Mon Sep 17 00:00:00 2001 From: Robert Terhaar Date: Mon, 13 Apr 2026 01:39:54 -0400 Subject: [PATCH 1/5] add .worktrees/ to .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index f815e09..7137aee 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,4 @@ config.toml **/.claude/settings.local.json CLAUDE.md +.worktrees/ From 0b00d19dc6e6a400cbb6092a6718f17ee1ba7ce3 Mon Sep 17 00:00:00 2001 From: Robert Terhaar Date: Mon, 13 Apr 2026 01:46:51 -0400 Subject: [PATCH 2/5] resolve goroutine leak in Starlark evaluator exec() Every Eval() call spawned a goroutine blocking on <-ctx.Done(). With non-cancellable contexts (e.g., context.Background()), these goroutines leaked unboundedly. Replace with context.AfterFunc which registers a callback and returns a stop function, avoiding the leak. --- engines/starlark/evaluator/evaluator.go | 9 ++++--- engines/starlark/evaluator/evaluator_test.go | 26 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/engines/starlark/evaluator/evaluator.go b/engines/starlark/evaluator/evaluator.go index fe3b5cd..fdf7832 100644 --- a/engines/starlark/evaluator/evaluator.go +++ b/engines/starlark/evaluator/evaluator.go @@ -112,11 +112,12 @@ func (be *Evaluator) exec( }, } - // Set up cancellation from context - go func() { - <-ctx.Done() + // Set up cancellation from context using AfterFunc to avoid goroutine leak + // when context is never cancelled (e.g., context.Background()) + stop := context.AfterFunc(ctx, func() { thread.Cancel(ctx.Err().Error()) - }() + }) + defer stop() // Execute the program finalGlobals, err := prog.Init(thread, globals) diff --git a/engines/starlark/evaluator/evaluator_test.go b/engines/starlark/evaluator/evaluator_test.go index b5104b3..eb181b4 100644 --- a/engines/starlark/evaluator/evaluator_test.go +++ b/engines/starlark/evaluator/evaluator_test.go @@ -6,6 +6,7 @@ import ( "log/slog" "net/http/httptest" "os" + "runtime" "testing" "github.com/robbyt/go-polyscript/engines/starlark/compiler" @@ -215,6 +216,31 @@ invalid_func() }) } +// TestEval_NoGoroutineLeak verifies that Eval() with a non-cancellable context does not leak goroutines +func TestEval_NoGoroutineLeak(t *testing.T) { + t.Parallel() + scriptContent := `_ = 1 + 1` + _, evaluator := evalBuilder(t, scriptContent) + + // Record baseline goroutine count + runtime.GC() + before := runtime.NumGoroutine() + + // Run 100 evaluations with context.Background() (never cancelled) + for range 100 { + ctx := context.WithValue(context.Background(), constants.EvalData, map[string]any{}) + _, err := evaluator.Eval(ctx) + require.NoError(t, err) + } + + // Allow goroutines to settle + runtime.GC() + after := runtime.NumGoroutine() + + growth := after - before + require.Less(t, growth, 5, "goroutine count grew by %d after 100 Eval() calls; suspected leak", growth) +} + // TestEvaluator_AddDataToContext tests the AddDataToContext method with various scenarios func TestEvaluator_AddDataToContext(t *testing.T) { t.Parallel() From df485f7ffe50c8c67d5b674d65105c6eb21124a1 Mon Sep 17 00:00:00 2001 From: Robert Terhaar Date: Mon, 13 Apr 2026 02:03:28 -0400 Subject: [PATCH 3/5] extract shared loadInputData helper to platform/data The loadInputData function was copy-pasted identically across all three engine evaluators (extism, risor, starlark). Extract to a shared data.LoadInputData helper and add a getDataProvider() method to each evaluator that encapsulates nil-safe provider extraction. --- engines/extism/evaluator/evaluator.go | 29 ++++--------- engines/risor/evaluator/evaluator.go | 29 ++++--------- engines/starlark/evaluator/evaluator.go | 29 ++++--------- platform/data/loadInputData.go | 34 +++++++++++++++ platform/data/loadInputData_test.go | 55 +++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 60 deletions(-) create mode 100644 platform/data/loadInputData.go create mode 100644 platform/data/loadInputData_test.go diff --git a/engines/extism/evaluator/evaluator.go b/engines/extism/evaluator/evaluator.go index 5db0297..ec0c443 100644 --- a/engines/extism/evaluator/evaluator.go +++ b/engines/extism/evaluator/evaluator.go @@ -43,29 +43,18 @@ func (be *Evaluator) String() string { return "extism.Evaluator" } +// getDataProvider returns the data provider from the executable unit, or nil if unavailable. +func (be *Evaluator) getDataProvider() data.Provider { + if be.execUnit == nil { + return nil + } + return be.execUnit.GetDataProvider() +} + // loadInputData retrieves input data using the data provider in the executable unit. // Returns a map that will be used as input for the WASM module. func (be *Evaluator) loadInputData(ctx context.Context) (map[string]any, error) { - logger := be.logger.WithGroup("loadInputData") - - // If no executable unit or data provider, return empty map - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - logger.WarnContext(ctx, "no data provider available, using empty data") - return make(map[string]any), nil - } - - // Get input data from provider - inputData, err := be.execUnit.GetDataProvider().GetData(ctx) - if err != nil { - logger.ErrorContext(ctx, "failed to get input data from provider", "error", err) - return nil, err - } - - if len(inputData) == 0 { - logger.WarnContext(ctx, "empty input data returned from provider") - } - logger.DebugContext(ctx, "input data loaded from provider", "inputData", inputData) - return inputData, nil + return data.LoadInputData(ctx, be.logger.WithGroup("loadInputData"), be.getDataProvider()) } // execHelper is a utility function to handle common execution logic diff --git a/engines/risor/evaluator/evaluator.go b/engines/risor/evaluator/evaluator.go index 753bc40..eeaf7b2 100644 --- a/engines/risor/evaluator/evaluator.go +++ b/engines/risor/evaluator/evaluator.go @@ -52,29 +52,18 @@ func (be *Evaluator) String() string { return "risor.Evaluator" } +// getDataProvider returns the data provider from the executable unit, or nil if unavailable. +func (be *Evaluator) getDataProvider() data.Provider { + if be.execUnit == nil { + return nil + } + return be.execUnit.GetDataProvider() +} + // loadInputData retrieves input data using the data provider in the executable unit. // Returns a map that will be used as input for the Risor engine. func (be *Evaluator) loadInputData(ctx context.Context) (map[string]any, error) { - logger := be.logger.WithGroup("loadInputData") - - // If no executable unit or data provider, return empty map - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - logger.WarnContext(ctx, "no data provider available, using empty data") - return make(map[string]any), nil - } - - // Get input data from provider - inputData, err := be.execUnit.GetDataProvider().GetData(ctx) - if err != nil { - logger.ErrorContext(ctx, "failed to get input data from provider", "error", err) - return nil, err - } - - if len(inputData) == 0 { - logger.WarnContext(ctx, "empty input data returned from provider") - } - logger.DebugContext(ctx, "input data loaded from provider", "inputData", inputData) - return inputData, nil + return data.LoadInputData(ctx, be.logger.WithGroup("loadInputData"), be.getDataProvider()) } // exec runs the bytecode with the provided environment map diff --git a/engines/starlark/evaluator/evaluator.go b/engines/starlark/evaluator/evaluator.go index fdf7832..56aad72 100644 --- a/engines/starlark/evaluator/evaluator.go +++ b/engines/starlark/evaluator/evaluator.go @@ -54,29 +54,18 @@ func (be *Evaluator) String() string { return "starlark.Evaluator" } +// getDataProvider returns the data provider from the executable unit, or nil if unavailable. +func (be *Evaluator) getDataProvider() data.Provider { + if be.execUnit == nil { + return nil + } + return be.execUnit.GetDataProvider() +} + // loadInputData retrieves input data using the data provider in the executable unit. // Returns a map that will be used as input for the Starlark engine. func (be *Evaluator) loadInputData(ctx context.Context) (map[string]any, error) { - logger := be.logger.WithGroup("loadInputData") - - // If no executable unit or data provider, return empty map - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - logger.WarnContext(ctx, "no data provider available, using empty data") - return make(map[string]any), nil - } - - // Get input data from provider - inputData, err := be.execUnit.GetDataProvider().GetData(ctx) - if err != nil { - logger.ErrorContext(ctx, "failed to get input data from provider", "error", err) - return nil, err - } - - if len(inputData) == 0 { - logger.WarnContext(ctx, "empty input data returned from provider") - } - logger.DebugContext(ctx, "input data loaded from provider", "inputData", inputData) - return inputData, nil + return data.LoadInputData(ctx, be.logger.WithGroup("loadInputData"), be.getDataProvider()) } // prepareGlobals merges the universe and input globals into a single Starlark dictionary diff --git a/platform/data/loadInputData.go b/platform/data/loadInputData.go new file mode 100644 index 0000000..f7fa448 --- /dev/null +++ b/platform/data/loadInputData.go @@ -0,0 +1,34 @@ +package data + +import ( + "context" + "log/slog" +) + +// LoadInputData retrieves input data using the given data provider. +// If the provider is nil, it returns an empty map. This function consolidates +// the common data-loading logic used across all engine evaluators. +func LoadInputData( + ctx context.Context, + logger *slog.Logger, + provider Provider, +) (map[string]any, error) { + // If no data provider, return empty map + if provider == nil { + logger.WarnContext(ctx, "no data provider available, using empty data") + return make(map[string]any), nil + } + + // Get input data from provider + inputData, err := provider.GetData(ctx) + if err != nil { + logger.ErrorContext(ctx, "failed to get input data from provider", "error", err) + return nil, err + } + + if len(inputData) == 0 { + logger.WarnContext(ctx, "empty input data returned from provider") + } + logger.DebugContext(ctx, "input data loaded from provider", "inputData", inputData) + return inputData, nil +} diff --git a/platform/data/loadInputData_test.go b/platform/data/loadInputData_test.go new file mode 100644 index 0000000..a317593 --- /dev/null +++ b/platform/data/loadInputData_test.go @@ -0,0 +1,55 @@ +package data + +import ( + "log/slog" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func TestLoadInputData(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(os.Stderr, nil)) + + t.Run("nil provider returns empty map", func(t *testing.T) { + result, err := LoadInputData(t.Context(), logger, nil) + require.NoError(t, err) + assert.Empty(t, result) + assert.NotNil(t, result) + }) + + t.Run("provider returns data", func(t *testing.T) { + provider := new(MockProvider) + expected := map[string]any{"key": "value", "count": 42} + provider.On("GetData", mock.Anything).Return(expected, nil) + + result, err := LoadInputData(t.Context(), logger, provider) + require.NoError(t, err) + assert.Equal(t, expected, result) + provider.AssertExpectations(t) + }) + + t.Run("provider returns error", func(t *testing.T) { + provider := new(MockProvider) + provider.On("GetData", mock.Anything).Return(nil, assert.AnError) + + result, err := LoadInputData(t.Context(), logger, provider) + require.Error(t, err) + assert.Nil(t, result) + provider.AssertExpectations(t) + }) + + t.Run("provider returns empty map", func(t *testing.T) { + provider := new(MockProvider) + provider.On("GetData", mock.Anything).Return(map[string]any{}, nil) + + result, err := LoadInputData(t.Context(), logger, provider) + require.NoError(t, err) + assert.Empty(t, result) + provider.AssertExpectations(t) + }) +} From 6ae19df7a029bcfab8b4cb681036cc75b0be9528 Mon Sep 17 00:00:00 2001 From: Robert Terhaar Date: Mon, 13 Apr 2026 02:04:11 -0400 Subject: [PATCH 4/5] consolidate AddDataToContext boilerplate across engines All three evaluators duplicated the same nil-check + delegation pattern for AddDataToContext. Add data.AddDataToContextFromProvider as a convenience wrapper and simplify all evaluator methods to use it via the shared getDataProvider() method. --- engines/extism/evaluator/evaluator.go | 14 +----------- engines/risor/evaluator/evaluator.go | 14 +----------- engines/starlark/evaluator/evaluator.go | 14 +----------- platform/data/addDataToContext.go | 15 +++++++++++++ platform/data/addDataToContext_test.go | 30 +++++++++++++++++++++++++ 5 files changed, 48 insertions(+), 39 deletions(-) diff --git a/engines/extism/evaluator/evaluator.go b/engines/extism/evaluator/evaluator.go index ec0c443..19f4087 100644 --- a/engines/extism/evaluator/evaluator.go +++ b/engines/extism/evaluator/evaluator.go @@ -203,17 +203,5 @@ func (be *Evaluator) AddDataToContext( ctx context.Context, d ...map[string]any, ) (context.Context, error) { - logger := be.logger.WithGroup("AddDataToContext") - - // Use the shared helper function for context preparation - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - return ctx, fmt.Errorf("no data provider available") - } - - return data.AddDataToContextHelper( - ctx, - logger, - be.execUnit.GetDataProvider(), - d..., - ) + return data.AddDataToContextFromProvider(ctx, be.logger.WithGroup("AddDataToContext"), be.getDataProvider(), d...) } diff --git a/engines/risor/evaluator/evaluator.go b/engines/risor/evaluator/evaluator.go index eeaf7b2..1cd5ee6 100644 --- a/engines/risor/evaluator/evaluator.go +++ b/engines/risor/evaluator/evaluator.go @@ -161,17 +161,5 @@ func (be *Evaluator) AddDataToContext( ctx context.Context, d ...map[string]any, ) (context.Context, error) { - logger := be.logger.WithGroup("AddDataToContext") - - // Use the shared helper function for context preparation - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - return ctx, fmt.Errorf("no data provider available") - } - - return data.AddDataToContextHelper( - ctx, - logger, - be.execUnit.GetDataProvider(), - d..., - ) + return data.AddDataToContextFromProvider(ctx, be.logger.WithGroup("AddDataToContext"), be.getDataProvider(), d...) } diff --git a/engines/starlark/evaluator/evaluator.go b/engines/starlark/evaluator/evaluator.go index 56aad72..4842605 100644 --- a/engines/starlark/evaluator/evaluator.go +++ b/engines/starlark/evaluator/evaluator.go @@ -218,17 +218,5 @@ func (be *Evaluator) AddDataToContext( ctx context.Context, d ...map[string]any, ) (context.Context, error) { - logger := be.logger.WithGroup("AddDataToContext") - - // Use the shared helper function for context preparation - if be.execUnit == nil || be.execUnit.GetDataProvider() == nil { - return ctx, fmt.Errorf("no data provider available") - } - - return data.AddDataToContextHelper( - ctx, - logger, - be.execUnit.GetDataProvider(), - d..., - ) + return data.AddDataToContextFromProvider(ctx, be.logger.WithGroup("AddDataToContext"), be.getDataProvider(), d...) } diff --git a/platform/data/addDataToContext.go b/platform/data/addDataToContext.go index d9f3310..4f2c307 100644 --- a/platform/data/addDataToContext.go +++ b/platform/data/addDataToContext.go @@ -43,3 +43,18 @@ func AddDataToContextHelper( return enrichedCtx, err } + +// AddDataToContextFromProvider is a convenience wrapper that handles a nil provider +// by returning a clear error, then delegates to AddDataToContextHelper. +// This consolidates the nil-check + delegation pattern duplicated across all engine evaluators. +func AddDataToContextFromProvider( + ctx context.Context, + logger *slog.Logger, + provider Provider, + d ...map[string]any, +) (context.Context, error) { + if provider == nil { + return ctx, fmt.Errorf("no data provider available") + } + return AddDataToContextHelper(ctx, logger, provider, d...) +} diff --git a/platform/data/addDataToContext_test.go b/platform/data/addDataToContext_test.go index 17c7fac..3e932a3 100644 --- a/platform/data/addDataToContext_test.go +++ b/platform/data/addDataToContext_test.go @@ -174,6 +174,36 @@ func TestAddDataToContextHelper(t *testing.T) { }) } +// TestAddDataToContextFromProvider tests the convenience wrapper +func TestAddDataToContextFromProvider(t *testing.T) { + t.Parallel() + + logger := slog.Default() + + t.Run("nil provider returns error", func(t *testing.T) { + ctx := t.Context() + result, err := AddDataToContextFromProvider(ctx, logger, nil, map[string]any{"key": "value"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no data provider available") + assert.Equal(t, ctx, result) + }) + + t.Run("valid provider delegates to helper", func(t *testing.T) { + provider := NewContextProvider(constants.EvalData) + ctx := t.Context() + + result, err := AddDataToContextFromProvider(ctx, logger, provider, map[string]any{"key": "value"}) + require.NoError(t, err) + assert.NotEqual(t, ctx, result) + + data := result.Value(constants.EvalData) + require.NotNil(t, data) + contextMap, ok := data.(map[string]any) + require.True(t, ok) + assert.Equal(t, "value", contextMap["key"]) + }) +} + // TestAddDataToContextWithErrorHandling tests error propagation in the AddDataToContextHelper func TestAddDataToContextWithErrorHandling(t *testing.T) { t.Parallel() From 9955cae2a7f3a74aa6f3dba0b70824d76a6d918e Mon Sep 17 00:00:00 2001 From: Robert Terhaar Date: Mon, 13 Apr 2026 02:06:25 -0400 Subject: [PATCH 5/5] remove fragile field-name heuristic from FixJSONNumberTypes The old implementation used field-name suffixes (_id, Id, _count, count) to decide whether a json.Number should become int or float64. This was surprising and fragile. Now all json.Number values use a simple rule: try int first, fall back to float64. Also fixes bare json.Number values in slices which were previously left unconverted. --- engines/extism/internal/jsonHelpers.go | 49 ++--- engines/extism/internal/jsonHelpers_test.go | 212 +++++++++++-------- engines/starlark/evaluator/evaluator_test.go | 4 +- 3 files changed, 147 insertions(+), 118 deletions(-) diff --git a/engines/extism/internal/jsonHelpers.go b/engines/extism/internal/jsonHelpers.go index 0e34b4b..2b8affc 100644 --- a/engines/extism/internal/jsonHelpers.go +++ b/engines/extism/internal/jsonHelpers.go @@ -2,48 +2,39 @@ package internal import ( "encoding/json" - "strings" ) -// FixJSONNumberTypes converts json.Number values to appropriate Go types based on semantic rules +// convertJSONNumber converts a json.Number to an int (if it fits) or float64. +func convertJSONNumber(num json.Number) any { + if n, err := num.Int64(); err == nil { + return int(n) + } + if n, err := num.Float64(); err == nil { + return n + } + return num +} + +// FixJSONNumberTypes converts json.Number values to appropriate Go types. +// Integer-representable numbers become int; all others become float64. func FixJSONNumberTypes(data any) any { switch v := data.(type) { case map[string]any: - // Process each key in the map for k, val := range v { - // Handle nested structures recursively - if nestedMap, ok := val.(map[string]any); ok { - v[k] = FixJSONNumberTypes(nestedMap) - continue - } - - if nestedSlice, ok := val.([]any); ok { - v[k] = FixJSONNumberTypes(nestedSlice) - continue - } - - // Convert json.Number to appropriate type if num, ok := val.(json.Number); ok { - // Fields that should be integers - if strings.HasSuffix(k, "_count") || k == "count" || - strings.HasSuffix(k, "_id") || strings.HasSuffix(k, "Id") { - if n, err := num.Int64(); err == nil { - v[k] = int(n) - } - continue - } - - // Default to float64 for other numeric fields - if n, err := num.Float64(); err == nil { - v[k] = n - } + v[k] = convertJSONNumber(num) + continue } + v[k] = FixJSONNumberTypes(val) } return v case []any: - // Process each item in the slice for i, item := range v { + if num, ok := item.(json.Number); ok { + v[i] = convertJSONNumber(num) + continue + } v[i] = FixJSONNumberTypes(item) } return v diff --git a/engines/extism/internal/jsonHelpers_test.go b/engines/extism/internal/jsonHelpers_test.go index db4c096..3fefaf8 100644 --- a/engines/extism/internal/jsonHelpers_test.go +++ b/engines/extism/internal/jsonHelpers_test.go @@ -8,64 +8,45 @@ import ( ) func TestFixJSONNumberTypes(t *testing.T) { + t.Parallel() + t.Run("handles nil input", func(t *testing.T) { result := FixJSONNumberTypes(nil) assert.Nil(t, result) }) t.Run("handles primitive types", func(t *testing.T) { - // Test string - str := "test string" - result := FixJSONNumberTypes(str) - assert.Equal(t, str, result) - - // Test bool - boolVal := true - result = FixJSONNumberTypes(boolVal) - assert.Equal(t, boolVal, result) + assert.Equal(t, "test string", FixJSONNumberTypes("test string")) + assert.Equal(t, true, FixJSONNumberTypes(true)) + assert.Equal(t, 42, FixJSONNumberTypes(42)) }) - t.Run("converts count fields to integers", func(t *testing.T) { - // Create a map with json.Number values + t.Run("converts integer numbers to int regardless of field name", func(t *testing.T) { data := map[string]any{ "item_count": json.Number("42"), "count": json.Number("100"), + "user_id": json.Number("123"), + "productId": json.Number("456"), + "quantity": json.Number("7"), + "status": json.Number("0"), } result := FixJSONNumberTypes(data) mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") - - // Check that the values were converted to integers - assert.Equal(t, int(42), mapResult["item_count"]) - assert.Equal(t, int(100), mapResult["count"]) - assert.IsType(t, int(0), mapResult["item_count"]) - assert.IsType(t, int(0), mapResult["count"]) - }) + assert.True(t, ok) - t.Run("converts ID fields to integers", func(t *testing.T) { - // Create a map with json.Number values - data := map[string]any{ - "user_id": json.Number("123"), - "productId": json.Number("456"), - "category_id": json.Number("789"), + for k, v := range mapResult { + assert.IsType(t, int(0), v, "field %q should be int", k) } - - result := FixJSONNumberTypes(data) - mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") - - // Check that the values were converted to integers - assert.Equal(t, int(123), mapResult["user_id"]) - assert.Equal(t, int(456), mapResult["productId"]) - assert.Equal(t, int(789), mapResult["category_id"]) - assert.IsType(t, int(0), mapResult["user_id"]) - assert.IsType(t, int(0), mapResult["productId"]) - assert.IsType(t, int(0), mapResult["category_id"]) + assert.Equal(t, 42, mapResult["item_count"]) + assert.Equal(t, 100, mapResult["count"]) + assert.Equal(t, 123, mapResult["user_id"]) + assert.Equal(t, 456, mapResult["productId"]) + assert.Equal(t, 7, mapResult["quantity"]) + assert.Equal(t, 0, mapResult["status"]) }) - t.Run("converts other number fields to float64", func(t *testing.T) { - // Create a map with json.Number values for non-integer fields + t.Run("converts decimal numbers to float64", func(t *testing.T) { data := map[string]any{ "price": json.Number("19.99"), "rating": json.Number("4.5"), @@ -74,19 +55,17 @@ func TestFixJSONNumberTypes(t *testing.T) { result := FixJSONNumberTypes(data) mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") + assert.True(t, ok) - // Check that the values were converted to float64 - assert.InDelta(t, float64(19.99), mapResult["price"], 0.0001) - assert.InDelta(t, float64(4.5), mapResult["rating"], 0.0001) - assert.InDelta(t, float64(75.5), mapResult["percentage"], 0.0001) + assert.InDelta(t, 19.99, mapResult["price"], 0.0001) + assert.InDelta(t, 4.5, mapResult["rating"], 0.0001) + assert.InDelta(t, 75.5, mapResult["percentage"], 0.0001) assert.IsType(t, float64(0), mapResult["price"]) assert.IsType(t, float64(0), mapResult["rating"]) assert.IsType(t, float64(0), mapResult["percentage"]) }) t.Run("handles nested maps", func(t *testing.T) { - // Create a map with nested maps data := map[string]any{ "user": map[string]any{ "user_id": json.Number("123"), @@ -100,24 +79,21 @@ func TestFixJSONNumberTypes(t *testing.T) { result := FixJSONNumberTypes(data) mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") + assert.True(t, ok) - // Check top level integer conversion - assert.Equal(t, int(10), mapResult["item_count"]) + assert.Equal(t, 10, mapResult["item_count"]) - // Check nested map conversions user, ok := mapResult["user"].(map[string]any) - assert.True(t, ok, "user should be a map") - assert.Equal(t, int(123), user["user_id"]) + assert.True(t, ok) + assert.Equal(t, 123, user["user_id"]) stats, ok := user["stats"].(map[string]any) - assert.True(t, ok, "stats should be a map") - assert.Equal(t, int(42), stats["login_count"]) - assert.InDelta(t, float64(95.5), stats["score"], 0.0001) + assert.True(t, ok) + assert.Equal(t, 42, stats["login_count"]) + assert.InDelta(t, 95.5, stats["score"], 0.0001) }) - t.Run("handles slices", func(t *testing.T) { - // Create a slice with json.Number values + t.Run("converts numbers in slices", func(t *testing.T) { data := []any{ json.Number("1"), json.Number("2.5"), @@ -130,23 +106,23 @@ func TestFixJSONNumberTypes(t *testing.T) { result := FixJSONNumberTypes(data) sliceResult, ok := result.([]any) - assert.True(t, ok, "Result should be a slice") + assert.True(t, ok) - // Values in the slice should remain as json.Number - // since they don't have field names to determine type - assert.IsType(t, json.Number(""), sliceResult[0]) - assert.IsType(t, json.Number(""), sliceResult[1]) + // Numbers in slices are now converted + assert.Equal(t, 1, sliceResult[0]) + assert.IsType(t, int(0), sliceResult[0]) + assert.InDelta(t, 2.5, sliceResult[1], 0.0001) + assert.IsType(t, float64(0), sliceResult[1]) assert.Equal(t, "test", sliceResult[2]) - // Check the map in the slice + // Nested map in slice itemMap, ok := sliceResult[3].(map[string]any) - assert.True(t, ok, "Item at index 3 should be a map") - assert.Equal(t, int(123), itemMap["item_id"]) - assert.InDelta(t, float64(9.99), itemMap["price"], 0.0001) + assert.True(t, ok) + assert.Equal(t, 123, itemMap["item_id"]) + assert.InDelta(t, 9.99, itemMap["price"], 0.0001) }) - t.Run("handles nested slices", func(t *testing.T) { - // Create a map with nested slices + t.Run("handles nested slices in maps", func(t *testing.T) { data := map[string]any{ "products": []any{ map[string]any{ @@ -163,42 +139,104 @@ func TestFixJSONNumberTypes(t *testing.T) { result := FixJSONNumberTypes(data) mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") + assert.True(t, ok) - // Check top level count conversion - assert.Equal(t, int(2), mapResult["total_count"]) + assert.Equal(t, 2, mapResult["total_count"]) - // Check nested slice products, ok := mapResult["products"].([]any) - assert.True(t, ok, "products should be a slice") + assert.True(t, ok) assert.Len(t, products, 2) - // Check first product product1, ok := products[0].(map[string]any) - assert.True(t, ok, "First product should be a map") - assert.Equal(t, int(1), product1["product_id"]) - assert.InDelta(t, float64(19.99), product1["price"], 0.0001) + assert.True(t, ok) + assert.Equal(t, 1, product1["product_id"]) + assert.InDelta(t, 19.99, product1["price"], 0.0001) - // Check second product product2, ok := products[1].(map[string]any) - assert.True(t, ok, "Second product should be a map") - assert.Equal(t, int(2), product2["product_id"]) - assert.InDelta(t, float64(29.99), product2["price"], 0.0001) + assert.True(t, ok) + assert.Equal(t, 2, product2["product_id"]) + assert.InDelta(t, 29.99, product2["price"], 0.0001) }) t.Run("handles invalid numbers gracefully", func(t *testing.T) { - // Create a map with invalid json.Number data := map[string]any{ - "item_count": json.Number("not-a-number"), - "price": json.Number("also-not-a-number"), + "bad_int": json.Number("not-a-number"), + "bad_float": json.Number("also-not-a-number"), } result := FixJSONNumberTypes(data) mapResult, ok := result.(map[string]any) - assert.True(t, ok, "Result should be a map") + assert.True(t, ok) - // Values should remain as json.Number since conversion failed - assert.IsType(t, json.Number(""), mapResult["item_count"]) - assert.IsType(t, json.Number(""), mapResult["price"]) + // Invalid numbers remain as json.Number + assert.IsType(t, json.Number(""), mapResult["bad_int"]) + assert.IsType(t, json.Number(""), mapResult["bad_float"]) }) + + t.Run("handles large integers", func(t *testing.T) { + data := map[string]any{ + "big": json.Number("9223372036854775807"), // max int64 + } + + result := FixJSONNumberTypes(data) + mapResult, ok := result.(map[string]any) + assert.True(t, ok) + assert.Equal(t, 9223372036854775807, mapResult["big"]) + assert.IsType(t, int(0), mapResult["big"]) + }) + + t.Run("handles mixed types in map", func(t *testing.T) { + data := map[string]any{ + "name": "test", + "active": true, + "count": json.Number("5"), + "rate": json.Number("3.14"), + "tags": []any{"a", "b"}, + "nested": map[string]any{"x": json.Number("1")}, + "nothing": nil, + } + + result := FixJSONNumberTypes(data) + mapResult, ok := result.(map[string]any) + assert.True(t, ok) + + assert.Equal(t, "test", mapResult["name"]) + assert.Equal(t, true, mapResult["active"]) + assert.Equal(t, 5, mapResult["count"]) + assert.InDelta(t, 3.14, mapResult["rate"], 0.0001) + assert.Equal(t, []any{"a", "b"}, mapResult["tags"]) + nested, ok := mapResult["nested"].(map[string]any) + assert.True(t, ok) + assert.Equal(t, 1, nested["x"]) + assert.Nil(t, mapResult["nothing"]) + }) +} + +func TestConvertJSONNumber(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input json.Number + expected any + }{ + {"integer", json.Number("42"), int(42)}, + {"zero", json.Number("0"), int(0)}, + {"negative integer", json.Number("-10"), int(-10)}, + {"float", json.Number("3.14"), float64(3.14)}, + {"negative float", json.Number("-2.5"), float64(-2.5)}, + {"invalid", json.Number("xyz"), json.Number("xyz")}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + result := convertJSONNumber(tc.input) + if f, ok := tc.expected.(float64); ok { + assert.InDelta(t, f, result, 0.0001) + } else { + assert.Equal(t, tc.expected, result) + } + }) + } } diff --git a/engines/starlark/evaluator/evaluator_test.go b/engines/starlark/evaluator/evaluator_test.go index eb181b4..17b1bea 100644 --- a/engines/starlark/evaluator/evaluator_test.go +++ b/engines/starlark/evaluator/evaluator_test.go @@ -226,9 +226,9 @@ func TestEval_NoGoroutineLeak(t *testing.T) { runtime.GC() before := runtime.NumGoroutine() - // Run 100 evaluations with context.Background() (never cancelled) + // Run 100 evaluations with a context that won't be cancelled during Eval() for range 100 { - ctx := context.WithValue(context.Background(), constants.EvalData, map[string]any{}) + ctx := context.WithValue(t.Context(), constants.EvalData, map[string]any{}) _, err := evaluator.Eval(ctx) require.NoError(t, err) }