From 49597b19ec295aa7104bf12655819dbd8c06cd15 Mon Sep 17 00:00:00 2001 From: Jonathan Gimeno Date: Thu, 11 Jun 2020 17:37:23 +0200 Subject: [PATCH] module: pass route by value (#6404) * use instance * add some comments * Update types/router.go * rename Nil to Empty * run make mocks Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Alessio Treglia --- baseapp/baseapp_test.go | 2 +- baseapp/router.go | 2 +- tests/mocks/tendermint_tm_db_DB.go | 14 ++++----- tests/mocks/types_module_module.go | 4 +-- tests/mocks/types_router.go | 2 +- types/module/module.go | 6 ++-- types/module/module_test.go | 17 +++++++---- types/router.go | 19 +++++++++--- types/router_test.go | 46 ++++++++++++++++++++++++++++++ x/auth/module.go | 2 +- x/bank/module.go | 2 +- x/capability/module.go | 2 +- x/crisis/module.go | 2 +- x/distribution/module.go | 2 +- x/evidence/module.go | 2 +- x/gov/module.go | 2 +- x/ibc-transfer/module.go | 2 +- x/ibc/module.go | 2 +- x/mint/module.go | 2 +- x/params/module.go | 2 +- x/slashing/module.go | 2 +- x/staking/module.go | 2 +- x/upgrade/module.go | 2 +- 23 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 types/router_test.go diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index 22e0acd3cd7..7b555641e44 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -1650,7 +1650,7 @@ type testCustomRouter struct { routes sync.Map } -func (rtr *testCustomRouter) AddRoute(route *sdk.Route) sdk.Router { +func (rtr *testCustomRouter) AddRoute(route sdk.Route) sdk.Router { rtr.routes.Store(route.Path(), route.Handler()) return rtr } diff --git a/baseapp/router.go b/baseapp/router.go index 4108afe36c3..7e2e70a0c6f 100644 --- a/baseapp/router.go +++ b/baseapp/router.go @@ -21,7 +21,7 @@ func NewRouter() *Router { // AddRoute adds a route path to the router with a given handler. The route must // be alphanumeric. -func (rtr *Router) AddRoute(route *sdk.Route) sdk.Router { +func (rtr *Router) AddRoute(route sdk.Route) sdk.Router { if !sdk.IsAlphaNumeric(route.Path()) { panic("route expressions can only contain alphanumeric characters") } diff --git a/tests/mocks/tendermint_tm_db_DB.go b/tests/mocks/tendermint_tm_db_DB.go index cdf5e5ab0a2..bed59a498d0 100644 --- a/tests/mocks/tendermint_tm_db_DB.go +++ b/tests/mocks/tendermint_tm_db_DB.go @@ -6,7 +6,7 @@ package mocks import ( gomock "github.com/golang/mock/gomock" - db "github.com/tendermint/tm-db" + tm_db "github.com/tendermint/tm-db" reflect "reflect" ) @@ -106,10 +106,10 @@ func (mr *MockDBMockRecorder) Has(arg0 interface{}) *gomock.Call { } // Iterator mocks base method -func (m *MockDB) Iterator(arg0, arg1 []byte) (db.Iterator, error) { +func (m *MockDB) Iterator(arg0, arg1 []byte) (tm_db.Iterator, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Iterator", arg0, arg1) - ret0, _ := ret[0].(db.Iterator) + ret0, _ := ret[0].(tm_db.Iterator) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -121,10 +121,10 @@ func (mr *MockDBMockRecorder) Iterator(arg0, arg1 interface{}) *gomock.Call { } // NewBatch mocks base method -func (m *MockDB) NewBatch() db.Batch { +func (m *MockDB) NewBatch() tm_db.Batch { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "NewBatch") - ret0, _ := ret[0].(db.Batch) + ret0, _ := ret[0].(tm_db.Batch) return ret0 } @@ -149,10 +149,10 @@ func (mr *MockDBMockRecorder) Print() *gomock.Call { } // ReverseIterator mocks base method -func (m *MockDB) ReverseIterator(arg0, arg1 []byte) (db.Iterator, error) { +func (m *MockDB) ReverseIterator(arg0, arg1 []byte) (tm_db.Iterator, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ReverseIterator", arg0, arg1) - ret0, _ := ret[0].(db.Iterator) + ret0, _ := ret[0].(tm_db.Iterator) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/tests/mocks/types_module_module.go b/tests/mocks/types_module_module.go index d2663f5d4c9..a56300ab453 100644 --- a/tests/mocks/types_module_module.go +++ b/tests/mocks/types_module_module.go @@ -437,10 +437,10 @@ func (mr *MockAppModuleMockRecorder) RegisterInvariants(arg0 interface{}) *gomoc } // Route mocks base method -func (m *MockAppModule) Route() *types.Route { +func (m *MockAppModule) Route() types.Route { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Route") - ret0, _ := ret[0].(*types.Route) + ret0, _ := ret[0].(types.Route) return ret0 } diff --git a/tests/mocks/types_router.go b/tests/mocks/types_router.go index 33fe6edee67..48f96b0b162 100644 --- a/tests/mocks/types_router.go +++ b/tests/mocks/types_router.go @@ -34,7 +34,7 @@ func (m *MockRouter) EXPECT() *MockRouterMockRecorder { } // AddRoute mocks base method -func (m *MockRouter) AddRoute(r *types.Route) types.Router { +func (m *MockRouter) AddRoute(r types.Route) types.Router { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AddRoute", r) ret0, _ := ret[0].(types.Router) diff --git a/types/module/module.go b/types/module/module.go index a46bb74b706..d95db1d5ce4 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -141,7 +141,7 @@ type AppModule interface { RegisterInvariants(sdk.InvariantRegistry) // routes - Route() *sdk.Route + Route() sdk.Route // Deprecated: use RegisterQueryService QuerierRoute() string // Deprecated: use RegisterQueryService @@ -172,7 +172,7 @@ func NewGenesisOnlyAppModule(amg AppModuleGenesis) AppModule { func (GenesisOnlyAppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route empty module message route -func (GenesisOnlyAppModule) Route() *sdk.Route { return nil } +func (GenesisOnlyAppModule) Route() sdk.Route { return sdk.Route{} } // QuerierRoute returns an empty module querier route func (GenesisOnlyAppModule) QuerierRoute() string { return "" } @@ -251,7 +251,7 @@ func (m *Manager) RegisterInvariants(ir sdk.InvariantRegistry) { // RegisterRoutes registers all module routes and module querier routes func (m *Manager) RegisterRoutes(router sdk.Router, queryRouter sdk.QueryRouter) { for _, module := range m.Modules { - if module.Route() != nil { + if !module.Route().Empty() { router.AddRoute(module.Route()) } if module.QuerierRoute() != "" { diff --git a/types/module/module_test.go b/types/module/module_test.go index ead9fa3afe3..e751889c71a 100644 --- a/types/module/module_test.go +++ b/types/module/module_test.go @@ -69,7 +69,7 @@ func TestGenesisOnlyAppModule(t *testing.T) { mockInvariantRegistry := mocks.NewMockInvariantRegistry(mockCtrl) goam := module.NewGenesisOnlyAppModule(mockModule) - require.Nil(t, goam.Route()) + require.True(t, goam.Route().Empty()) require.Empty(t, goam.QuerierRoute()) require.Nil(t, goam.NewQuerierHandler()) @@ -140,11 +140,16 @@ func TestManager_RegisterRoutes(t *testing.T) { require.Equal(t, 2, len(mm.Modules)) router := mocks.NewMockRouter(mockCtrl) - handler1, handler2 := sdk.Handler(nil), sdk.Handler(nil) - mockAppModule1.EXPECT().Route().Times(2).Return(sdk.NewRoute("route1", handler1)) - mockAppModule2.EXPECT().Route().Times(2).Return(sdk.NewRoute("route2", handler2)) - router.EXPECT().AddRoute(gomock.Eq(sdk.NewRoute("route1", handler1))).Times(1) - router.EXPECT().AddRoute(gomock.Eq(sdk.NewRoute("route2", handler2))).Times(1) + handler1, handler2 := sdk.Handler(func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { + return nil,nil + }), sdk.Handler(func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { + return nil,nil + }) + route1 := sdk.NewRoute("route1", handler1) + route2 := sdk.NewRoute("route2", handler2) + mockAppModule1.EXPECT().Route().Times(2).Return(route1) + mockAppModule2.EXPECT().Route().Times(2).Return(route2) + router.EXPECT().AddRoute(gomock.Any()).Times(2) // Use of Any due to limitations to compare Functions as the sdk.Handler queryRouter := mocks.NewMockQueryRouter(mockCtrl) mockAppModule1.EXPECT().QuerierRoute().Times(2).Return("querierRoute1") diff --git a/types/router.go b/types/router.go index d009e2afa22..27c2127d5f2 100644 --- a/types/router.go +++ b/types/router.go @@ -1,6 +1,9 @@ package types -import "regexp" +import ( + "regexp" + "strings" +) var ( // IsAlphaNumeric defines a regular expression for matching against alpha-numeric @@ -26,7 +29,7 @@ var ( // Router provides handlers for each transaction type. type Router interface { - AddRoute(r *Route) Router + AddRoute(r Route) Router Route(ctx Context, path string) Handler } @@ -35,18 +38,26 @@ type Route struct { handler Handler } -func NewRoute(p string, h Handler) *Route { - return &Route{path: p, handler: h} +// NewRoute returns an instance of Route. +func NewRoute(p string, h Handler) Route { + return Route{path: p, handler: h} } +// Path returns the path the route has assigned. func (r Route) Path() string { return r.path } +// Handler returns the handler that handles the route. func (r Route) Handler() Handler { return r.handler } +// Empty returns true only if both handler and path are not empty. +func (r Route) Empty() bool { + return r.handler == nil || r.path == strings.TrimSpace("") +} + // QueryRouter provides queryables for each query path. type QueryRouter interface { AddRoute(r string, h Querier) QueryRouter diff --git a/types/router_test.go b/types/router_test.go new file mode 100644 index 00000000000..4721fed6da4 --- /dev/null +++ b/types/router_test.go @@ -0,0 +1,46 @@ +package types + +import ( + "github.com/stretchr/testify/assert" + "testing" +) + +func TestNilRoute(t *testing.T) { + tests := []struct{ + name string + route Route + expected bool + } { + { + name: "all empty", + route: NewRoute("", nil), + expected: true, + }, + { + name: "only path", + route: NewRoute("some", nil), + expected: true, + }, + { + name: "only handler", + route: NewRoute("", func(ctx Context, msg Msg) (*Result, error) { + return nil, nil + }), + expected: true, + }, + { + name: "correct route", + route: NewRoute("some", func(ctx Context, msg Msg) (*Result, error) { + return nil, nil + }), + expected: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.route.Empty()) + }) + } +} diff --git a/x/auth/module.go b/x/auth/module.go index d448304e7cb..afb93d73adf 100644 --- a/x/auth/module.go +++ b/x/auth/module.go @@ -105,7 +105,7 @@ func (AppModule) Name() string { func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route returns the message routing key for the auth module. -func (AppModule) Route() *sdk.Route { return nil } +func (AppModule) Route() sdk.Route { return sdk.Route{} } // QuerierRoute returns the auth module's querier route name. func (AppModule) QuerierRoute() string { diff --git a/x/bank/module.go b/x/bank/module.go index 0d7daff9d6c..65cd34f6c0f 100644 --- a/x/bank/module.go +++ b/x/bank/module.go @@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route returns the message routing key for the bank module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/capability/module.go b/x/capability/module.go index e3d4a865d2d..e99699db879 100644 --- a/x/capability/module.go +++ b/x/capability/module.go @@ -95,7 +95,7 @@ func (am AppModule) Name() string { } // Route returns the capability module's message routing key. -func (AppModule) Route() *sdk.Route { return nil } +func (AppModule) Route() sdk.Route { return sdk.Route{} } // QuerierRoute returns the capability module's query routing key. func (AppModule) QuerierRoute() string { return "" } diff --git a/x/crisis/module.go b/x/crisis/module.go index d32614c00e4..8bf20520297 100644 --- a/x/crisis/module.go +++ b/x/crisis/module.go @@ -94,7 +94,7 @@ func (AppModule) Name() string { func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route returns the message routing key for the crisis module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(*am.keeper)) } diff --git a/x/distribution/module.go b/x/distribution/module.go index 7c1dc66ef47..e350e53b890 100644 --- a/x/distribution/module.go +++ b/x/distribution/module.go @@ -119,7 +119,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route returns the message routing key for the distribution module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/evidence/module.go b/x/evidence/module.go index 96af56883c5..bef19f5d44a 100644 --- a/x/evidence/module.go +++ b/x/evidence/module.go @@ -127,7 +127,7 @@ func (am AppModule) Name() string { } // Route returns the evidence module's message routing key. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/gov/module.go b/x/gov/module.go index 8ae5b938866..2a046a98fd3 100644 --- a/x/gov/module.go +++ b/x/gov/module.go @@ -135,7 +135,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route returns the message routing key for the gov module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/ibc-transfer/module.go b/x/ibc-transfer/module.go index 3ab73573704..4b162310d35 100644 --- a/x/ibc-transfer/module.go +++ b/x/ibc-transfer/module.go @@ -105,7 +105,7 @@ func (AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route implements the AppModule interface -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/ibc/module.go b/x/ibc/module.go index 507ed8658d3..6d44cef373d 100644 --- a/x/ibc/module.go +++ b/x/ibc/module.go @@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route returns the message routing key for the ibc module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(*am.keeper)) } diff --git a/x/mint/module.go b/x/mint/module.go index d02cc6fc6c9..d17378693fd 100644 --- a/x/mint/module.go +++ b/x/mint/module.go @@ -100,7 +100,7 @@ func (AppModule) Name() string { func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route returns the message routing key for the mint module. -func (AppModule) Route() *sdk.Route { return nil } +func (AppModule) Route() sdk.Route { return sdk.Route{} } // QuerierRoute returns the mint module's querier route name. func (AppModule) QuerierRoute() string { diff --git a/x/params/module.go b/x/params/module.go index e37b905ac39..267ad2b7942 100644 --- a/x/params/module.go +++ b/x/params/module.go @@ -84,7 +84,7 @@ func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONMarshaler, _ json.Raw return []abci.ValidatorUpdate{} } -func (AppModule) Route() *sdk.Route { return nil } +func (AppModule) Route() sdk.Route { return sdk.Route{} } // GenerateGenesisState performs a no-op. func (AppModule) GenerateGenesisState(simState *module.SimulationState) {} diff --git a/x/slashing/module.go b/x/slashing/module.go index 7dd55f981c9..c588a971ca6 100644 --- a/x/slashing/module.go +++ b/x/slashing/module.go @@ -110,7 +110,7 @@ func (AppModule) Name() string { func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route returns the message routing key for the slashing module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/staking/module.go b/x/staking/module.go index 777c4dab9db..1b999444d07 100644 --- a/x/staking/module.go +++ b/x/staking/module.go @@ -134,7 +134,7 @@ func (am AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { } // Route returns the message routing key for the staking module. -func (am AppModule) Route() *sdk.Route { +func (am AppModule) Route() sdk.Route { return sdk.NewRoute(RouterKey, NewHandler(am.keeper)) } diff --git a/x/upgrade/module.go b/x/upgrade/module.go index 93e034b35ff..a6611b6d65a 100644 --- a/x/upgrade/module.go +++ b/x/upgrade/module.go @@ -99,7 +99,7 @@ func NewAppModule(keeper keeper.Keeper) AppModule { func (AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {} // Route is empty, as we do not handle Messages (just proposals) -func (AppModule) Route() *sdk.Route { return nil } +func (AppModule) Route() sdk.Route { return sdk.Route{} } // QuerierRoute returns the route we respond to for abci queries func (AppModule) QuerierRoute() string { return types.QuerierKey }