Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added option to disable logging when serving static assets #147

Merged
merged 5 commits into from
Jan 10, 2014
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import (
// Logger returns a middleware handler that logs the request as it goes in and the response as it goes out.
func Logger() Handler {
return func(res http.ResponseWriter, req *http.Request, c Context, log *log.Logger) {
start := time.Now()
log.Printf("Started %s %s", req.Method, req.URL.Path)
if !c.Written() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this call here? With how Martini works, this handler shouldn't be called if the context is written right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran your newly added test without this check and it still passes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah. I noticed that now too. Doh!
Removed it. ;)

start := time.Now()
log.Printf("Started %s %s", req.Method, req.URL.Path)

rw := res.(ResponseWriter)
c.Next()
rw := res.(ResponseWriter)
c.Next()

log.Printf("Completed %v %s in %v\n", rw.Status(), http.StatusText(rw.Status()), time.Since(start))
log.Printf("Completed %v %s in %v\n", rw.Status(), http.StatusText(rw.Status()), time.Since(start))
}
}
}
22 changes: 22 additions & 0 deletions logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,25 @@ func Test_Logger(t *testing.T) {
expect(t, recorder.Code, http.StatusNotFound)
refute(t, len(buff.String()), 0)
}

func Test_LoggerWritten(t *testing.T) {
buff := bytes.NewBufferString("")
recorder := httptest.NewRecorder()

m := New()
// replace log for testing
m.Map(log.New(buff, "[martini] ", 0))
m.Use(func(res http.ResponseWriter) {
res.WriteHeader(http.StatusNotFound)
}) // c.Written() should be true now
m.Use(Logger())

req, err := http.NewRequest("GET", "http://localhost:3000/foobar", nil)
if err != nil {
t.Error(err)
}

m.ServeHTTP(recorder, req)
expect(t, recorder.Code, http.StatusNotFound)
expect(t, len(buff.String()), 0)
}
63 changes: 45 additions & 18 deletions static.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,32 @@ import (
"strings"
)

type StaticOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need godoc documentation

SkipLogging bool
SkipServeIndex bool
IndexFile string
}

func prepareStaticOptions(options []StaticOptions) StaticOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this consistent to how we prepare options in the render middleware https://github.com/codegangsta/martini-contrib/blob/master/render/render.go#L126-L141

if len(options) > 0 {
// Default index file to serve should be index.html
if options[0].IndexFile == "" {
options[0].IndexFile = "index.html"
}
return options[0]
}
return StaticOptions{
SkipLogging: false, // Logging is on by default
SkipServeIndex: false, // Try to serve index.html by default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having this option would it be okay to have the zero value of IndexFile act as a way to skip index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to, but the problem is that since in prepareStaticOptions() I already check for the zero value (and set it to "index.html" as default value if it is) it can never be the zero value. It's either something or index.html.
Any other way to make this work? I can't figure it out, so I left the bool flag for now..

IndexFile: "index.html", // Default index file to serve
}
}

// Static returns a middleware handler that serves static files in the given directory.
func Static(directory string) Handler {
func Static(directory string, staticOpt ...StaticOptions) Handler {
dir := http.Dir(directory)
opt := prepareStaticOptions(staticOpt)

return func(res http.ResponseWriter, req *http.Request, log *log.Logger) {
file := req.URL.Path
f, err := dir.Open(file)
Expand All @@ -24,29 +47,33 @@ func Static(directory string) Handler {
return
}

// Try to serve index.html
if fi.IsDir() {
if !opt.SkipServeIndex {
// Try to serve index.html
if fi.IsDir() {

// redirect if missing trailing slash
if !strings.HasSuffix(file, "/") {
http.Redirect(res, req, file+"/", http.StatusFound)
return
}
// redirect if missing trailing slash
if !strings.HasSuffix(file, "/") {
http.Redirect(res, req, file+"/", http.StatusFound)
return
}

file = path.Join(file, "index.html")
f, err = dir.Open(file)
if err != nil {
return
}
defer f.Close()
file = path.Join(file, opt.IndexFile)
f, err = dir.Open(file)
if err != nil {
return
}
defer f.Close()

fi, err = f.Stat()
if err != nil || fi.IsDir() {
return
fi, err = f.Stat()
if err != nil || fi.IsDir() {
return
}
}
}

log.Println("[Static] Serving " + file)
if !opt.SkipLogging {
log.Println("[Static] Serving " + file)
}
http.ServeContent(res, req, file, fi.ModTime(), f)
}
}
69 changes: 69 additions & 0 deletions static_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package martini

import (
"bytes"
"github.com/codegangsta/inject"
"log"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -53,3 +56,69 @@ func Test_Static_BadDir(t *testing.T) {
m.ServeHTTP(response, req)
refute(t, response.Code, http.StatusOK)
}

func Test_Static_Options_Logging(t *testing.T) {
response := httptest.NewRecorder()

var buffer bytes.Buffer
m := &Martini{inject.New(), []Handler{}, func() {}, log.New(&buffer, "[martini] ", 0)}
m.Map(m.logger)
m.Map(defaultReturnHandler())

opt := StaticOptions{}
m.Use(Static(".", opt))

req, err := http.NewRequest("GET", "http://localhost:3000/martini.go", nil)
if err != nil {
t.Error(err)
}

m.ServeHTTP(response, req)
expect(t, response.Code, http.StatusOK)
expect(t, buffer.String(), "[martini] [Static] Serving /martini.go\n")

// Now without logging
m.Handlers()
buffer.Reset()

// This should disable logging
opt.SkipLogging = true
m.Use(Static(".", opt))

m.ServeHTTP(response, req)
expect(t, response.Code, http.StatusOK)
expect(t, buffer.String(), "")
}

func Test_Static_Options_ServeIndex(t *testing.T) {
response := httptest.NewRecorder()

var buffer bytes.Buffer
m := &Martini{inject.New(), []Handler{}, func() {}, log.New(&buffer, "[martini] ", 0)}
m.Map(m.logger)
m.Map(defaultReturnHandler())

opt := StaticOptions{IndexFile: "martini.go"} // Define martini.go as index file
m.Use(Static(".", opt))

req, err := http.NewRequest("GET", "http://localhost:3000/", nil)
if err != nil {
t.Error(err)
}

m.ServeHTTP(response, req)
expect(t, response.Code, http.StatusOK)
expect(t, buffer.String(), "[martini] [Static] Serving /martini.go\n")

// Now without index serving
m.Handlers()
buffer.Reset()

// This should make Static() stop serving index.html (or martini.go in this case)
opt.SkipServeIndex = true
m.Use(Static(".", opt))

m.ServeHTTP(response, req)
expect(t, response.Code, http.StatusOK)
expect(t, buffer.String(), "[martini] [Static] Serving /\n")
}