Skip to content

Commit 150849c

Browse files
notzippynotzippy
committed
Fixes wildcards in router files in app from affecting modules or visa verisy
1 parent c793ee6 commit 150849c

File tree

5 files changed

+126
-34
lines changed

5 files changed

+126
-34
lines changed

‎controller.go

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package revel
77
import (
88
"errors"
99
"fmt"
10+
"go/build"
1011
"io"
1112
"net/http"
1213
"os"
@@ -131,7 +132,7 @@ func (c *Controller) RenderTemplate(templatePath string) Result {
131132
}
132133

133134
return &RenderTemplateResult{
134-
Template: template,
135+
Template: template,
135136
ViewArgs: c.ViewArgs,
136137
}
137138
}
@@ -364,6 +365,7 @@ func findControllers(appControllerType reflect.Type) (indexes [][]int) {
364365
// Controller registry and types.
365366

366367
type ControllerType struct {
368+
ModuleSource *Module // The module for the controller
367369
Type reflect.Type
368370
Methods []*MethodType
369371
ControllerIndexes [][]int // FieldByIndex to all embedded *Controllers
@@ -398,8 +400,7 @@ var controllers = make(map[string]*ControllerType)
398400
func RegisterController(c interface{}, methods []*MethodType) {
399401
// De-star the controller type
400402
// (e.g. given TypeOf((*Application)(nil)), want TypeOf(Application))
401-
t := reflect.TypeOf(c)
402-
elem := t.Elem()
403+
elem := reflect.TypeOf(c).Elem()
403404

404405
// De-star all of the method arg types too.
405406
for _, m := range methods {
@@ -408,8 +409,27 @@ func RegisterController(c interface{}, methods []*MethodType) {
408409
arg.Type = arg.Type.Elem()
409410
}
410411
}
412+
path := elem.PkgPath()
413+
gopathList := filepath.SplitList(build.Default.GOPATH)
414+
var controllerModule *Module
415+
416+
// See if the path exists in the module based
417+
for i := range Modules {
418+
found := false
419+
for _, gopath := range gopathList {
420+
if strings.HasPrefix(gopath+"/src/"+path, Modules[i].Path) {
421+
controllerModule = Modules[i]
422+
found = true
423+
break
424+
}
425+
}
426+
if found {
427+
break
428+
}
429+
}
411430

412431
controllers[strings.ToLower(elem.Name())] = &ControllerType{
432+
ModuleSource: controllerModule,
413433
Type: elem,
414434
Methods: methods,
415435
ControllerIndexes: findControllers(elem),

‎revel.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ var (
7070
// 3. user supplied configs (...) - User configs can override/add any from above
7171
ConfPaths []string
7272

73-
Modules []Module
73+
Modules []*Module
7474

7575
// Server config.
7676
//
@@ -399,7 +399,7 @@ func ResolveImportPath(importPath string) (string, error) {
399399
}
400400

401401
func addModule(name, importPath, modulePath string) {
402-
Modules = append(Modules, Module{Name: name, ImportPath: importPath, Path: modulePath})
402+
Modules = append(Modules, &Module{Name: name, ImportPath: importPath, Path: modulePath})
403403
if codePath := filepath.Join(modulePath, "app"); DirExists(codePath) {
404404
CodePaths = append(CodePaths, codePath)
405405
if viewsPath := filepath.Join(modulePath, "app", "views"); DirExists(viewsPath) {
@@ -422,13 +422,13 @@ func addModule(name, importPath, modulePath string) {
422422
}
423423

424424
// ModuleByName returns the module of the given name, if loaded.
425-
func ModuleByName(name string) (m Module, found bool) {
425+
func ModuleByName(name string) (m *Module, found bool) {
426426
for _, module := range Modules {
427427
if module.Name == name {
428428
return module, true
429429
}
430430
}
431-
return Module{}, false
431+
return nil, false
432432
}
433433

434434
// CheckInit method checks `revel.Initialized` if not initialized it panics

‎router.go

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,15 @@ import (
1515
"regexp"
1616
"strings"
1717

18-
"github.com/robfig/pathtree"
18+
"github.com/revel/pathtree"
1919
)
2020

2121
const (
2222
httpStatusCode = "404"
2323
)
2424

2525
type Route struct {
26+
ModuleSource *Module // Module name of route
2627
Method string // e.g. GET
2728
Path string // e.g. /app/:id
2829
Action string // e.g. "Application.ShowApp", "404"
@@ -89,12 +90,13 @@ func treePath(method, path string) string {
8990
type Router struct {
9091
Routes []*Route
9192
Tree *pathtree.Node
93+
Module string // The module the route is associated with
9294
path string // path to the routes file
9395
}
9496

9597
var notFound = &RouteMatch{Action: "404"}
9698

97-
func (router *Router) Route(req *http.Request) *RouteMatch {
99+
func (router *Router) Route(req *http.Request) (routeMatch *RouteMatch) {
98100
// Override method if set in header
99101
if method := req.Header.Get("X-HTTP-Method-Override"); method != "" && req.Method == "POST" {
100102
req.Method = method
@@ -104,7 +106,8 @@ func (router *Router) Route(req *http.Request) *RouteMatch {
104106
if leaf == nil {
105107
return nil
106108
}
107-
route := leaf.Value.(*Route)
109+
routeList := leaf.Value.([]*Route)
110+
INFO.Printf("Found route %s %#v",req.URL.Path,routeList)
108111

109112
// Create a map of the route parameters.
110113
var params url.Values
@@ -114,27 +117,51 @@ func (router *Router) Route(req *http.Request) *RouteMatch {
114117
params[leaf.Wildcards[i]] = []string{v}
115118
}
116119
}
120+
var route *Route
121+
var controllerName string
122+
for index := range routeList {
123+
route = routeList[index]
124+
// Special handling for explicit 404's.
125+
if route.Action == httpStatusCode {
126+
route = nil
127+
break
128+
}
117129

118-
// Special handling for explicit 404's.
119-
if route.Action == httpStatusCode {
120-
return notFound
121-
}
130+
// If the action is variablized, replace into it with the captured args.
131+
controllerName = route.ControllerName
132+
if controllerName[0] == ':' {
133+
controllerName = params[controllerName[1:]][0]
122134

123-
// If the action is variablized, replace into it with the captured args.
124-
controllerName, methodName := route.ControllerName, route.MethodName
125-
if controllerName[0] == ':' {
126-
controllerName = params[controllerName[1:]][0]
127-
}
128-
if methodName[0] == ':' {
129-
methodName = params[methodName[1:]][0]
135+
// Check that the controller comes from a matching route
136+
if controller, found := controllers[strings.ToLower(controllerName)]; found {
137+
138+
if route.ModuleSource == controller.ModuleSource {
139+
break
140+
}
141+
}
142+
} else {
143+
break
144+
}
145+
route = nil
130146
}
131147

132-
return &RouteMatch{
133-
ControllerName: controllerName,
134-
MethodName: methodName,
135-
Params: params,
136-
FixedParams: route.FixedParams,
148+
if route ==nil {
149+
routeMatch = notFound
150+
} else {
151+
methodName := route.MethodName
152+
if methodName[0] == ':' {
153+
methodName = params[methodName[1:]][0]
154+
}
155+
156+
routeMatch = &RouteMatch{
157+
ControllerName: controllerName,
158+
MethodName: methodName,
159+
Params: params,
160+
FixedParams: route.FixedParams,
161+
}
137162
}
163+
164+
return
138165
}
139166

140167
// Refresh re-reads the routes file and re-calculates the routing table.
@@ -150,17 +177,32 @@ func (router *Router) Refresh() (err *Error) {
150177

151178
func (router *Router) updateTree() *Error {
152179
router.Tree = pathtree.New()
180+
pathMap := map[string][]*Route{}
181+
182+
allPathsOrdered := []string{}
183+
// It is possible for some route paths to overlap
184+
// based on wildcard matches,
185+
// TODO when pathtree is fixed (made to be smart enough to not require a predefined intake order) keeping the routes in order is not necessary
153186
for _, route := range router.Routes {
154-
err := router.Tree.Add(route.TreePath, route)
187+
if _,found := pathMap[route.TreePath];!found {
188+
pathMap[route.TreePath] = append(pathMap[route.TreePath], route)
189+
allPathsOrdered = append(allPathsOrdered, route.TreePath)
190+
} else {
191+
pathMap[route.TreePath] = append(pathMap[route.TreePath], route)
192+
}
193+
}
194+
for _,path := range allPathsOrdered {
195+
routeList := pathMap[path]
196+
err := router.Tree.Add(path, routeList)
155197

156198
// Allow GETs to respond to HEAD requests.
157-
if err == nil && route.Method == "GET" {
158-
err = router.Tree.Add(treePath("HEAD", route.Path), route)
199+
if err == nil && routeList[0].Method == "GET" {
200+
err = router.Tree.Add(treePath("HEAD", routeList[0].Path), routeList)
159201
}
160202

161203
// Error adding a route to the pathtree.
162204
if err != nil {
163-
return routeError(err, route.routesPath, "", route.line)
205+
return routeError(err, path, fmt.Sprintf("%#v",routeList), routeList[0].line)
164206
}
165207
}
166208
return nil
@@ -291,15 +333,22 @@ func routeError(err error, routesPath, content string, n int) *Error {
291333

292334
// getModuleRoutes loads the routes file for the given module and returns the
293335
// list of routes.
294-
func getModuleRoutes(moduleName, joinedPath string, validate bool) ([]*Route, *Error) {
336+
func getModuleRoutes(moduleName, joinedPath string, validate bool) (routes []*Route, err *Error) {
295337
// Look up the module. It may be not found due to the common case of e.g. the
296338
// testrunner module being active only in dev mode.
297339
module, found := ModuleByName(moduleName)
298340
if !found {
299341
INFO.Println("Skipping routes for inactive module", moduleName)
300342
return nil, nil
301343
}
302-
return parseRoutesFile(filepath.Join(module.Path, "conf", "routes"), joinedPath, validate)
344+
routes, err = parseRoutesFile(filepath.Join(module.Path, "conf", "routes"), joinedPath, validate)
345+
if err == nil {
346+
for _, route := range routes {
347+
route.ModuleSource = module
348+
}
349+
}
350+
351+
return routes,err
303352
}
304353

305354
// Groups:
@@ -368,6 +417,20 @@ func (router *Router) Reverse(action string, argValues map[string]string) *Actio
368417
argValues[route.MethodName[1:]] = methodName
369418
}
370419

420+
// Constraint match - wildcard matches can only be made by the same module that created them
421+
if controllerWildcard || methodWildcard {
422+
if controller, found := controllers[strings.ToLower(controllerName)]; found {
423+
// Wildcard match boundary
424+
if controller.ModuleSource != route.ModuleSource {
425+
continue
426+
}
427+
// See if the path exists in the module based
428+
} else {
429+
ERROR.Printf("Controller %s not found in reverse lookup", controllerName)
430+
continue
431+
}
432+
}
433+
371434
// Build up the URL.
372435
var (
373436
queryValues = make(url.Values)

‎router_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,8 @@ var routeMatchTestCases = map[*http.Request]*RouteMatch{
256256
Params: map[string][]string{"id": {"123"}},
257257
},
258258
}
259-
260259
func TestRouteMatches(t *testing.T) {
260+
initControllers()
261261
BasePath = "/BasePath"
262262
router := NewRouter("")
263263
router.Routes, _ = parseRoutes("", "", TestRoutes, false)
@@ -342,7 +342,16 @@ var reverseRoutingTestCases = map[*ReverseRouteArgs]*ActionDefinition{
342342
},
343343
}
344344

345+
type testController struct {
346+
*Controller
347+
}
348+
func initControllers() {
349+
controllers["implicit"] = &ControllerType{}
350+
controllers["static"] = &ControllerType{}
351+
controllers["application"] = &ControllerType{}
352+
}
345353
func TestReverseRouting(t *testing.T) {
354+
initControllers()
346355
router := NewRouter("")
347356
router.Routes, _ = parseRoutes("", "", TestRoutes, false)
348357
for routeArgs, expected := range reverseRoutingTestCases {

‎templates/errors/404-dev.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ <h1>
5757
<h2>These routes have been tried, in this order :</h2>
5858
<ol>
5959
{{range .Router.Routes}}
60-
<li>{{pad .Method 10}}{{pad .Path 50}}{{.Action}}</li>
60+
<li>{{pad .Method 10}}{{pad .Path 50}}{{.Action}} {{with .ModuleSource}}(Module:{{.Name}}){{end}}</li>
6161
{{end}}
6262
</ol>
6363
</div>

0 commit comments

Comments
 (0)