From f55b781d862409fd10ebf73907350a6eee69844b Mon Sep 17 00:00:00 2001 From: Fabio Massaioli Date: Thu, 2 May 2024 13:46:07 +0200 Subject: [PATCH] [v3] Binding runtime fixes (#3431) * Unmarshal arguments to appropriate type in binding calls * Marshal multiple return values to arrays in binding calls * Improve logging of remote method calls * Add tests for `BoundMethod.Call` * Fix return value if error is nil * Update changelog --------- Co-authored-by: Andreas Bichinger --- mkdocs-website/docs/en/changelog.md | 3 + v3/pkg/application/bindings.go | 114 ++++++------ v3/pkg/application/bindings_test.go | 195 ++++++++++++++++++++ v3/pkg/application/messageprocessor_call.go | 13 +- 4 files changed, 269 insertions(+), 56 deletions(-) create mode 100644 v3/pkg/application/bindings_test.go diff --git a/mkdocs-website/docs/en/changelog.md b/mkdocs-website/docs/en/changelog.md index 14cb98d9..a69beb7b 100644 --- a/mkdocs-website/docs/en/changelog.md +++ b/mkdocs-website/docs/en/changelog.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add ESM exports from the bundled JS runtime script by [@fbbdev](https://github.com/fbbdev) in [#3295](https://github.com/wailsapp/wails/pull/3295) - Add binding generator flag for using the bundled JS runtime script instead of the npm package by [@fbbdev](https://github.com/fbbdev) in [#3334](https://github.com/wailsapp/wails/pull/3334) - Implement `setIcon` on linux by [@abichinger](https://github.com/abichinger) in [#3354](https://github.com/wailsapp/wails/pull/3354) +- Add tests for bound method calls by [@abichinger](https://github.com/abichinger) in [#3431](https://github.com/wailsapp/wails/pull/3431) ### Fixed @@ -59,6 +60,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix the import paths of model files in JS code produced by the binding generator by [@fbbdev](https://github.com/fbbdev) in [#3334](https://github.com/wailsapp/wails/pull/3334) - Fix drag-n-drop on some linux distros by [@abichinger](https://github.com/abichinger) in [#3346](https://github.com/wailsapp/wails/pull/3346) - Fix missing task for macOS when using `wails3 task dev` by [@hfoxy](https://github.com/hfoxy) in [#3417](https://github.com/wailsapp/wails/pull/3417) +- Fix unmarshaling of bound method parameters by [@fbbdev](https://github.com/fbbdev) in [#3431](https://github.com/wailsapp/wails/pull/3431) +- Fix handling of multiple return values from bound methods by [@fbbdev](https://github.com/fbbdev) in [#3431](https://github.com/wailsapp/wails/pull/3431) ### Changed diff --git a/v3/pkg/application/bindings.go b/v3/pkg/application/bindings.go index 632186ef..7f6e3210 100644 --- a/v3/pkg/application/bindings.go +++ b/v3/pkg/application/bindings.go @@ -2,6 +2,8 @@ package application import ( "context" + "encoding/json" + "errors" "fmt" "reflect" "runtime" @@ -13,20 +15,20 @@ import ( ) type CallOptions struct { - MethodID uint32 `json:"methodID"` - PackageName string `json:"packageName"` - StructName string `json:"structName"` - MethodName string `json:"methodName"` - Args []any `json:"args"` + MethodID uint32 `json:"methodID"` + PackageName string `json:"packageName"` + StructName string `json:"structName"` + MethodName string `json:"methodName"` + Args []json.RawMessage `json:"args"` } -func (c CallOptions) Name() string { +func (c *CallOptions) Name() string { return fmt.Sprintf("%s.%s.%s", c.PackageName, c.StructName, c.MethodName) } type PluginCallOptions struct { - Name string `json:"name"` - Args []any `json:"args"` + Name string `json:"name"` + Args []json.RawMessage `json:"args"` } var reservedPluginMethods = []string{ @@ -235,7 +237,7 @@ func (b *Bindings) getMethods(value interface{}, isPlugin bool) ([]*BoundMethod, structTypeString := structType.String() baseName := structTypeString[1:] - ctxType := reflect.TypeOf((*context.Context)(nil)).Elem() + ctxType := reflect.TypeFor[context.Context]() // Process Methods for i := 0; i < structType.NumMethod(); i++ { @@ -303,9 +305,10 @@ func (b *Bindings) getMethods(value interface{}, isPlugin bool) ([]*BoundMethod, return result, nil } -// Call will attempt to call this bound method with the given args -func (b *BoundMethod) Call(ctx context.Context, args []interface{}) (returnValue interface{}, err error) { +var errorType = reflect.TypeFor[error]() +// Call will attempt to call this bound method with the given args +func (b *BoundMethod) Call(ctx context.Context, args []json.RawMessage) (returnValue interface{}, err error) { // Use a defer statement to capture panics defer func() { if r := recover(); r != nil { @@ -325,65 +328,72 @@ func (b *BoundMethod) Call(ctx context.Context, args []interface{}) (returnValue } }() + argCount := len(args) if b.needsContext { - args = append([]any{ctx}, args...) + argCount++ } - // Check inputs - expectedInputLength := len(b.Inputs) - actualInputLength := len(args) - - // If the method is variadic, we need to check the minimum number of inputs - if b.Method.Type().IsVariadic() { - if actualInputLength < expectedInputLength-1 { - return nil, fmt.Errorf("%s takes at least %d inputs. Received %d", b.Name, expectedInputLength, actualInputLength) - } - } else { - if expectedInputLength != actualInputLength { - return nil, fmt.Errorf("%s takes %d inputs. Received %d", b.Name, expectedInputLength, actualInputLength) - } + if argCount != len(b.Inputs) { + err = fmt.Errorf("%s expects %d arguments, received %d", b.Name, len(b.Inputs), argCount) + return } - /** Convert inputs to reflect values **/ + // Convert inputs to values of appropriate type - // Create slice for the input arguments to the method call - callArgs := make([]reflect.Value, actualInputLength) + callArgs := make([]reflect.Value, argCount) + base := 0 + + if b.needsContext { + callArgs[0] = reflect.ValueOf(ctx) + base++ + } // Iterate over given arguments for index, arg := range args { - // Save the converted argument - if arg == nil { - callArgs[index] = reflect.Zero(b.Inputs[index].ReflectType) - continue + value := reflect.New(b.Inputs[base+index].ReflectType) + err = json.Unmarshal(arg, value.Interface()) + if err != nil { + err = fmt.Errorf("could not parse argument #%d: %w", index, err) + return } - callArgs[index] = reflect.ValueOf(arg) + callArgs[base+index] = value.Elem() } // Do the call - callResults := b.Method.Call(callArgs) + var callResults []reflect.Value + if b.Method.Type().IsVariadic() { + callResults = b.Method.CallSlice(callArgs) + } else { + callResults = b.Method.Call(callArgs) + } - //** Check results **// - switch len(b.Outputs) { - case 1: - // Loop over results and determine if the result - // is an error or not - for _, result := range callResults { - interfac := result.Interface() - temp, ok := interfac.(error) - if ok { - err = temp - } else { - returnValue = interfac + var nonErrorOutputs = make([]any, 0, len(callResults)) + var errorOutputs []error + + for _, result := range callResults { + if result.Type() == errorType { + if result.IsNil() { + continue } - } - case 2: - returnValue = callResults[0].Interface() - if temp, ok := callResults[1].Interface().(error); ok { - err = temp + if errorOutputs == nil { + errorOutputs = make([]error, 0, len(callResults)-len(nonErrorOutputs)) + nonErrorOutputs = nil + } + errorOutputs = append(errorOutputs, result.Interface().(error)) + } else if nonErrorOutputs != nil { + nonErrorOutputs = append(nonErrorOutputs, result.Interface()) } } - return returnValue, err + if errorOutputs != nil { + err = errors.Join(errorOutputs...) + } else if len(nonErrorOutputs) == 1 { + returnValue = nonErrorOutputs[0] + } else if len(nonErrorOutputs) > 1 { + returnValue = nonErrorOutputs + } + + return } // isStructPtr returns true if the value given is a diff --git a/v3/pkg/application/bindings_test.go b/v3/pkg/application/bindings_test.go new file mode 100644 index 00000000..b5ef8db3 --- /dev/null +++ b/v3/pkg/application/bindings_test.go @@ -0,0 +1,195 @@ +package application_test + +import ( + "context" + "encoding/json" + "errors" + "reflect" + "strings" + "testing" + + "github.com/wailsapp/wails/v3/pkg/application" +) + +type TestService struct { +} + +type Person struct { + Name string `json:"name"` +} + +func (t *TestService) Nil() {} + +func (t *TestService) String(s string) string { + return s +} + +func (t *TestService) Multiple(s string, i int, b bool) (string, int, bool) { + return s, i, b +} + +func (t *TestService) Struct(p Person) Person { + return p +} + +func (t *TestService) StructNil(p Person) (Person, error) { + return p, nil +} + +func (t *TestService) StructError(p Person) (Person, error) { + return p, errors.New("error") +} + +func (t *TestService) Variadic(s ...string) []string { + return s +} + +func (t *TestService) PositionalAndVariadic(a int, b ...string) int { + return a +} + +func (t *TestService) Slice(a []int) []int { + return a +} + +func newArgs(jsonArgs ...string) []json.RawMessage { + args := []json.RawMessage{} + + for _, j := range jsonArgs { + args = append(args, json.RawMessage(j)) + } + return args +} + +func TestBoundMethodCall(t *testing.T) { + + tests := []struct { + name string + method string + args []json.RawMessage + err error + expected interface{} + }{ + { + name: "nil", + method: "Nil", + args: []json.RawMessage{}, + err: nil, + expected: nil, + }, + { + name: "string", + method: "String", + args: newArgs(`"foo"`), + err: nil, + expected: "foo", + }, + { + name: "multiple", + method: "Multiple", + args: newArgs(`"foo"`, "0", "false"), + err: nil, + expected: []interface{}{"foo", 0, false}, + }, + { + name: "struct", + method: "Struct", + args: newArgs(`{ "name": "alice" }`), + err: nil, + expected: Person{Name: "alice"}, + }, + { + name: "struct, nil error", + method: "StructNil", + args: newArgs(`{ "name": "alice" }`), + err: nil, + expected: Person{Name: "alice"}, + }, + { + name: "struct, error", + method: "StructError", + args: newArgs(`{ "name": "alice" }`), + err: errors.New("error"), + expected: nil, + }, + { + name: "invalid argument count", + method: "Multiple", + args: newArgs(`"foo"`), + err: errors.New("expects 3 arguments, received 1"), + expected: nil, + }, + { + name: "invalid argument type", + method: "String", + args: newArgs("1"), + err: errors.New("could not parse"), + expected: nil, + }, + { + name: "variadic, no arguments", + method: "Variadic", + args: newArgs(`[]`), // variadic parameters are passed as arrays + err: nil, + expected: []string{}, + }, + { + name: "variadic", + method: "Variadic", + args: newArgs(`["foo", "bar"]`), + err: nil, + expected: []string{"foo", "bar"}, + }, + { + name: "positional and variadic", + method: "PositionalAndVariadic", + args: newArgs("42", `[]`), + err: nil, + expected: 42, + }, + { + name: "slice", + method: "Slice", + args: newArgs(`[1,2,3]`), + err: nil, + expected: []int{1, 2, 3}, + }, + } + + // init globalApplication + _ = application.New(application.Options{}) + + bindings, err := application.NewBindings( + []any{ + &TestService{}, + }, make(map[uint32]uint32), + ) + if err != nil { + t.Fatalf("application.NewBindings() error = %v\n", err) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + callOptions := &application.CallOptions{ + PackageName: "application_test", + StructName: "TestService", + MethodName: tt.method, + } + + method := bindings.Get(callOptions) + if method == nil { + t.Fatalf("bound method not found: %s", callOptions.Name()) + } + + result, err := method.Call(context.TODO(), tt.args) + if tt.err != err && (tt.err == nil || err == nil || !strings.Contains(err.Error(), tt.err.Error())) { + t.Fatalf("error: %v, expected error: %v", err, tt.err) + } + if !reflect.DeepEqual(result, tt.expected) { + t.Fatalf("result: %v, expected result: %v", result, tt.expected) + } + + }) + } + +} diff --git a/v3/pkg/application/messageprocessor_call.go b/v3/pkg/application/messageprocessor_call.go index 56659a6f..ce50026f 100644 --- a/v3/pkg/application/messageprocessor_call.go +++ b/v3/pkg/application/messageprocessor_call.go @@ -80,16 +80,16 @@ func (m *MessageProcessor) processCallMethod(method int, rw http.ResponseWriter, ctx, cancel := context.WithCancel(context.WithoutCancel(r.Context())) - ambigiousID := false + ambiguousID := false m.l.Lock() if m.runningCalls[*callID] != nil { - ambigiousID = true + ambiguousID = true } else { m.runningCalls[*callID] = cancel } m.l.Unlock() - if ambigiousID { + if ambiguousID { cancel() m.callErrorCallback(window, "Error calling method: %s, a method call with the same id is already running", callID, err) return @@ -119,7 +119,12 @@ func (m *MessageProcessor) processCallMethod(method int, rw http.ResponseWriter, } } m.callCallback(window, callID, string(jsonResult), true) - m.Info("Call Binding:", "method", boundMethod, "args", options.Args, "result", result) + + var jsonArgs struct { + Args json.RawMessage `json:"args"` + } + params.ToStruct(&jsonArgs) + m.Info("Call Binding:", "method", boundMethod, "args", string(jsonArgs.Args), "result", result) }() m.ok(rw) default: