Copilot commented on code in PR #3174: URL: https://github.com/apache/dubbo-go/pull/3174#discussion_r2712438877
########## filter/generic/service_test.go: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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 generic + +import ( + "context" + "testing" +) + +import ( + hessian "github.com/apache/dubbo-go-hessian2" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testUser struct { + Name string + Age int + Email string + Address *testAddress +} + +type testAddress struct { + City string + Country string +} + +func TestGenericService(t *testing.T) { + service := NewGenericService("HelloService") + reference := service.Reference() + assert.Equal(t, "HelloService", reference) +} + +func TestGenericService_InvokeWithType(t *testing.T) { + t.Run("simple struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + assert.Equal(t, "getUser", methodName) + return map[string]any{ + "name": "testUser", + "age": 25, + "email": "[email protected]", + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, &user) + + require.NoError(t, err) + assert.Equal(t, "testUser", user.Name) + assert.Equal(t, 25, user.Age) + assert.Equal(t, "[email protected]", user.Email) + }) + + t.Run("nested struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return map[string]any{ + "name": "nestedUser", + "age": 30, + "email": "[email protected]", + "address": map[string]any{ + "city": "Beijing", + "country": "China", + }, + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"456"}, &user) + + require.NoError(t, err) + assert.Equal(t, "nestedUser", user.Name) + assert.Equal(t, 30, user.Age) + require.NotNil(t, user.Address) + assert.Equal(t, "Beijing", user.Address.City) + assert.Equal(t, "China", user.Address.Country) + }) + + t.Run("slice result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return []any{ + map[string]any{"name": "user1", "age": 20}, + map[string]any{"name": "user2", "age": 25}, + }, nil + } + + var users []testUser + err := service.InvokeWithType(context.Background(), "listUsers", []string{}, []hessian.Object{}, &users) + + require.NoError(t, err) + require.Len(t, users, 2) + assert.Equal(t, "user1", users[0].Name) + assert.Equal(t, 20, users[0].Age) + assert.Equal(t, "user2", users[1].Name) + assert.Equal(t, 25, users[1].Age) + }) + + t.Run("nil result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return nil, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"789"}, &user) + + require.NoError(t, err) + assert.Empty(t, user.Name) + assert.Zero(t, user.Age) + }) + + t.Run("nil reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, nil) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply cannot be nil") + }) + + t.Run("non-pointer reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, user) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply must be a pointer") + }) +} Review Comment: The test suite is missing a test case for when the underlying `Invoke` method returns an error. Add a test case that verifies `InvokeWithType` correctly propagates errors from the `Invoke` method to ensure error handling is properly tested. ########## filter/generic/service_test.go: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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 generic + +import ( + "context" + "testing" +) + +import ( + hessian "github.com/apache/dubbo-go-hessian2" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testUser struct { + Name string + Age int + Email string + Address *testAddress +} + +type testAddress struct { + City string + Country string +} + +func TestGenericService(t *testing.T) { + service := NewGenericService("HelloService") + reference := service.Reference() + assert.Equal(t, "HelloService", reference) +} + +func TestGenericService_InvokeWithType(t *testing.T) { + t.Run("simple struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + assert.Equal(t, "getUser", methodName) + return map[string]any{ + "name": "testUser", + "age": 25, + "email": "[email protected]", + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, &user) + + require.NoError(t, err) + assert.Equal(t, "testUser", user.Name) + assert.Equal(t, 25, user.Age) + assert.Equal(t, "[email protected]", user.Email) + }) + + t.Run("nested struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return map[string]any{ + "name": "nestedUser", + "age": 30, + "email": "[email protected]", + "address": map[string]any{ + "city": "Beijing", + "country": "China", + }, + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"456"}, &user) + + require.NoError(t, err) + assert.Equal(t, "nestedUser", user.Name) + assert.Equal(t, 30, user.Age) + require.NotNil(t, user.Address) + assert.Equal(t, "Beijing", user.Address.City) + assert.Equal(t, "China", user.Address.Country) + }) + + t.Run("slice result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return []any{ + map[string]any{"name": "user1", "age": 20}, + map[string]any{"name": "user2", "age": 25}, + }, nil + } + + var users []testUser + err := service.InvokeWithType(context.Background(), "listUsers", []string{}, []hessian.Object{}, &users) + + require.NoError(t, err) + require.Len(t, users, 2) + assert.Equal(t, "user1", users[0].Name) + assert.Equal(t, 20, users[0].Age) + assert.Equal(t, "user2", users[1].Name) + assert.Equal(t, 25, users[1].Age) + }) + + t.Run("nil result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return nil, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"789"}, &user) + + require.NoError(t, err) + assert.Empty(t, user.Name) + assert.Zero(t, user.Age) + }) + + t.Run("nil reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, nil) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply cannot be nil") + }) + + t.Run("non-pointer reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, user) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply must be a pointer") + }) +} Review Comment: The test suite is missing a test case for when the generalizer's `Realize` method fails. This would occur when the result structure doesn't match the expected type. Add a test case to verify that `InvokeWithType` returns an appropriate error when deserialization fails, ensuring robust error handling. ########## filter/generic/filter_test.go: ########## @@ -101,3 +102,193 @@ func TestFilter_InvokeWithGenericCall(t *testing.T) { r := filter.Invoke(context.Background(), mockInvoker, genericInvocation) assert.NotNil(t, r) } + +// mockUser is a test struct for OnResponse deserialization +type mockUser struct { + Name string + Age int + Email string + Address *mockAddress +} + +type mockAddress struct { + City string + Country string +} + +// test OnResponse with struct deserialization +func TestFilter_OnResponse_WithStructDeserialization(t *testing.T) { + invokeUrl := common.NewURLWithOptions( + common.WithParams(url.Values{}), + common.WithParamsValue(constant.GenericKey, constant.GenericSerializationDefault)) + filter := &genericFilter{} + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockInvoker := mock.NewMockInvoker(ctrl) + mockInvoker.EXPECT().GetURL().Return(invokeUrl).AnyTimes() + + t.Run("simple struct deserialization", func(t *testing.T) { + // Create a reply pointer for the target struct + var user mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&user), + ) + + // Simulate a map result from generic call + mapResult := map[string]any{ + "name": "testUser", + "age": 25, + "email": "[email protected]", + } + res := &result.RPCResult{Rest: mapResult} + + // Call OnResponse + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // Verify the result is deserialized into user struct + require.NoError(t, newRes.Error()) + assert.Equal(t, "testUser", user.Name) + assert.Equal(t, 25, user.Age) + assert.Equal(t, "[email protected]", user.Email) + }) Review Comment: The test cases for successful deserialization (simple struct, nested struct, slice) do not verify what `newRes.Result()` returns after deserialization. While the tests verify that the reply parameter is correctly populated, they should also check that `newRes.Result()` contains the expected value. Add assertions to verify `newRes.Result()` in these test cases to ensure the complete behavior is tested. ########## filter/generic/service.go: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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 generic + +import ( + "context" + "reflect" +) + +import ( + hessian "github.com/apache/dubbo-go-hessian2" + + perrors "github.com/pkg/errors" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/filter/generic/generalizer" +) + +// GenericService uses for generic invoke for service call +type GenericService struct { + Invoke func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) `dubbo:"$invoke"` + referenceStr string +} + +// NewGenericService returns a GenericService instance +func NewGenericService(referenceStr string) *GenericService { + return &GenericService{referenceStr: referenceStr} +} + +// Reference gets referenceStr from GenericService +func (s *GenericService) Reference() string { + return s.referenceStr +} + +// InvokeWithType invokes the remote method and deserializes the result into the reply struct. +// The reply parameter must be a non-nil pointer to the target type. +// +// Example usage: +// +// var user User +// err := genericService.InvokeWithType(ctx, "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, &user) +// if err != nil { +// return err +// } +// fmt.Println(user.Name, user.Age) +func (s *GenericService) InvokeWithType(ctx context.Context, methodName string, types []string, args []hessian.Object, reply any) error { + if reply == nil { + return perrors.New("reply cannot be nil") + } + + replyValue := reflect.ValueOf(reply) + if replyValue.Kind() != reflect.Ptr { + return perrors.New("reply must be a pointer") + } + + if replyValue.IsNil() { + return perrors.New("reply cannot be a nil pointer") + } + + // Call the underlying Invoke method + result, err := s.Invoke(ctx, methodName, types, args) + if err != nil { + return err + } + + if result == nil { + return nil + } + + // Get the element type that the pointer points to + replyType := replyValue.Elem().Type() + + // Use MapGeneralizer to realize the map result to the target struct + g := generalizer.GetMapGeneralizer() + realized, err := g.Realize(result, replyType) + if err != nil { + return perrors.Errorf("failed to deserialize result to %s: %v", replyType.String(), err) + } + + // Set the realized value to reply + replyValue.Elem().Set(reflect.ValueOf(realized)) + return nil Review Comment: The deserialization logic in `InvokeWithType` is duplicated in the `OnResponse` method of `filter.go`. Both methods perform the same sequence: validate the reply pointer, get the element type, use the generalizer to realize the data, and set the value. Consider extracting this common logic into a shared helper function to improve maintainability and reduce code duplication. ########## filter/generic/service_test.go: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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 generic + +import ( + "context" + "testing" +) + +import ( + hessian "github.com/apache/dubbo-go-hessian2" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testUser struct { + Name string + Age int + Email string + Address *testAddress +} + +type testAddress struct { + City string + Country string +} + +func TestGenericService(t *testing.T) { + service := NewGenericService("HelloService") + reference := service.Reference() + assert.Equal(t, "HelloService", reference) +} + +func TestGenericService_InvokeWithType(t *testing.T) { + t.Run("simple struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + assert.Equal(t, "getUser", methodName) + return map[string]any{ + "name": "testUser", + "age": 25, + "email": "[email protected]", + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, &user) + + require.NoError(t, err) + assert.Equal(t, "testUser", user.Name) + assert.Equal(t, 25, user.Age) + assert.Equal(t, "[email protected]", user.Email) + }) + + t.Run("nested struct", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return map[string]any{ + "name": "nestedUser", + "age": 30, + "email": "[email protected]", + "address": map[string]any{ + "city": "Beijing", + "country": "China", + }, + }, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"456"}, &user) + + require.NoError(t, err) + assert.Equal(t, "nestedUser", user.Name) + assert.Equal(t, 30, user.Age) + require.NotNil(t, user.Address) + assert.Equal(t, "Beijing", user.Address.City) + assert.Equal(t, "China", user.Address.Country) + }) + + t.Run("slice result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return []any{ + map[string]any{"name": "user1", "age": 20}, + map[string]any{"name": "user2", "age": 25}, + }, nil + } + + var users []testUser + err := service.InvokeWithType(context.Background(), "listUsers", []string{}, []hessian.Object{}, &users) + + require.NoError(t, err) + require.Len(t, users, 2) + assert.Equal(t, "user1", users[0].Name) + assert.Equal(t, 20, users[0].Age) + assert.Equal(t, "user2", users[1].Name) + assert.Equal(t, 25, users[1].Age) + }) + + t.Run("nil result", func(t *testing.T) { + service := NewGenericService("TestService") + service.Invoke = func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) { + return nil, nil + } + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"789"}, &user) + + require.NoError(t, err) + assert.Empty(t, user.Name) + assert.Zero(t, user.Age) + }) + + t.Run("nil reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, nil) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply cannot be nil") + }) + + t.Run("non-pointer reply error", func(t *testing.T) { + service := NewGenericService("TestService") + + var user testUser + err := service.InvokeWithType(context.Background(), "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, user) + + require.Error(t, err) + assert.Contains(t, err.Error(), "reply must be a pointer") + }) Review Comment: In the error test cases for nil reply and non-pointer reply, the `service.Invoke` function is not set. When `InvokeWithType` is called on line 133 and 143, it will attempt to call `s.Invoke` (line 77 in service.go), which would cause a nil pointer dereference panic. These test cases should either set a mock Invoke function or test a different error path. Add `service.Invoke = func(...) (any, error) { return nil, nil }` before calling `InvokeWithType` in these tests. ########## filter/generic/service.go: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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 generic + +import ( + "context" + "reflect" +) + +import ( + hessian "github.com/apache/dubbo-go-hessian2" + + perrors "github.com/pkg/errors" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/filter/generic/generalizer" +) + +// GenericService uses for generic invoke for service call +type GenericService struct { + Invoke func(ctx context.Context, methodName string, types []string, args []hessian.Object) (any, error) `dubbo:"$invoke"` + referenceStr string +} + +// NewGenericService returns a GenericService instance +func NewGenericService(referenceStr string) *GenericService { + return &GenericService{referenceStr: referenceStr} +} + +// Reference gets referenceStr from GenericService +func (s *GenericService) Reference() string { + return s.referenceStr +} + +// InvokeWithType invokes the remote method and deserializes the result into the reply struct. +// The reply parameter must be a non-nil pointer to the target type. +// +// Example usage: +// +// var user User +// err := genericService.InvokeWithType(ctx, "getUser", []string{"java.lang.String"}, []hessian.Object{"123"}, &user) +// if err != nil { +// return err +// } +// fmt.Println(user.Name, user.Age) +func (s *GenericService) InvokeWithType(ctx context.Context, methodName string, types []string, args []hessian.Object, reply any) error { + if reply == nil { + return perrors.New("reply cannot be nil") + } + + replyValue := reflect.ValueOf(reply) + if replyValue.Kind() != reflect.Ptr { + return perrors.New("reply must be a pointer") + } + + if replyValue.IsNil() { + return perrors.New("reply cannot be a nil pointer") + } + + // Call the underlying Invoke method + result, err := s.Invoke(ctx, methodName, types, args) + if err != nil { + return err + } + + if result == nil { + return nil + } + + // Get the element type that the pointer points to + replyType := replyValue.Elem().Type() + + // Use MapGeneralizer to realize the map result to the target struct + g := generalizer.GetMapGeneralizer() Review Comment: The `InvokeWithType` method always uses `GetMapGeneralizer()` for deserialization, which means it only supports map-based generic serialization. If users are using other serialization types like Gson or Protobuf-JSON, this method will not work correctly. Consider either documenting this limitation in the method's documentation or enhancing the method to support different serialization types by accepting an additional parameter or using configuration. ########## filter/generic/filter_test.go: ########## @@ -101,3 +102,193 @@ func TestFilter_InvokeWithGenericCall(t *testing.T) { r := filter.Invoke(context.Background(), mockInvoker, genericInvocation) assert.NotNil(t, r) } + +// mockUser is a test struct for OnResponse deserialization +type mockUser struct { + Name string + Age int + Email string + Address *mockAddress +} + +type mockAddress struct { + City string + Country string +} + +// test OnResponse with struct deserialization +func TestFilter_OnResponse_WithStructDeserialization(t *testing.T) { + invokeUrl := common.NewURLWithOptions( + common.WithParams(url.Values{}), + common.WithParamsValue(constant.GenericKey, constant.GenericSerializationDefault)) + filter := &genericFilter{} + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockInvoker := mock.NewMockInvoker(ctrl) + mockInvoker.EXPECT().GetURL().Return(invokeUrl).AnyTimes() + + t.Run("simple struct deserialization", func(t *testing.T) { + // Create a reply pointer for the target struct + var user mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&user), + ) + + // Simulate a map result from generic call + mapResult := map[string]any{ + "name": "testUser", + "age": 25, + "email": "[email protected]", + } + res := &result.RPCResult{Rest: mapResult} + + // Call OnResponse + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // Verify the result is deserialized into user struct + require.NoError(t, newRes.Error()) + assert.Equal(t, "testUser", user.Name) + assert.Equal(t, 25, user.Age) + assert.Equal(t, "[email protected]", user.Email) + }) + + t.Run("nested struct deserialization", func(t *testing.T) { + var user mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&user), + ) + + // Simulate a nested map result + mapResult := map[string]any{ + "name": "nestedUser", + "age": 30, + "email": "[email protected]", + "address": map[string]any{ + "city": "Beijing", + "country": "China", + }, + } + res := &result.RPCResult{Rest: mapResult} + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + require.NoError(t, newRes.Error()) + assert.Equal(t, "nestedUser", user.Name) + assert.Equal(t, 30, user.Age) + require.NotNil(t, user.Address) + assert.Equal(t, "Beijing", user.Address.City) + assert.Equal(t, "China", user.Address.Country) + }) + + t.Run("nil reply - no deserialization", func(t *testing.T) { + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(nil), + ) + + mapResult := map[string]any{"name": "test"} + res := &result.RPCResult{Rest: mapResult} + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // Result should remain as map since no reply pointer provided + assert.Equal(t, mapResult, newRes.Result()) + }) + + t.Run("nil result - no deserialization", func(t *testing.T) { + var user mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&user), + ) + + res := &result.RPCResult{Rest: nil} + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // User should remain zero value + assert.Empty(t, user.Name) + assert.Equal(t, 0, user.Age) + assert.Nil(t, newRes.Result()) + }) + + t.Run("primitive result - no deserialization", func(t *testing.T) { + var count int + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&count), + ) + + // Primitive result should pass through without deserialization + res := &result.RPCResult{Rest: 42} + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // Result should remain as primitive + assert.Equal(t, 42, newRes.Result()) + }) + + t.Run("error result - no deserialization", func(t *testing.T) { + var user mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&user), + ) + + res := &result.RPCResult{ + Rest: map[string]any{"name": "test"}, + Err: assert.AnError, + } + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + // Should return immediately without deserialization when there's an error + require.Error(t, newRes.Error()) + assert.Empty(t, user.Name) + }) +} + +func TestFilter_OnResponse_WithSliceDeserialization(t *testing.T) { + invokeUrl := common.NewURLWithOptions( + common.WithParams(url.Values{}), + common.WithParamsValue(constant.GenericKey, constant.GenericSerializationDefault)) + filter := &genericFilter{} + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockInvoker := mock.NewMockInvoker(ctrl) + mockInvoker.EXPECT().GetURL().Return(invokeUrl).AnyTimes() + + var users []mockUser + inv := invocation.NewRPCInvocationWithOptions( + invocation.WithMethodName(constant.Generic), + invocation.WithReply(&users), + ) + + // Simulate a slice of maps result + sliceResult := []any{ + map[string]any{ + "name": "user1", + "age": 20, + }, + map[string]any{ + "name": "user2", + "age": 25, + }, + } + res := &result.RPCResult{Rest: sliceResult} + + newRes := filter.OnResponse(context.Background(), res, mockInvoker, inv) + + require.NoError(t, newRes.Error()) + require.Len(t, users, 2) + assert.Equal(t, "user1", users[0].Name) + assert.Equal(t, 20, users[0].Age) + assert.Equal(t, "user2", users[1].Name) + assert.Equal(t, 25, users[1].Age) +} Review Comment: The test suite is missing a test case for when the generalizer's `Realize` method fails during the `OnResponse` processing. Add a test case that provides incompatible data that cannot be deserialized to the target type, and verify that the filter handles this error gracefully by logging a warning and returning the original result. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
