Skip to content

Commit

Permalink
fix(baseapp): hybrid handlers do not drop any (#22866)
Browse files Browse the repository at this point in the history
  • Loading branch information
testinginprod authored Dec 13, 2024
1 parent 957e241 commit 25cbf98
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 18 deletions.
25 changes: 25 additions & 0 deletions baseapp/grpcrouter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,31 @@ func TestGRPCRouterHybridHandlers(t *testing.T) {
}
assertRouterBehaviour(helper)
})

t.Run("any cached value is not dropped", func(t *testing.T) {
// ref: https://github.com/cosmos/cosmos-sdk/issues/22779
qr := baseapp.NewGRPCQueryRouter()
interfaceRegistry := testdata.NewTestInterfaceRegistry()
testdata.RegisterInterfaces(interfaceRegistry)
qr.SetInterfaceRegistry(interfaceRegistry)
testdata.RegisterQueryServer(qr, testdata.QueryImpl{})
helper := &baseapp.QueryServiceTestHelper{
GRPCQueryRouter: qr,
Ctx: sdk.Context{}.WithContext(context.Background()),
}

anyMsg, err := types.NewAnyWithValue(&testdata.Dog{})
require.NoError(t, err)

handler := qr.HybridHandlerByRequestName("testpb.TestAnyRequest")[0]

resp := new(testdata.TestAnyResponse)
err = handler(helper.Ctx, &testdata.TestAnyRequest{
AnyAnimal: anyMsg,
}, resp)
require.NoError(t, err)
require.NotNil(t, resp.HasAnimal.Animal.GetCachedValue())
})
}

func TestRegisterQueryServiceTwice(t *testing.T) {
Expand Down
28 changes: 10 additions & 18 deletions baseapp/internal/protocompat/protocompat.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"reflect"

gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/golang/protobuf/proto" //nolint: staticcheck // needed because gogoproto.Merge does not work consistently. See NOTE: comments.
"google.golang.org/grpc"
proto2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
Expand Down Expand Up @@ -124,19 +123,13 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B
return fmt.Errorf("invalid request type %T, method %s does not accept protov2 messages", inReq, prefMethod.FullName())
}
resp, err := method.Handler(handler, ctx, func(msg any) error {
// merge! ref: https://github.com/cosmos/cosmos-sdk/issues/18003
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but
// using proto.Merge with gogo messages seems to work fine.
proto.Merge(msg.(gogoproto.Message), inReq)
setPointer(msg, inReq)
return nil
}, nil)
if err != nil {
return err
}
// merge resp, ref: https://github.com/cosmos/cosmos-sdk/issues/18003
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but
// using proto.Merge with gogo messages seems to work fine.
proto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message))
setPointer(outResp, resp)
return nil
}, nil
}
Expand Down Expand Up @@ -168,20 +161,13 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B
case gogoproto.Message:
// we can just call the handler after making a copy of the message, for safety reasons.
resp, err := method.Handler(handler, ctx, func(msg any) error {
// ref: https://github.com/cosmos/cosmos-sdk/issues/18003
asGogoProto := msg.(gogoproto.Message)
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but
// using proto.Merge with gogo messages seems to work fine.
proto.Merge(asGogoProto, m)
setPointer(msg, m)
return nil
}, nil)
if err != nil {
return err
}
// merge on the resp, ref: https://github.com/cosmos/cosmos-sdk/issues/18003
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but
// using proto.Merge with gogo messages seems to work fine.
proto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message))
setPointer(outResp, resp)
return nil
default:
panic("unreachable")
Expand Down Expand Up @@ -246,3 +232,9 @@ func ResponseFullNameFromMethodDesc(sd *grpc.ServiceDesc, method grpc.MethodDesc
}
return methodDesc.Output().FullName(), nil
}

// since proto.Merge breaks due to the custom cosmos sdk any, we are forced to do this ugly setPointer hack.
// ref: https://github.com/cosmos/cosmos-sdk/issues/22779
func setPointer(dst, src any) {
reflect.ValueOf(dst).Elem().Set(reflect.ValueOf(src).Elem())
}

0 comments on commit 25cbf98

Please sign in to comment.