electricdusk/rushlink
3
3
Fork 3
Code Issues 21 Pull Requests 2 Releases Activity

Fix error handling in renderStatic #64

Merged
electricdusk merged 2 commits from issue-60 into master 2020-07-06 17:05:00 +02:00
Conversation 0 Commits 2 Files Changed 3 +21 -11
3 changed files with 21 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d37222f82a - Show all commits

4
handlers_test.go
View File

@ -57,7 +57,7 @@ func createTemporaryRouter(t *testing.T) (*mux.Router, *rushlink) {
fs: fileStore,
rootURL: rootURL,
}
return CreateMainRouter(&rl, true), &rl
return CreateMainRouter(&rl), &rl
mrngm commented 2020-06-28 13:37:13 +02:00
Outdated
Review
Copy Link

(see comment below on the use of debug as a function parameter)

(see comment below on the use of `debug` as a function parameter)
}
// checkStatusCode checks whether the status code from a recorded response is equal
@ -150,7 +150,7 @@ func TestIssue53(t *testing.T) {
func TestIssue60(t *testing.T) {
srv, _ := createTemporaryRouter(t)
// Put a URL with a fragment identifier into the database.
// Request a nonexistent static file
req, err := http.NewRequest("GET", "/css/nonexistent_file.css", nil)
if err != nil {
t.Fatal(err)

18
router.go
View File

@ -82,12 +82,13 @@ func (w *statusResponseWriter) WriteHeader(statusCode int) {
}
// CreateMainRouter creates the main Gorilla router for the application.
func CreateMainRouter(rl *rushlink, debug bool) *mux.Router {
//
mrngm commented 2020-06-28 13:35:20 +02:00
Outdated
Review
Copy Link

Instead of adding a debug parameter here, would it make sense to set router.Use(rl.recoveryMiddleware) inside TestIssue60?

func TestIssue60(t *testing.T) {
	srv, rl := createTemporaryRouter(t)
	srv.Use(rl.recoveryMiddleware)
	srv.Use(rl.metricsMiddleware)

	// Request a nonexistent static file
	req, err := http.NewRequest("GET", "/css/nonexistent_file.css", nil)
	if err != nil {
		t.Fatal(err)
	}
	rr := httptest.NewRecorder()
	srv.ServeHTTP(rr, req)
	checkStatusCode(t, rr, http.StatusNotFound)

And, if we want debugging options, this should be a globally configurable option, instead of a function parameter.

Instead of adding a debug parameter here, would it make sense to set `router.Use(rl.recoveryMiddleware)` inside `TestIssue60`? ```go func TestIssue60(t *testing.T) { srv, rl := createTemporaryRouter(t) srv.Use(rl.recoveryMiddleware) srv.Use(rl.metricsMiddleware) // Request a nonexistent static file req, err := http.NewRequest("GET", "/css/nonexistent_file.css", nil) if err != nil { t.Fatal(err) } rr := httptest.NewRecorder() srv.ServeHTTP(rr, req) checkStatusCode(t, rr, http.StatusNotFound) ``` And, if we want debugging options, this should be a globally configurable option, instead of a function parameter.
electricdusk commented 2020-06-28 14:42:09 +02:00
Outdated
Review
Copy Link

So you mean, as a global variable? So in this case, debug describes that the panic recovery middleware should be disabled, because otherwise we cannot run the tests. We could also change the name debug to noPanicMiddleware.

What kind of solution would be good here?

So you mean, as a global variable? So in this case, `debug` describes that the panic recovery middleware should be disabled, because otherwise we cannot run the tests. We could also change the name `debug` to `noPanicMiddleware`. What kind of solution would be good here?
// This function will not populate the router with an error-recovery and
// metrics-reporting middleware. If these middleware are required, then the
// caller should encapsulate this router inside of another router and register
// the middlewares on the encapsulating router.
func CreateMainRouter(rl *rushlink) *mux.Router {
router := mux.NewRouter()
if !debug {
router.Use(rl.recoveryMiddleware)
router.Use(rl.metricsMiddleware)
}
router.HandleFunc("/{path:img/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
router.HandleFunc("/{path:css/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
router.HandleFunc("/{path:js/"+staticFilenameExpr+"}", rl.staticGetHandler).Methods("GET", "HEAD")
@ -122,8 +123,13 @@ func StartMainServer(addr string, db *db.Database, fs *db.FileStore, rawRootURL
rootURL: rootURL,
}
router := mux.NewRouter()
mrngm commented 2020-06-28 13:37:40 +02:00
Outdated
Review
Copy Link

(see comment above on the use of debug as a function parameter)

(see comment above on the use of `debug` as a function parameter)
router.Use(rl.metricsMiddleware)
router.Use(rl.recoveryMiddleware)
router.Handle("/", CreateMainRouter(&rl))
srv := &http.Server{
Handler: CreateMainRouter(&rl, false),
Handler: router,
Addr: addr,
WriteTimeout: 15 * time.Second,
ReadTimeout: 15 * time.Second,

10
views.go
View File

@ -88,7 +88,7 @@ func mapExtend(m map[string]interface{}, key string, value interface{}) {
func (rl *rushlink) renderStatic(w http.ResponseWriter, r *http.Request, path string) {
var modTime time.Time
if info, err := AssetInfo(path); err != nil {
if info, err := AssetInfo(path); err == nil {
modTime = info.ModTime()
}
contents, err := Asset(path)
@ -118,7 +118,12 @@ func (rl *rushlink) render(w http.ResponseWriter, r *http.Request, status int, t
err = fmt.Errorf("'%v' not in textTemplates", tmplName)
break
}
err = tmpl.Execute(w, data)
if status != 0 {
w.WriteHeader(status)
}
if r.Method != "HEAD" {
err = tmpl.Execute(w, data)
}
case "text/html":
w.Header().Set("Content-Type", "text/html")
tmpl := htmlTemplates[tmplName]
@ -142,7 +147,6 @@ func (rl *rushlink) render(w http.ResponseWriter, r *http.Request, status int, t
if status != 0 {
w.WriteHeader(status)
}
if r.Method != "HEAD" {
err = tmpl.Execute(w, data)
}