Fix error handling in renderStatic #64

electricdusk · 2020-05-30T17:55:28+02:00

electricdusk commented

2020-05-30 17:55:28 +02:00

In renderStatic, modTime should be set to the file's mod time if it exists. If the file does not exist, renderStatic should

keep modTime uninitialized,
return early with a 404 Not Found error.

Fixes #60.

In `renderStatic`, `modTime` should be set to the file's mod time if it exists. If the file does not exist, `renderStatic` should - keep `modTime` uninitialized, - return early with a `404 Not Found` error. Fixes #60.

mrngm was assigned by electricdusk

2020-05-30 17:55:28 +02:00

mrngm requested changes 2020-06-28 13:38:01 +02:00

handlers_test.go Outdated

						
				@ -58,3 +58,3 @@

						rootURL: rootURL,

					}

					return CreateMainRouter(&rl), &rl

					return CreateMainRouter(&rl, true), &rl

mrngm commented

2020-06-28 13:37:13 +02:00

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

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

router.go

						
				@ -40,6 +41,7 @@ func (rl *rushlink) recoveryMiddleware(next http.Handler) http.Handler {

							}()

							if err := recover(); err != nil {

								w.WriteHeader(500)

mrngm commented

2020-06-28 13:36:42 +02:00

Can we definitively state that panic()s are caused due to errors on the server side?

Can we definitively state that `panic()`s are caused due to errors on the server side?

electricdusk commented

2020-06-28 14:39:45 +02:00

Yes. panics during runtime should always indicate a bug.

Yes. `panic`s during runtime should always indicate a bug.

router.go Outdated

						
				@ -81,3 +83,3 @@

				// CreateMainRouter creates the main Gorilla router for the application.

				func CreateMainRouter(rl *rushlink) *mux.Router {

				func CreateMainRouter(rl *rushlink, debug bool) *mux.Router {

mrngm commented

2020-06-28 13:35:20 +02:00

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

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?

router.go Outdated

						
				@ -120,3 +124,3 @@

					srv := &http.Server{

						Handler:      CreateMainRouter(&rl),

						Handler:      CreateMainRouter(&rl, false),