From b9212a46fbdb47fcc27624e510be1b457194fa78 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 24 Aug 2026 11:34:04 +0200 Subject: [PATCH 1/2] Go: a recipe option binds to its field's declared type Every non-string option on every Go recipe was unreachable from the Moderne CLI. `mod run --recipe=org.openrewrite.golang.AddImport -PonlyIfReferenced=false` failed with `Internal error: reflect.Set: value of type string is not assignable to type bool`, for both `true` and `false`. The registry bound options by `f.Set(reflect.ValueOf(val))` with no conversion. `val` is whatever `encoding/json` decoded into `map[string]any`, and the CLI declares `-P` as picocli `Map`, so every command-line option arrives as a string whatever its declared type. A JSON number decodes to `float64`, so an `int` field failed identically even for a correctly typed caller. Values are now coerced to the field's declared type before being set, mirroring Jackson (which the Java host reaches via RecipeLoader's `convertValue` fallback, not `RecipeIntrospectionUtils.convert`) and the C# server's `Convert.ChangeType`. An unconvertible value produces an `OptionBindError` naming the recipe, option, declared type and value, which `handlePrepareRecipe` returns as `-32602` rather than a recovered panic. This requires `RecipeConstructor` to return an error. `PrepareRecipe` decodes with `UseNumber` so an integer option past 2^53 survives: `9007199254740993` otherwise bound as `9007199254740992` with no error at all. Option names resolve to fields case-insensitively when the capitalized spelling misses, so `url` reaches a `URL` field as it does on the Java host (`ACCEPT_CASE_INSENSITIVE_PROPERTIES`) and in C# (`BindingFlags.IgnoreCase`). An option naming no field stays ignored, matching Java (Jackson with `FAIL_ON_UNKNOWN_PROPERTIES` disabled) and C#. `GolangRecipeIntegTest` covers the wire path the CLI uses: with the coercion reverted, `booleanRecipeOptionArrivingAsAString` reproduces the reported `reflect.Set` message verbatim. --- rewrite-go/cmd/rpc/main.go | 11 +- .../cmd/rpc/prepare_recipe_options_test.go | 77 +++++++ rewrite-go/pkg/recipe/options.go | 189 +++++++++++++++ rewrite-go/pkg/recipe/options_test.go | 215 ++++++++++++++++++ rewrite-go/pkg/recipe/registry.go | 37 +-- rewrite-go/pkg/recipe/registry_test.go | 3 +- .../golang/rpc/GolangRecipeIntegTest.java | 30 +++ rewrite-go/test/recipe_test.go | 37 ++- 8 files changed, 580 insertions(+), 19 deletions(-) create mode 100644 rewrite-go/cmd/rpc/prepare_recipe_options_test.go create mode 100644 rewrite-go/pkg/recipe/options.go create mode 100644 rewrite-go/pkg/recipe/options_test.go diff --git a/rewrite-go/cmd/rpc/main.go b/rewrite-go/cmd/rpc/main.go index af2ac61e3b2..496432d0ac4 100644 --- a/rewrite-go/cmd/rpc/main.go +++ b/rewrite-go/cmd/rpc/main.go @@ -1743,7 +1743,11 @@ type delegatesToResponse struct { // handlePrepareRecipe instantiates a recipe by name with options. func (s *server) handlePrepareRecipe(params json.RawMessage) (any, *rpcError) { var req prepareRecipeRequest - if err := json.Unmarshal(params, &req); err != nil { + // UseNumber keeps an integer option's literal digits, which float64 cannot + // hold past 2^53; recipe.coerceOption parses them against the field's type. + dec := json.NewDecoder(bytes.NewReader(params)) + dec.UseNumber() + if err := dec.Decode(&req); err != nil { return nil, &rpcError{Code: -32602, Message: fmt.Sprintf("Invalid params: %v", err)} } @@ -1777,7 +1781,10 @@ func (s *server) handlePrepareRecipe(params json.RawMessage) (any, *rpcError) { // be described, so return the stored descriptor with no prepared child tree. Execution happens // in the CLI-built binary, where the recipe module is linked and its constructor exists, so // prepareInstance below runs and returns the whole tree. - instance := reg.Constructor(req.Options) + instance, err := reg.Constructor(req.Options) + if err != nil { + return nil, &rpcError{Code: -32602, Message: err.Error()} + } if instance == nil { recipeID := uuid.New().String() // Store the nil instance keyed by id so a later Visit can fail loudly by recipe name diff --git a/rewrite-go/cmd/rpc/prepare_recipe_options_test.go b/rewrite-go/cmd/rpc/prepare_recipe_options_test.go new file mode 100644 index 00000000000..c2542e7bfac --- /dev/null +++ b/rewrite-go/cmd/rpc/prepare_recipe_options_test.go @@ -0,0 +1,77 @@ +/* + * Copyright 2026 the original author or authors. + * + * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://docs.moderne.io/licensing/moderne-source-available-license + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package main + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" +) + +type optionBindingRecipe struct { + recipe.Base + Threshold int64 + Enabled bool +} + +func (r *optionBindingRecipe) Name() string { return "org.openrewrite.go.example.OptionBinding" } +func (r *optionBindingRecipe) DisplayName() string { return "Option binding" } +func (r *optionBindingRecipe) Description() string { return "Binds options sent over the wire." } + +func prepareWithRawOptions(t *testing.T, s *server, rawOptions string) (*optionBindingRecipe, *rpcError) { + t.Helper() + params := json.RawMessage(`{"id":"org.openrewrite.go.example.OptionBinding","options":` + rawOptions + `}`) + resp, rpcErr := s.handlePrepareRecipe(params) + if rpcErr != nil { + return nil, rpcErr + } + prepared := s.preparedRecipes[resp.(prepareRecipeResponse).ID] + bound, ok := prepared.(*optionBindingRecipe) + require.True(t, ok, "expected an *optionBindingRecipe") + return bound, nil +} + +func TestPrepareRecipeBindsOptionsFromWireJSON(t *testing.T) { + s, _ := newTestServer(t) + s.registry.Register(&optionBindingRecipe{}, recipe.CategoryDescriptor{DisplayName: "Go"}) + + // The CLI's -P delivers every option as a JSON string, whatever its type. + bound, rpcErr := prepareWithRawOptions(t, s, `{"threshold":"12","enabled":"false"}`) + require.Nil(t, rpcErr) + assert.Equal(t, int64(12), bound.Threshold) + assert.False(t, bound.Enabled) + + // A bare JSON integer past 2^53 has no exact float64 form. + bound, rpcErr = prepareWithRawOptions(t, s, `{"threshold":9007199254740993}`) + require.Nil(t, rpcErr) + assert.Equal(t, int64(9007199254740993), bound.Threshold) +} + +func TestPrepareRecipeRejectsUnbindableOption(t *testing.T) { + s, _ := newTestServer(t) + s.registry.Register(&optionBindingRecipe{}, recipe.CategoryDescriptor{DisplayName: "Go"}) + + _, rpcErr := prepareWithRawOptions(t, s, `{"enabled":"yes"}`) + require.NotNil(t, rpcErr) + assert.Equal(t, -32602, rpcErr.Code) + assert.Contains(t, rpcErr.Message, "org.openrewrite.go.example.OptionBinding") + assert.Contains(t, rpcErr.Message, "enabled") +} diff --git a/rewrite-go/pkg/recipe/options.go b/rewrite-go/pkg/recipe/options.go new file mode 100644 index 00000000000..d29498d6017 --- /dev/null +++ b/rewrite-go/pkg/recipe/options.go @@ -0,0 +1,189 @@ +/* + * Copyright 2026 the original author or authors. + * + * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://docs.moderne.io/licensing/moderne-source-available-license + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package recipe + +import ( + "encoding/json" + "fmt" + "math" + "reflect" + "strconv" + "strings" +) + +// OptionBindError reports an option value that could not be bound to the +// recipe field it names. +type OptionBindError struct { + Recipe string + Option string + Target reflect.Type + Value any +} + +func (e *OptionBindError) Error() string { + return fmt.Sprintf("recipe %s: option %q expects %s, got %s %#v", + e.Recipe, e.Option, e.Target, reflect.TypeOf(e.Value), e.Value) +} + +// coerceOption converts a recipe option value to the declared type of the field +// it binds to. Options are JSON-decoded into `any`, so a number is a float64 +// whatever the field's width, and the Moderne CLI declares -P as a +// Map, making every command-line option a string. Accepted +// conversions mirror Jackson (reached via RecipeLoader's convertValue fallback, +// not RecipeIntrospectionUtils.convert) and C#'s Convert.ChangeType. +func coerceOption(val any, t reflect.Type) (reflect.Value, bool) { + if val == nil { + return reflect.Zero(t), true + } + v := reflect.ValueOf(val) + if v.Type().AssignableTo(t) { + return v, true + } + + if t.Kind() == reflect.Ptr { + elem, ok := coerceOption(val, t.Elem()) + if !ok { + return reflect.Value{}, false + } + p := reflect.New(t.Elem()) + p.Elem().Set(elem) + return p, true + } + + switch t.Kind() { + case reflect.Bool: + if s, ok := val.(string); ok { + if b, err := strconv.ParseBool(s); err == nil { + return reflect.ValueOf(b).Convert(t), true + } + } + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if n, ok := asInt64(val); ok && !reflect.Zero(t).OverflowInt(n) { + return reflect.ValueOf(n).Convert(t), true + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if n, ok := asUint64(val); ok && !reflect.Zero(t).OverflowUint(n) { + return reflect.ValueOf(n).Convert(t), true + } + case reflect.Float32, reflect.Float64: + if f, ok := asFloat64(val); ok && !reflect.Zero(t).OverflowFloat(f) { + return reflect.ValueOf(f).Convert(t), true + } + case reflect.String: + if s, ok := asString(val); ok { + return reflect.ValueOf(s).Convert(t), true + } + case reflect.Slice: + if elems, ok := val.([]any); ok { + out := reflect.MakeSlice(t, len(elems), len(elems)) + for i, e := range elems { + ev, ok := coerceOption(e, t.Elem()) + if !ok { + return reflect.Value{}, false + } + out.Index(i).Set(ev) + } + return out, true + } + } + return reflect.Value{}, false +} + +// asInt64 accepts an integer's wire forms: a decimal string, a json.Number +// holding the literal digits, and a float64 that represents the value exactly. +func asInt64(val any) (int64, bool) { + switch x := val.(type) { + case string: + n, err := strconv.ParseInt(x, 10, 64) + return n, err == nil + case json.Number: + n, err := strconv.ParseInt(string(x), 10, 64) + return n, err == nil + case float64: + // Compare before converting: a float64 outside int64's range converts + // to an implementation-defined value rather than saturating. + if x != math.Trunc(x) || x < math.MinInt64 || x >= math.MaxInt64 { + return 0, false + } + return int64(x), true + } + return 0, false +} + +// asUint64 parses the full unsigned range, which int64 cannot hold. +func asUint64(val any) (uint64, bool) { + switch x := val.(type) { + case string: + n, err := strconv.ParseUint(x, 10, 64) + return n, err == nil + case json.Number: + n, err := strconv.ParseUint(string(x), 10, 64) + return n, err == nil + case float64: + if x != math.Trunc(x) || x < 0 || x >= math.MaxUint64 { + return 0, false + } + return uint64(x), true + } + return 0, false +} + +func asFloat64(val any) (float64, bool) { + switch x := val.(type) { + case string: + f, err := strconv.ParseFloat(x, 64) + return f, err == nil + case json.Number: + f, err := strconv.ParseFloat(string(x), 64) + return f, err == nil + case float64: + return x, true + } + return 0, false +} + +// asString renders the scalars a string option can arrive as, matching Java's +// Objects.toString(o, null) for String-typed options. Decimal notation keeps a +// JSON number reading back as it was written. +func asString(val any) (string, bool) { + switch x := val.(type) { + case string: + return x, true + case json.Number: + return string(x), true + case bool: + return strconv.FormatBool(x), true + case float64: + return strconv.FormatFloat(x, 'f', -1, 64), true + } + return "", false +} + +// optionField resolves an option name to the struct field it binds to. The +// leading letter is capitalized so "oldName" reaches OldName, and the +// fold-equal fallback lets "url" reach a URL field, matching the Java host +// (MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES) and the C# server +// (BindingFlags.IgnoreCase). +func optionField(elem reflect.Value, name string) reflect.Value { + if name == "" { + return reflect.Value{} + } + if f := elem.FieldByName(strings.ToUpper(name[:1]) + name[1:]); f.IsValid() { + return f + } + return elem.FieldByNameFunc(func(n string) bool { return strings.EqualFold(n, name) }) +} diff --git a/rewrite-go/pkg/recipe/options_test.go b/rewrite-go/pkg/recipe/options_test.go new file mode 100644 index 00000000000..ebebf0fd6de --- /dev/null +++ b/rewrite-go/pkg/recipe/options_test.go @@ -0,0 +1,215 @@ +/* + * Copyright 2026 the original author or authors. + * + * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://docs.moderne.io/licensing/moderne-source-available-license + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package recipe + +import ( + "encoding/json" + "math" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// optionsRecipe exercises every option field kind the binder supports. +type optionsRecipe struct { + Base + Text string + Flag bool + Count int + Small int8 + Unsigned uint16 + Ratio float64 + Alias *string + Enabled *bool + Patterns []string + Sizes []int + Raw any + Big uint64 + URL string +} + +func (r *optionsRecipe) Name() string { return "com.example.Options" } +func (r *optionsRecipe) DisplayName() string { return "Options" } +func (r *optionsRecipe) Description() string { return "Binds options of every supported kind." } + +func bind(t *testing.T, options map[string]any) (*optionsRecipe, error) { + t.Helper() + inst, err := newReflectConstructor(&optionsRecipe{})(options) + if err != nil { + return nil, err + } + return inst.(*optionsRecipe), nil +} + +func TestBindCoercesToDeclaredFieldType(t *testing.T) { + tests := []struct { + name string + option string + value any + got func(*optionsRecipe) any + want any + }{ + {"string passthrough", "text", "hello", func(r *optionsRecipe) any { return r.Text }, "hello"}, + {"bool passthrough", "flag", true, func(r *optionsRecipe) any { return r.Flag }, true}, + {"string to bool true", "flag", "true", func(r *optionsRecipe) any { return r.Flag }, true}, + {"string to bool false", "flag", "false", func(r *optionsRecipe) any { return r.Flag }, false}, + {"string to bool mixed case", "flag", "TRUE", func(r *optionsRecipe) any { return r.Flag }, true}, + {"float64 to int", "count", float64(42), func(r *optionsRecipe) any { return r.Count }, 42}, + {"string to int", "count", "42", func(r *optionsRecipe) any { return r.Count }, 42}, + {"string to negative int", "count", "-7", func(r *optionsRecipe) any { return r.Count }, -7}, + {"float64 to uint", "unsigned", float64(65535), func(r *optionsRecipe) any { return r.Unsigned }, uint16(65535)}, + {"string to uint", "unsigned", "12", func(r *optionsRecipe) any { return r.Unsigned }, uint16(12)}, + {"float64 to float", "ratio", 1.5, func(r *optionsRecipe) any { return r.Ratio }, 1.5}, + {"string to float", "ratio", "1.5", func(r *optionsRecipe) any { return r.Ratio }, 1.5}, + {"bool to string", "text", true, func(r *optionsRecipe) any { return r.Text }, "true"}, + {"integral float64 to string", "text", float64(42), func(r *optionsRecipe) any { return r.Text }, "42"}, + {"any passthrough", "raw", []any{"a"}, func(r *optionsRecipe) any { return r.Raw }, []any{"a"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, err := bind(t, map[string]any{tt.option: tt.value}) + require.NoError(t, err) + assert.Equal(t, tt.want, tt.got(r)) + }) + } +} + +func TestBindPointerOptions(t *testing.T) { + r, err := bind(t, map[string]any{"alias": "fmtutil", "enabled": "false"}) + require.NoError(t, err) + require.NotNil(t, r.Alias) + assert.Equal(t, "fmtutil", *r.Alias) + require.NotNil(t, r.Enabled) + assert.False(t, *r.Enabled) + + // A pointer option distinguishes "unset" from "set to the zero value". + r, err = bind(t, map[string]any{"alias": nil}) + require.NoError(t, err) + assert.Nil(t, r.Alias) +} + +func TestBindSliceOptions(t *testing.T) { + r, err := bind(t, map[string]any{ + "patterns": []any{"a", "b"}, + "sizes": []any{float64(1), "2"}, + }) + require.NoError(t, err) + assert.Equal(t, []string{"a", "b"}, r.Patterns) + assert.Equal(t, []int{1, 2}, r.Sizes) +} + +func TestBindRejectsOutOfRangeValues(t *testing.T) { + for _, tt := range []struct { + name string + option string + value any + }{ + {"int8 overflow from number", "small", float64(300)}, + {"int8 overflow from string", "small", "300"}, + {"uint from negative number", "unsigned", float64(-1)}, + {"uint from negative string", "unsigned", "-1"}, + {"int from non-integral number", "count", 1.5}, + {"int from number beyond int64 range", "count", math.MaxFloat64}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := bind(t, map[string]any{tt.option: tt.value}) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.option) + }) + } +} + +func TestBindErrorNamesRecipeOptionTypeAndValue(t *testing.T) { + _, err := bind(t, map[string]any{"flag": "yes"}) + require.Error(t, err) + msg := err.Error() + assert.Contains(t, msg, "com.example.Options") + assert.Contains(t, msg, "flag") + assert.Contains(t, msg, "bool") + assert.Contains(t, msg, "yes") +} + +func TestBindRejectsUnconvertibleValues(t *testing.T) { + for _, tt := range []struct { + name string + option string + value any + }{ + {"non-boolean string to bool", "flag", "yes"}, + {"non-numeric string to int", "count", "abc"}, + {"object to string", "text", map[string]any{"a": 1}}, + {"scalar to slice", "patterns", "a"}, + {"unconvertible slice element", "sizes", []any{"abc"}}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := bind(t, map[string]any{tt.option: tt.value}) + require.Error(t, err) + }) + } +} + +func TestBindIgnoresUnknownOptions(t *testing.T) { + r, err := bind(t, map[string]any{"text": "hi", "noSuchOption": "x"}) + require.NoError(t, err) + assert.Equal(t, "hi", r.Text) +} + +func TestBindLeavesUnsetOptionsAtZeroValue(t *testing.T) { + r, err := bind(t, nil) + require.NoError(t, err) + assert.Equal(t, "", r.Text) + assert.False(t, r.Flag) + assert.Nil(t, r.Alias) +} + +func TestBindLargeUnsignedOption(t *testing.T) { + r, err := bind(t, map[string]any{"big": "18446744073709551615"}) + require.NoError(t, err) + assert.Equal(t, uint64(math.MaxUint64), r.Big) +} + +func TestBindPreservesIntegerPrecision(t *testing.T) { + r, err := bind(t, map[string]any{"count": json.Number("9007199254740993")}) + require.NoError(t, err) + assert.Equal(t, 9007199254740993, r.Count) + + r, err = bind(t, map[string]any{"ratio": json.Number("1.5")}) + require.NoError(t, err) + assert.Equal(t, 1.5, r.Ratio) + + r, err = bind(t, map[string]any{"text": json.Number("42")}) + require.NoError(t, err) + assert.Equal(t, "42", r.Text) + + _, err = bind(t, map[string]any{"count": json.Number("1.5")}) + require.Error(t, err) +} + +func TestBindMatchesFieldNamesCaseInsensitively(t *testing.T) { + for _, name := range []string{"url", "URL", "Url"} { + r, err := bind(t, map[string]any{name: "https://example.com"}) + require.NoErrorf(t, err, "option %q", name) + assert.Equalf(t, "https://example.com", r.URL, "option %q", name) + } +} + +func TestBindIgnoresEmptyOptionName(t *testing.T) { + r, err := bind(t, map[string]any{"": "x", "text": "hi"}) + require.NoError(t, err) + assert.Equal(t, "hi", r.Text) +} diff --git a/rewrite-go/pkg/recipe/registry.go b/rewrite-go/pkg/recipe/registry.go index d1d4f33e9a2..7e78f9fe745 100644 --- a/rewrite-go/pkg/recipe/registry.go +++ b/rewrite-go/pkg/recipe/registry.go @@ -18,7 +18,6 @@ package recipe import ( "reflect" - "strings" "sync" ) @@ -27,7 +26,9 @@ type CategoryDescriptor struct { Description string } -type RecipeConstructor func(options map[string]any) Recipe +// An error means an option value could not be bound to its declared field +// type; see OptionBindError. +type RecipeConstructor func(options map[string]any) (Recipe, error) type Registration struct { Descriptor RecipeDescriptor @@ -132,7 +133,7 @@ func (r *Registry) registerSubRecipes(rec Recipe) { child := sub r.subByName[name] = &Registration{ Descriptor: Describe(child), - Constructor: func(map[string]any) Recipe { return child }, + Constructor: func(map[string]any) (Recipe, error) { return child, nil }, } r.registerSubRecipes(child) } @@ -151,8 +152,8 @@ func (r *Registry) RegisterWithCategories(desc RecipeDescriptor, categories []Ca // (e.g., registered via Activate in the custom binary) with a nil // constructor from the installer's descriptor-only registration. if existing, ok := r.byName[desc.Name]; ok && existing.Constructor != nil { - testInstance := existing.Constructor(nil) - if testInstance != nil { + testInstance, err := existing.Constructor(nil) + if err == nil && testInstance != nil { // Existing registration has a real implementation — keep it, // just update categories if provided. if len(categories) > 0 { @@ -163,7 +164,7 @@ func (r *Registry) RegisterWithCategories(desc RecipeDescriptor, categories []Ca } reg := &Registration{ Descriptor: desc, - Constructor: func(options map[string]any) Recipe { return nil }, + Constructor: func(map[string]any) (Recipe, error) { return nil, nil }, Categories: categories, } r.byName[desc.Name] = reg @@ -231,8 +232,8 @@ func (r *Registry) findOrCreateSubcategory(parent *Category, desc CategoryDescri // newReflectConstructor creates a RecipeConstructor that instantiates new // copies of the prototype's concrete type via reflection. Option names are -// mapped to exported struct fields by capitalizing the first letter -// (e.g., "oldName" → "OldName"). +// mapped to exported struct fields by optionField, and each value is coerced +// to the field's declared type by coerceOption. func newReflectConstructor(prototype Recipe) RecipeConstructor { t := reflect.TypeOf(prototype) isPtr := t.Kind() == reflect.Ptr @@ -240,21 +241,27 @@ func newReflectConstructor(prototype Recipe) RecipeConstructor { t = t.Elem() } - return func(options map[string]any) Recipe { + return func(options map[string]any) (Recipe, error) { v := reflect.New(t) elem := v.Elem() for name, val := range options { - fieldName := strings.ToUpper(name[:1]) + name[1:] - f := elem.FieldByName(fieldName) - if f.IsValid() && f.CanSet() { - f.Set(reflect.ValueOf(val)) + f := optionField(elem, name) + // An option that names no field is ignored, matching the Java host + // (Jackson with FAIL_ON_UNKNOWN_PROPERTIES disabled) and the C# server. + if !f.IsValid() || !f.CanSet() { + continue } + cv, ok := coerceOption(val, f.Type()) + if !ok { + return nil, &OptionBindError{Recipe: prototype.Name(), Option: name, Target: f.Type(), Value: val} + } + f.Set(cv) } if isPtr { - return v.Interface().(Recipe) + return v.Interface().(Recipe), nil } - return v.Elem().Interface().(Recipe) + return v.Elem().Interface().(Recipe), nil } } diff --git a/rewrite-go/pkg/recipe/registry_test.go b/rewrite-go/pkg/recipe/registry_test.go index f7a6bcee6dc..5605bcf95f9 100644 --- a/rewrite-go/pkg/recipe/registry_test.go +++ b/rewrite-go/pkg/recipe/registry_test.go @@ -69,7 +69,8 @@ func TestRegisterResolvesCompositeChildren(t *testing.T) { for _, child := range []string{"com.example.Composite$Keep", "com.example.Composite$Negate"} { reg, ok := r.FindRecipe(child) require.Truef(t, ok, "expected child recipe %q to resolve via FindRecipe", child) - inst := reg.Constructor(nil) + inst, err := reg.Constructor(nil) + require.NoErrorf(t, err, "expected child recipe %q constructor to succeed", child) require.NotNilf(t, inst, "expected child recipe %q constructor to return an instance", child) assert.Equalf(t, inst.Name(), child, "child %q resolved to instance with name", child) } diff --git a/rewrite-go/src/integTest/java/org/openrewrite/golang/rpc/GolangRecipeIntegTest.java b/rewrite-go/src/integTest/java/org/openrewrite/golang/rpc/GolangRecipeIntegTest.java index daf5b199f93..95e3478cc1c 100644 --- a/rewrite-go/src/integTest/java/org/openrewrite/golang/rpc/GolangRecipeIntegTest.java +++ b/rewrite-go/src/integTest/java/org/openrewrite/golang/rpc/GolangRecipeIntegTest.java @@ -34,9 +34,11 @@ import org.openrewrite.test.TypeValidation; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Map; import java.util.UUID; import java.util.concurrent.TimeUnit; @@ -280,6 +282,34 @@ void goNativeRecipeViaRpc() { assertThat(printed).contains("var flag = true").contains("_ = flag"); } + @Test + void booleanRecipeOptionArrivingAsAString() { + GoRewriteRpc rpc = GoRewriteRpc.getOrStart(); + String source = "package main\n\nfunc f() {\n}\n"; + + SourceFile cu = GolangParser.builder().build().parse(source).findFirst().orElseThrow(); + var unconditional = rpc.prepareRecipe("org.openrewrite.golang.AddImport", + Map.of("packagePath", "strings", "onlyIfReferenced", "false")); + Tree added = unconditional.getVisitor().visit(cu, new InMemoryExecutionContext()); + assertThat(rpc.print((SourceFile) added)).contains("import \"strings\""); + + cu = GolangParser.builder().build().parse(source).findFirst().orElseThrow(); + var gated = rpc.prepareRecipe("org.openrewrite.golang.AddImport", + Map.of("packagePath", "strings", "onlyIfReferenced", "true")); + Tree untouched = gated.getVisitor().visit(cu, new InMemoryExecutionContext()); + assertThat(rpc.print((SourceFile) untouched)).doesNotContain("strings"); + } + + @Test + void unbindableRecipeOptionNamesTheOption() { + GoRewriteRpc rpc = GoRewriteRpc.getOrStart(); + assertThatThrownBy(() -> rpc.prepareRecipe("org.openrewrite.golang.AddImport", + Map.of("packagePath", "strings", "onlyIfReferenced", "yes"))) + .hasMessageContaining("org.openrewrite.golang.AddImport") + .hasMessageContaining("onlyIfReferenced") + .hasMessageContaining("bool"); + } + /** * Simulates the CLI path where the tree is loaded from a JAR (no RPC baseline). * Forces the Visit RPC to use ADD (not CHANGE) by resetting the Go RPC state. diff --git a/rewrite-go/test/recipe_test.go b/rewrite-go/test/recipe_test.go index 8a5e8cf43da..eae0ad2424e 100644 --- a/rewrite-go/test/recipe_test.go +++ b/rewrite-go/test/recipe_test.go @@ -27,6 +27,7 @@ import ( "github.com/openrewrite/rewrite/rewrite-go/pkg/parser" "github.com/openrewrite/rewrite/rewrite-go/pkg/printer" "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe" + "github.com/openrewrite/rewrite/rewrite-go/pkg/recipe/golang" "github.com/openrewrite/rewrite/rewrite-go/pkg/test" "github.com/openrewrite/rewrite/rewrite-go/pkg/tree/java" "github.com/openrewrite/rewrite/rewrite-go/pkg/visitor" @@ -222,10 +223,44 @@ func TestRegistryReflectConstructor(t *testing.T) { require.True(t, ok, "expected to find recipe") // Constructor auto-derived from prototype via reflection - instance := found.Constructor(nil) + instance, err := found.Constructor(nil) + require.NoError(t, err) assert.Equal(t, "org.openrewrite.golang.test.RenameFooToBar", instance.Name(), "unexpected name") } +func TestRegistryBindsOptionsArrivingAsStrings(t *testing.T) { + reg := recipe.NewRegistry() + reg.Activate(func(r *recipe.Registry) { + r.Register(&golang.AddImport{}, recipe.CategoryDescriptor{DisplayName: "Go"}) + }) + + found, ok := reg.FindRecipe("org.openrewrite.golang.AddImport") + require.True(t, ok, "expected to find recipe") + + instance, err := found.Constructor(map[string]any{ + "packagePath": "strings", + "alias": "s", + "onlyIfReferenced": "true", + }) + require.NoError(t, err) + + addImport, ok := instance.(*golang.AddImport) + require.True(t, ok, "expected an *AddImport") + assert.Equal(t, "strings", addImport.PackagePath) + require.NotNil(t, addImport.Alias) + assert.Equal(t, "s", *addImport.Alias) + assert.True(t, addImport.OnlyIfReferenced) + + instance, err = found.Constructor(map[string]any{"packagePath": "strings", "onlyIfReferenced": "false"}) + require.NoError(t, err) + assert.False(t, instance.(*golang.AddImport).OnlyIfReferenced) + + _, err = found.Constructor(map[string]any{"packagePath": "strings", "onlyIfReferenced": "yes"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "org.openrewrite.golang.AddImport") + assert.Contains(t, err.Error(), "onlyIfReferenced") +} + func TestFencedMarkerPrinting(t *testing.T) { r := &findFoo{} editor := r.Editor() From da4ab3ab6928aacc50610c3ab2199f4c9a07105a Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 24 Aug 2026 11:54:16 +0200 Subject: [PATCH 2/2] Go: bind option values passed as Go's own numeric types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RecipeConstructor is exported and takes map[string]any, so options reach the binder from two origins: JSON-decoded wire values (string, json.Number) and Go values passed directly in-process. Only the first was accepted, so `Constructor(map[string]any{"count": 42})` bound to an `int` field via the assignable fast path but failed against `int64`, `uint64`, `float64` or `string` — the same value that binds when it arrives as json.Number("42"). The conversion helpers now read any integer, unsigned or float kind through reflection, range-checked as before. Also corrects two comments. Register's doc claimed "for recipes without options, the prototype itself is returned", which newReflectConstructor has never done — it allocates a zero-valued instance, discarding any field set on the prototype. And coerceOption claimed its conversions "mirror" Jackson and Convert.ChangeType, which both read a numeric 1/0 as a bool where this does not; the deviation is now stated at the case it applies to. --- rewrite-go/pkg/recipe/options.go | 73 ++++++++++++++++++++------- rewrite-go/pkg/recipe/options_test.go | 42 +++++++++++++++ rewrite-go/pkg/recipe/registry.go | 9 +--- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/rewrite-go/pkg/recipe/options.go b/rewrite-go/pkg/recipe/options.go index d29498d6017..cd3c62e13c5 100644 --- a/rewrite-go/pkg/recipe/options.go +++ b/rewrite-go/pkg/recipe/options.go @@ -42,9 +42,9 @@ func (e *OptionBindError) Error() string { // coerceOption converts a recipe option value to the declared type of the field // it binds to. Options are JSON-decoded into `any`, so a number is a float64 // whatever the field's width, and the Moderne CLI declares -P as a -// Map, making every command-line option a string. Accepted -// conversions mirror Jackson (reached via RecipeLoader's convertValue fallback, -// not RecipeIntrospectionUtils.convert) and C#'s Convert.ChangeType. +// Map, making every command-line option a string. Conversions +// are drawn from Jackson (reached via RecipeLoader's convertValue fallback, not +// RecipeIntrospectionUtils.convert) and C#'s Convert.ChangeType. func coerceOption(val any, t reflect.Type) (reflect.Value, bool) { if val == nil { return reflect.Zero(t), true @@ -66,6 +66,7 @@ func coerceOption(val any, t reflect.Type) (reflect.Value, bool) { switch t.Kind() { case reflect.Bool: + // Only a string spelling: a number's truthiness is ambiguous. if s, ok := val.(string); ok { if b, err := strconv.ParseBool(s); err == nil { return reflect.ValueOf(b).Convert(t), true @@ -103,8 +104,10 @@ func coerceOption(val any, t reflect.Type) (reflect.Value, bool) { return reflect.Value{}, false } -// asInt64 accepts an integer's wire forms: a decimal string, a json.Number -// holding the literal digits, and a float64 that represents the value exactly. +// asInt64 accepts an integer's wire forms — a decimal string and a json.Number +// holding the literal digits — plus the numeric types an in-process caller +// passes directly. RecipeConstructor is exported and takes map[string]any, so +// both origins reach here. func asInt64(val any) (int64, bool) { switch x := val.(type) { case string: @@ -113,13 +116,22 @@ func asInt64(val any) (int64, bool) { case json.Number: n, err := strconv.ParseInt(string(x), 10, 64) return n, err == nil - case float64: - // Compare before converting: a float64 outside int64's range converts - // to an implementation-defined value rather than saturating. - if x != math.Trunc(x) || x < math.MinInt64 || x >= math.MaxInt64 { - return 0, false + } + v := reflect.ValueOf(val) + switch v.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return v.Int(), true + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if u := v.Uint(); u <= math.MaxInt64 { + return int64(u), true + } + case reflect.Float32, reflect.Float64: + // Compare before converting: a float outside int64's range converts to + // an implementation-defined value rather than saturating. + f := v.Float() + if f == math.Trunc(f) && f >= math.MinInt64 && f < math.MaxInt64 { + return int64(f), true } - return int64(x), true } return 0, false } @@ -133,11 +145,20 @@ func asUint64(val any) (uint64, bool) { case json.Number: n, err := strconv.ParseUint(string(x), 10, 64) return n, err == nil - case float64: - if x != math.Trunc(x) || x < 0 || x >= math.MaxUint64 { - return 0, false + } + v := reflect.ValueOf(val) + switch v.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if n := v.Int(); n >= 0 { + return uint64(n), true + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return v.Uint(), true + case reflect.Float32, reflect.Float64: + f := v.Float() + if f == math.Trunc(f) && f >= 0 && f < math.MaxUint64 { + return uint64(f), true } - return uint64(x), true } return 0, false } @@ -150,8 +171,15 @@ func asFloat64(val any) (float64, bool) { case json.Number: f, err := strconv.ParseFloat(string(x), 64) return f, err == nil - case float64: - return x, true + } + v := reflect.ValueOf(val) + switch v.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return float64(v.Int()), true + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return float64(v.Uint()), true + case reflect.Float32, reflect.Float64: + return v.Float(), true } return 0, false } @@ -167,8 +195,15 @@ func asString(val any) (string, bool) { return string(x), true case bool: return strconv.FormatBool(x), true - case float64: - return strconv.FormatFloat(x, 'f', -1, 64), true + } + v := reflect.ValueOf(val) + switch v.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return strconv.FormatInt(v.Int(), 10), true + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return strconv.FormatUint(v.Uint(), 10), true + case reflect.Float32, reflect.Float64: + return strconv.FormatFloat(v.Float(), 'f', -1, 64), true } return "", false } diff --git a/rewrite-go/pkg/recipe/options_test.go b/rewrite-go/pkg/recipe/options_test.go index ebebf0fd6de..66d9e0d477d 100644 --- a/rewrite-go/pkg/recipe/options_test.go +++ b/rewrite-go/pkg/recipe/options_test.go @@ -213,3 +213,45 @@ func TestBindIgnoresEmptyOptionName(t *testing.T) { require.NoError(t, err) assert.Equal(t, "hi", r.Text) } + +func TestBindGoNativeNumericOptions(t *testing.T) { + tests := []struct { + name string + option string + value any + got func(*optionsRecipe) any + want any + }{ + {"int to int8", "small", 42, func(r *optionsRecipe) any { return r.Small }, int8(42)}, + {"int to uint64", "big", 42, func(r *optionsRecipe) any { return r.Big }, uint64(42)}, + {"int to float64", "ratio", 42, func(r *optionsRecipe) any { return r.Ratio }, float64(42)}, + {"int to string", "text", 42, func(r *optionsRecipe) any { return r.Text }, "42"}, + {"int64 to int", "count", int64(42), func(r *optionsRecipe) any { return r.Count }, 42}, + {"uint8 to int", "count", uint8(42), func(r *optionsRecipe) any { return r.Count }, 42}, + {"float32 to float64", "ratio", float32(1.5), func(r *optionsRecipe) any { return r.Ratio }, 1.5}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, err := bind(t, map[string]any{tt.option: tt.value}) + require.NoError(t, err) + assert.Equal(t, tt.want, tt.got(r)) + }) + } +} + +func TestBindRejectsOutOfRangeGoNativeNumbers(t *testing.T) { + for _, tt := range []struct { + name string + option string + value any + }{ + {"negative int to uint", "unsigned", -1}, + {"int overflowing int8", "small", 300}, + {"uint64 overflowing int64", "count", uint64(math.MaxUint64)}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := bind(t, map[string]any{tt.option: tt.value}) + require.Error(t, err) + }) + } +} diff --git a/rewrite-go/pkg/recipe/registry.go b/rewrite-go/pkg/recipe/registry.go index 7e78f9fe745..3d373900251 100644 --- a/rewrite-go/pkg/recipe/registry.go +++ b/rewrite-go/pkg/recipe/registry.go @@ -84,13 +84,8 @@ func NewRegistry() *Registry { } } -// The prototype is used to extract the recipe descriptor. A constructor is -// automatically derived via reflection: it creates a new instance of the -// prototype's type and sets exported fields from the options map. Option -// names are mapped to field names by capitalizing the first letter -// (e.g., option "oldName" sets field "OldName"). -// -// For recipes without options, the prototype itself is returned. +// The prototype supplies the recipe descriptor and the concrete type that +// newReflectConstructor instantiates, zero-valued, once per prepared run. func (r *Registry) Register(prototype Recipe, categories ...CategoryDescriptor) { r.mu.Lock() defer r.mu.Unlock()