diff --git a/baseapp/grpcrouter.go b/baseapp/grpcrouter.go index a32da67581..566333b2a4 100644 --- a/baseapp/grpcrouter.go +++ b/baseapp/grpcrouter.go @@ -54,13 +54,31 @@ func (qrt *GRPCQueryRouter) Route(path string) GRPCQueryHandler { } // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC -// service description, handler is an object which implements that gRPC service +// service description, handler is an object which implements that gRPC service/ +// +// This functions PANICS: +// - if a protobuf service is registered twice. func (qrt *GRPCQueryRouter) RegisterService(sd *grpc.ServiceDesc, handler interface{}) { // adds a top-level query handler based on the gRPC service name for _, method := range sd.Methods { fqName := fmt.Sprintf("/%s/%s", sd.ServiceName, method.MethodName) methodHandler := method.Handler + // Check that each service is only registered once. If a service is + // registered more than once, then we should error. Since we can't + // return an error (`Server.RegisterService` interface restriction) we + // panic (at startup). + _, found := qrt.routes[fqName] + if found { + panic( + fmt.Errorf( + "gRPC query service %s has already been registered. Please make sure to only register each service once. "+ + "This usually means that there are conflicting modules registering the same gRPC query service", + fqName, + ), + ) + } + qrt.routes[fqName] = func(ctx sdk.Context, req abci.RequestQuery) (abci.ResponseQuery, error) { // call the method handler from the service description with the handler object, // a wrapped sdk.Context with proto-unmarshaled data from the ABCI request data diff --git a/baseapp/grpcrouter_helpers.go b/baseapp/grpcrouter_helpers.go index bd7d498e7a..2ea74b55fc 100644 --- a/baseapp/grpcrouter_helpers.go +++ b/baseapp/grpcrouter_helpers.go @@ -18,7 +18,7 @@ import ( // service client. type QueryServiceTestHelper struct { *GRPCQueryRouter - ctx sdk.Context + Ctx sdk.Context } var ( @@ -31,7 +31,7 @@ var ( func NewQueryServerTestHelper(ctx sdk.Context, interfaceRegistry types.InterfaceRegistry) *QueryServiceTestHelper { qrt := NewGRPCQueryRouter() qrt.SetInterfaceRegistry(interfaceRegistry) - return &QueryServiceTestHelper{GRPCQueryRouter: qrt, ctx: ctx} + return &QueryServiceTestHelper{GRPCQueryRouter: qrt, Ctx: ctx} } // Invoke implements the grpc ClientConn.Invoke method @@ -45,7 +45,7 @@ func (q *QueryServiceTestHelper) Invoke(_ gocontext.Context, method string, args return err } - res, err := querier(q.ctx, abci.RequestQuery{Data: reqBz}) + res, err := querier(q.Ctx, abci.RequestQuery{Data: reqBz}) if err != nil { return err } diff --git a/baseapp/grpcrouter_test.go b/baseapp/grpcrouter_test.go index d2051a1132..6a83939b4a 100644 --- a/baseapp/grpcrouter_test.go +++ b/baseapp/grpcrouter_test.go @@ -1,24 +1,29 @@ -package baseapp +package baseapp_test import ( "context" + "os" "testing" "github.com/stretchr/testify/require" + "github.com/tendermint/tendermint/libs/log" + dbm "github.com/tendermint/tm-db" + "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/codec/types" + "github.com/cosmos/cosmos-sdk/simapp" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" ) func TestGRPCRouter(t *testing.T) { - qr := NewGRPCQueryRouter() + qr := baseapp.NewGRPCQueryRouter() interfaceRegistry := testdata.NewTestInterfaceRegistry() qr.SetInterfaceRegistry(interfaceRegistry) testdata.RegisterQueryServer(qr, testdata.QueryImpl{}) - helper := &QueryServiceTestHelper{ + helper := &baseapp.QueryServiceTestHelper{ GRPCQueryRouter: qr, - ctx: sdk.Context{}.WithContext(context.Background()), + Ctx: sdk.Context{}.WithContext(context.Background()), } client := testdata.NewQueryClient(helper) @@ -44,3 +49,28 @@ func TestGRPCRouter(t *testing.T) { require.NotNil(t, res3) require.Equal(t, spot, res3.HasAnimal.Animal.GetCachedValue()) } + +func TestRegisterQueryServiceTwice(t *testing.T) { + // Setup baseapp. + db := dbm.NewMemDB() + encCfg := simapp.MakeTestEncodingConfig() + app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder()) + app.SetInterfaceRegistry(encCfg.InterfaceRegistry) + testdata.RegisterInterfaces(encCfg.InterfaceRegistry) + + // First time registering service shouldn't panic. + require.NotPanics(t, func() { + testdata.RegisterQueryServer( + app.GRPCQueryRouter(), + testdata.QueryImpl{}, + ) + }) + + // Second time should panic. + require.Panics(t, func() { + testdata.RegisterQueryServer( + app.GRPCQueryRouter(), + testdata.QueryImpl{}, + ) + }) +} diff --git a/baseapp/msg_service_router.go b/baseapp/msg_service_router.go index b883faa37b..ea2ed4b4eb 100644 --- a/baseapp/msg_service_router.go +++ b/baseapp/msg_service_router.go @@ -40,8 +40,10 @@ func (msr *MsgServiceRouter) Handler(methodName string) MsgServiceHandler { // RegisterService implements the gRPC Server.RegisterService method. sd is a gRPC // service description, handler is an object which implements that gRPC service. // -// This function PANICs if it is called before the service `Msg`s have been -// registered using RegisterInterfaces. +// This function PANICs: +// - if it is called before the service `Msg`s have been registered using +// RegisterInterfaces, +// - or if a service is being registered twice. func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler interface{}) { // Adds a top-level query handler based on the gRPC service name. for _, method := range sd.Methods { @@ -66,6 +68,21 @@ func (msr *MsgServiceRouter) RegisterService(sd *grpc.ServiceDesc, handler inter ) } + // Check that each service is only registered once. If a service is + // registered more than once, then we should error. Since we can't + // return an error (`Server.RegisterService` interface restriction) we + // panic (at startup). + _, found := msr.routes[fqMethod] + if found { + panic( + fmt.Errorf( + "msg service %s has already been registered. Please make sure to only register each service once. "+ + "This usually means that there are conflicting modules registering the same msg service", + fqMethod, + ), + ) + } + msr.routes[fqMethod] = func(ctx sdk.Context, req sdk.MsgRequest) (*sdk.Result, error) { ctx = ctx.WithEventManager(sdk.NewEventManager()) interceptor := func(goCtx context.Context, _ interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { diff --git a/baseapp/msg_service_router_test.go b/baseapp/msg_service_router_test.go index bbbf9e1356..34f9c08027 100644 --- a/baseapp/msg_service_router_test.go +++ b/baseapp/msg_service_router_test.go @@ -18,7 +18,7 @@ import ( authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" ) -func TestRegisterService(t *testing.T) { +func TestRegisterMsgService(t *testing.T) { db := dbm.NewMemDB() // Create an encoding config that doesn't register testdata Msg services. @@ -42,6 +42,31 @@ func TestRegisterService(t *testing.T) { }) } +func TestRegisterMsgServiceTwice(t *testing.T) { + // Setup baseapp. + db := dbm.NewMemDB() + encCfg := simapp.MakeTestEncodingConfig() + app := baseapp.NewBaseApp("test", log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, encCfg.TxConfig.TxDecoder()) + app.SetInterfaceRegistry(encCfg.InterfaceRegistry) + testdata.RegisterInterfaces(encCfg.InterfaceRegistry) + + // First time registering service shouldn't panic. + require.NotPanics(t, func() { + testdata.RegisterMsgServer( + app.MsgServiceRouter(), + testdata.MsgServerImpl{}, + ) + }) + + // Second time should panic. + require.Panics(t, func() { + testdata.RegisterMsgServer( + app.MsgServiceRouter(), + testdata.MsgServerImpl{}, + ) + }) +} + func TestMsgService(t *testing.T) { priv, _, _ := testdata.KeyTestPubAddr() encCfg := simapp.MakeTestEncodingConfig() diff --git a/client/grpc/reflection/reflection_test.go b/client/grpc/reflection/reflection_test.go index 037e7e06ae..211fc397ad 100644 --- a/client/grpc/reflection/reflection_test.go +++ b/client/grpc/reflection/reflection_test.go @@ -21,14 +21,9 @@ type IntegrationTestSuite struct { func (s *IntegrationTestSuite) SetupSuite() { app := simapp.Setup(false) - srv := reflection.NewReflectionServiceServer(app.InterfaceRegistry()) - sdkCtx := app.BaseApp.NewContext(false, tmproto.Header{}) queryHelper := baseapp.NewQueryServerTestHelper(sdkCtx, app.InterfaceRegistry()) - - reflection.RegisterReflectionServiceServer(queryHelper, srv) queryClient := reflection.NewReflectionServiceClient(queryHelper) - s.queryClient = queryClient } diff --git a/codec/types/interface_registry.go b/codec/types/interface_registry.go index 57650af61d..0f9eb760be 100644 --- a/codec/types/interface_registry.go +++ b/codec/types/interface_registry.go @@ -115,6 +115,11 @@ func (registry *interfaceRegistry) RegisterInterface(protoName string, iface int registry.RegisterImplementations(iface, impls...) } +// RegisterImplementations registers a concrete proto Message which implements +// the given interface. +// +// This function PANICs if different concrete types are registered under the +// same typeURL. func (registry *interfaceRegistry) RegisterImplementations(iface interface{}, impls ...proto.Message) { for _, impl := range impls { typeURL := "/" + proto.MessageName(impl) @@ -122,10 +127,20 @@ func (registry *interfaceRegistry) RegisterImplementations(iface interface{}, im } } +// RegisterCustomTypeURL registers a concrete type which implements the given +// interface under `typeURL`. +// +// This function PANICs if different concrete types are registered under the +// same typeURL. func (registry *interfaceRegistry) RegisterCustomTypeURL(iface interface{}, typeURL string, impl proto.Message) { registry.registerImpl(iface, typeURL, impl) } +// registerImpl registers a concrete type which implements the given +// interface under `typeURL`. +// +// This function PANICs if different concrete types are registered under the +// same typeURL. func (registry *interfaceRegistry) registerImpl(iface interface{}, typeURL string, impl proto.Message) { ityp := reflect.TypeOf(iface).Elem() imap, found := registry.interfaceImpls[ityp] @@ -138,6 +153,24 @@ func (registry *interfaceRegistry) registerImpl(iface interface{}, typeURL strin panic(fmt.Errorf("type %T doesn't actually implement interface %+v", impl, ityp)) } + // Check if we already registered something under the given typeURL. It's + // okay to register the same concrete type again, but if we are registering + // a new concrete type under the same typeURL, then we throw an error (here, + // we panic). + foundImplType, found := imap[typeURL] + if found && foundImplType != implType { + panic( + fmt.Errorf( + "concrete type %s has already been registered under typeURL %s, cannot register %s under same typeURL. "+ + "This usually means that there are conflicting modules registering different concrete types "+ + "for a same interface implementation", + foundImplType, + typeURL, + implType, + ), + ) + } + imap[typeURL] = implType registry.typeURLMap[typeURL] = implType diff --git a/codec/types/types_test.go b/codec/types/types_test.go index 5e2a0f7f77..1ff03a5f56 100644 --- a/codec/types/types_test.go +++ b/codec/types/types_test.go @@ -48,22 +48,61 @@ type TestI interface { DoSomething() } +// A struct that has the same typeURL as testdata.Dog, but is actually another +// concrete type. +type FakeDog struct{} + +var ( + _ proto.Message = &FakeDog{} + _ testdata.Animal = &FakeDog{} +) + +// dummy implementation of proto.Message and testdata.Animal +func (dog FakeDog) Reset() {} +func (dog FakeDog) String() string { return "fakedog" } +func (dog FakeDog) ProtoMessage() {} +func (dog FakeDog) XXX_MessageName() string { return proto.MessageName(&testdata.Dog{}) } +func (dog FakeDog) Greet() string { return "fakedog" } + func TestRegister(t *testing.T) { registry := types.NewInterfaceRegistry() registry.RegisterInterface("Animal", (*testdata.Animal)(nil)) registry.RegisterInterface("TestI", (*TestI)(nil)) + + // Happy path. require.NotPanics(t, func() { registry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Dog{}) }) + + // testdata.Dog doesn't implement TestI require.Panics(t, func() { registry.RegisterImplementations((*TestI)(nil), &testdata.Dog{}) }) + + // nil proto message require.Panics(t, func() { registry.RegisterImplementations((*TestI)(nil), nil) }) + + // Not an interface. require.Panics(t, func() { registry.RegisterInterface("not_an_interface", (*testdata.Dog)(nil)) }) + + // Duplicate registration with same concrete type. + require.NotPanics(t, func() { + registry.RegisterImplementations((*testdata.Animal)(nil), &testdata.Dog{}) + }) + + // Duplicate registration with different concrete type on same typeURL. + require.PanicsWithError( + t, + "concrete type *testdata.Dog has already been registered under typeURL /testdata.Dog, cannot register *types_test.FakeDog under same typeURL. "+ + "This usually means that there are conflicting modules registering different concrete types for a same interface implementation", + func() { + registry.RegisterImplementations((*testdata.Animal)(nil), &FakeDog{}) + }, + ) } func TestUnpackInterfaces(t *testing.T) {