Fix Static files route not working #1723
Conversation
Fix Static files route not working
add tests
Codecov Report
@@ Coverage Diff @@
## master #1723 +/- ##
=======================================
Coverage 85.77% 85.77%
=======================================
Files 29 29
Lines 2017 2018 +1
=======================================
+ Hits 1730 1731 +1
Misses 181 181
Partials 106 106
Continue to review full report at Codecov.
|
| @@ -161,6 +170,40 @@ func TestEchoStatic(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestEchoStaticRedirectIndex(t *testing.T) { | |||
There was a problem hiding this comment.
You should not run tests like that because:
- it adds constant 200ms to execution time - using sleeps makes tests slow in long run and brittle some cases
- it assumes that port 1323 is free - use
127.0.0.1:0:0etc instead.:0would bind to random port and you can get the address where server was binded withaddr := e.Listener.Addr(). See documentation https://golang.org/pkg/net/#ListenIf the port in the address parameter is empty or "0", as in "127.0.0.1:" or "[::1]:0", a port number is automatically chosen. The Addr method of Listener can be used to discover the chosen port.
TestEchoStatic that you modified above is pretty much same as this is. When you spin up new http server. Each request will invoke ServeHTTP(rec, req) method same as TestEchoStatic does ... but without starting a server.
| @@ -500,6 +500,7 @@ func (common) static(prefix, root string, get func(string, HandlerFunc, ...Middl | |||
| } | |||
| return c.File(name) | |||
| } | |||
| get(prefix, h) | |||
There was a problem hiding this comment.
Then dealing with this method we need to consider where is is called. Is it e.Static() or group.Static()
e.Static()is on server root level i.e.http://server/assets/cat.gifURLsgroup.Staticis on group level which means in addition to this method prefix we have group path as prefix in url. i.e.http://server/group/assets/cat.gif
and regarding to prefix we should consider it ends with slash or not
i.e. prefixes:
- ``
//assets/assets/
now lets add two possibilities here: if group path is /user or /user/ what routes we will end up with?
There was a problem hiding this comment.
something like that
// prefix does not end with slash
if prefix != "" && prefix[len(prefix) - 1] != '/' {
get(prefix, h) // i.e. prefix=/assets and URL `/assets` must give directory index
return get(prefix+"/*", h) // and asterisk to match anything in that directory
}
if prefix != "" {
get(prefix[:len(prefix) - 1], h) // i.e. prefix=/assets/ then add route `/assets` for directory index
}
// i.e. prefix="" (empty) and URL `http://server` or `http://server/` must give directory index
// i.e. prefix=/assets/ and URL `/assets/` must give directory index, `/assets/*` serve files
return get(prefix+"*", h)
|
@lammel consider this patch for merge. It will add exact match route for directory when prefix does not end with slash. Also documents behavior and possible problems |
fix 404 error when using Static files route Echo.Static()
resubmit of #1671
related issue