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 <alessio@tendermint.com>
cosmos · Jun 11, 2020 · 49597b1 · 49597b1
1 parent 0215b5c
commit 49597b1
Show file tree

Hide file tree

Showing 23 changed files with 101 additions and 39 deletions.
diff --git 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
@@ -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
diff --git a/tests/mocks/types_module_module.go b/tests/mocks/types_module_module.go
diff --git a/tests/mocks/types_router.go b/tests/mocks/types_router.go
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 }