Added option to disable logging when serving static assets

[JamesClonk]

I added an optional parameter to Static(), to disable logging when serving static assets.

The reason for this is that my current project has a lot of static CSS, JS, etc. files that are served with each GET request. This produces a lot of "spam" that I don't want / need to have in my logfile/stdout.

The new variadic bool parameter for Static() is called skipLogging.
If it is set to true, no logging output will be produced. Since the default for a missing parameter / bool is false, this change is fully backwards compatible.

Example:

//m := martini.Classic()
r := martini.NewRouter()
m := martini.New()
m.Use(martini.Recovery())
m.Use(martini.Static("public", true)) // skip logging on static content
m.Use(martini.Logger()) // place Logger after Static
m.Action(r.Handle)

r.Get("/", index)
m.Run()

// Output:
// [martini] listening on port 3000
// [martini] Started GET /
// [martini] Completed 200 OK in 615.252us

vs. before:

m := martini.Classic()

m.Get("/", index)
m.Run()

// Output:
// [martini] listening on port 3000
// [martini] Started GET /
// [martini] Completed 200 OK in 604.306us
// [martini] Started GET /stylesheets/base.css
// [martini] [Static] Serving /stylesheets/base.css
// [martini] Completed 304 Not Modified in 52.72us
// [martini] Started GET /stylesheets/custom.css
// [martini] [Static] Serving /stylesheets/custom.css
// [martini] Completed 304 Not Modified in 36.677us
// [martini] Started GET /stylesheets/layout.css
// [martini] [Static] Serving /stylesheets/layout.css
// [martini] Completed 304 Not Modified in 32.378us
// [martini] Started GET /stylesheets/skeleton.css
// [martini] [Static] Serving /stylesheets/skeleton.css
// [martini] Completed 304 Not Modified in 33.953us
// [martini] Started GET /images/bg.png
// [martini] [Static] Serving /images/bg.png
// [martini] Completed 304 Not Modified in 42.832us
// [martini] Started GET /images/color.png
// [martini] [Static] Serving /images/color.png
// [martini] Completed 304 Not Modified in 37.132us

[JamesClonk]

I did some more thinking and realized it's probably better (and more Go-idiomatic?) to use a variadic struct as optional parameter for Static(), similar to how it's handled in render.go.
So I changed it to add a new StaticOptions struct.
(See latest commit 023e107)

Example usage:

m := martini.New()

m.Use(martini.Static("public", martini.StaticOptions{
    SkipLogging: true, // skip logging on static content
    // SkipServeIndex: true // possible to disable serving index file this way
    IndexFile: "custom.html", // can also set custom index file to serve
}))

[codegangsta]

cool. I will take a look at this very soon

[achun]

Here is a simpler way to do

Export defaultReturnHandler

func DefaultReturnHandler() ReturnHandler {
// ....
}

Add NewSimple

type devNull struct{}
func (devNull) Write(p []byte) (int, error) {
    return len(p), nil
}
func NewSimple() *Martini {
    null, _ := os.Open(os.DevNull) // or null := new(devNull)
    m := &Martini{inject.New(), []Handler{}, func() {}, log.New(null, "", 0)}
    m.Map(m.logger)
    return m
}

Example usage:

m := martini.NewSimple()
m.Use(logger) // handler
m.Map(myLogger)
m.Map(martini.DefaultReturnHandler())

[DisposaBoy]

@achun in-case you weren't aware, you can probably replace os.Devnull and type devNull with ioutil.Discard http://golang.org/pkg/io/ioutil/#Discard

[JamesClonk]

From my understanding this seems as it unfortunately would not do what I needed.
Looks like this would disable all Martini and middleware logging because *log.Logger prints everything to /dev/null, right?

I want to stop just the Static handler from logging. ("[Static] Serving ...")
All other middleware should not be affected.

It seemed to me to be the simplest way to add an option to the Static handler itself.

But anyway, by now in the last change I also added further options as a StaticOptions struct, so it's becomes possible to define the index file for example.

I was thinking that maybe if breaking backwards compatibility is still ok (?), to also move the 'directory' string into StaticOptions?
While this would ofc break anyones code who used the Static handler before directly, but Martini.Classic() could just be adapted as below and no one else would notice the change:

func Classic() *ClassicMartini {
    r := NewRouter()
    m := New()
    m.Use(Logger())
    m.Use(Recovery())
    m.Use(Static(StaticOptions{
        Directory: "public",
        // SkipLogging: false,
        // SkipServeIndex: false,
        // IndexFile: "index.html",
    }))
    m.Action(r.Handle)
    return &ClassicMartini{m, r}
}

[achun]

@JamesClonk StaticOptions and injector in different code style.

BTW:

m := &martini.ClassicMartini{martini.New(),martini.NewRouter()}
m.Action(m.Handle)
// m.Map(something)
m.Use(MyLogger())
m.Use(MyRecovery())
m.Use(MyStatic("public"))
http.ListenAndServe(":80", m)

[codegangsta]

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

[codegangsta]

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

[JamesClonk]

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

[codegangsta]

This is a welcome change. I think i would prefer to not break changes if it really isn't needed.

[codegangsta]

fix those nits and I will land it

[JamesClonk]

I had to add another check to static.go:

// Do nothing if we are not serving any index file and the request is at the root URL path.
if opt.SkipServeIndex && req.URL.Path == "/" {
    return
}

Otherwise when turning off serving an index file would be bad for Martini. Requesting http://localhost:3000/ would not work anymore, since "/" is a directory.
I also had to add another test case and even a folder "testdata" for this. :|

[JamesClonk]

After some more thinking I removed the option to skip serving index files from StaticOptions. There's just not really any useful situation for this, and it greatly simplifies my changes to static.go and static_test.go

[codegangsta]

Awesome. I'm down with that

Sent from my iPhone

On Jan 9, 2014, at 1:11 AM, JamesClonk [email protected] wrote:

After some more thinking I removed the option to skip serving index files from StaticOptions. There's just not really any useful situation for this, and it greatly simplifies my changes to static.go and static_test.go

—
Reply to this email directly or view it on GitHub.

[codegangsta]

This looks great. Merging!