code review 4629047: http: add FileSystem interface, make FileServer use it

http: add FileSystem interface, make FileServer use it

Permits serving from virtual filesystems, such as files linked
into a binary, or from a zip file.

Also adds a gofix for:

http.FileServer(root, prefix) -> http.StripPrefix(prefix, http.FileServer(http.Dir(root)))

[bradfitz, 2011-06-17 19:30:09]

Hello r@golang.org, rsc@golang.org, gri@golang.org (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-06-17 23:28:01]

LGTM but wait for others

does this help godoc?

[adg, 2011-06-18 01:28:18]

Maybe I missed an earlier discussion, but this seems too general for just http.

I would totally implement this interface in my own stuff, but
http.FileSystem seems like a goofy name if I'm not doing anything
related to http.

Of course I could just declare the interface myself, but it'd be nice
if it were to track os. os.FileSystem? io.Filesystem?

Andrew

[adg, 2011-06-18 01:39:37]

Yes, I missed it.

On 18 June 2011 11:27, Andrew Gerrand <adg@golang.org> wrote:
> Maybe I missed an earlier discussion, but this seems too general for just
http.
>
> I would totally implement this interface in my own stuff, but
> http.FileSystem seems like a goofy name if I'm not doing anything
> related to http.
>
> Of course I could just declare the interface myself, but it'd be nice
> if it were to track os. os.FileSystem? io.Filesystem?
>
> Andrew
>

[dsymonds, 2011-06-18 01:40:44]

Should it be called ServeFileFromFS (s/Fs/FS/)?


Dave.

[r2, 2011-06-18 01:42:45]

ServeOverHTTPTheFileSystemNamedInTheArgumentsSpecificallyProvidedNamely

-rob

[dsymonds, 2011-06-18 01:52:32]

On Sat, Jun 18, 2011 at 11:42 AM, Rob 'Commander' Pike <r@google.com> wrote:

> ServeOverHTTPTheFileSystemNamedInTheArgumentsSpecificallyProvidedNamely

You can import that from the bike/shed package, I believe.

But really I was just catching the CamelCasing of an initialism.


Dave.

[dsymonds, 2011-06-18 05:47:29]

On Sat, Jun 18, 2011 at 3:34 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Oh, is that the convention?  I didn't look around too much for initials in
> methods to see what the style was.

Yes. I can't find it written down explicitly, but see, for instance,
URL, ServeHTTP, etc.

The only notable exception is if an initialism occurs at the start,
and you want it unexported. Then every letter would be lowercase, as
in (a hypothetical) urlParser.


Dave.

[bradfitz, 2011-06-18 05:50:22]

Oh, is that the convention?  I didn't look around too much for initials in
methods to see what the style was.

On Fri, Jun 17, 2011 at 6:40 PM, David Symonds <dsymonds@golang.org> wrote:

> Should it be called ServeFileFromFS (s/Fs/FS/)?
>
>
> Dave.
>

[gri1, 2011-06-20 20:11:05]

I think FileInfo should be abstract as well.

Also, do you need the Readdir functionality in the File?

http://codereview.appspot.com/4629047/diff/1002/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/1002/src/pkg/http/fs.go#newcode76
src/pkg/http/fs.go:76: Stat() (*os.FileInfo, os.Error)
In my original proposal I suggested FileInfo to be an abstract interface as
well. The reason being that the current os.FileInfo struct contains some fields
that may not make sense for certain file system proxies, and also that one may
not want to have to set up a complete (or even just reserve space for an emty)
FileInfo struct for Readdir, or Stat, if most of the time only a few fields are
used (say the name).

http://codereview.appspot.com/4629047/diff/1002/src/pkg/http/fs.go#newcode77
src/pkg/http/fs.go:77: Readdir(count int) ([]os.FileInfo, os.Error)
Per rsc's suggestion, the API I am using is godoc (src/cmd/godoc/filesystem.go)
is simplified. There's no File abstraction because I only need the file
contents. I think that might not work here, where you want a File (or perhaps
it's good enough to have an io.ReadCloser?)

The godoc FileSystem interface in turn has

ReadFile
ReadDir

as found in io/ioutil, because that's what godoc really needs. The result of
ReadDir is sorted by name.

[rsc, 2011-06-20 20:20:07]

On Mon, Jun 20, 2011 at 16:11,  <gri@google.com> wrote:
> I think FileInfo should be abstract as well.

I disagree.  FileInfo is already a portable struct.
Just fill it in, avoid an unnecessary layer of abstraction.

Russ

[bradfitz, 2011-06-20 20:22:33]

On Mon, Jun 20, 2011 at 1:11 PM, <gri@google.com> wrote:

> I think FileInfo should be abstract as well.
>
> Also, do you need the Readdir functionality in the File?
>

Yes, otherwise I wouldn't have included it.

Per the comment, it's the minimal interface I needed.

[gri1, 2011-06-20 20:40:43]

On Mon, Jun 20, 2011 at 1:20 PM, Russ Cox <rsc@golang.org> wrote:
> On Mon, Jun 20, 2011 at 16:11,  <gri@google.com> wrote:
>> I think FileInfo should be abstract as well.
>
> I disagree.  FileInfo is already a portable struct.
> Just fill it in, avoid an unnecessary layer of abstraction.

I wouldn't know what to fill in for the Dev, Ino, Nlink, Rdev,
Blksize, Blocks fields for some file that doesn't have any of those
things because it's say from a memory file system. Also, those fields
are rarely accessed. It seems cheaper to have a FileInfo interface
that contains a pointer which may just point to the underlying file.
Also, some functions return an os.FileInfo, others (Readdir) an
*os.FileInfo. All these things become more consistent with a FileInfo
interface.

- gri

>
> Russ
>

[rsc, 2011-06-20 20:45:39]

> I wouldn't know what to fill in for the Dev, Ino, Nlink, Rdev,
> Blksize, Blocks fields for some file that doesn't have any of those
> things because it's say from a memory file system. Also, those fields
> are rarely accessed. It seems cheaper to have a FileInfo interface
> that contains a pointer which may just point to the underlying file.

Like elsewhere in Go, the zero value is a sensible default.
It's fine to just leave those not filled in.  Defining a new
interface does not solve the problem: someone must still
define the set of common fields (now methods).
The os.FileInfo struct already does that.

Russ

[gri1, 2011-06-20 20:55:40]

On Mon, Jun 20, 2011 at 1:45 PM, Russ Cox <rsc@golang.org> wrote:
>> I wouldn't know what to fill in for the Dev, Ino, Nlink, Rdev,
>> Blksize, Blocks fields for some file that doesn't have any of those
>> things because it's say from a memory file system. Also, those fields
>> are rarely accessed. It seems cheaper to have a FileInfo interface
>> that contains a pointer which may just point to the underlying file.
>
> Like elsewhere in Go, the zero value is a sensible default.
> It's fine to just leave those not filled in.  Defining a new
> interface does not solve the problem: someone must still
> define the set of common fields (now methods).
> The os.FileInfo struct already does that.

It may use less space. Also, by having those fields present (even if
0), clients are ok to use them, and will have to know that the fields
might be 0 and thus invalid. Makes the API more complex.

All I am saying is that I would keep them abstract. Personal preference.

- gri

>
> Russ
>

[rsc, 2011-06-20 20:58:20]

> All I am saying is that I would keep them abstract. Personal preference.

All I am saying is that they are already abstract.
That's why we have os.FileInfo instead of syscall.Stat_t.

Russ

[gri1, 2011-06-20 21:10:27]

I guess we have different notions of what abstract means.
- Robert

On Mon, Jun 20, 2011 at 1:58 PM, Russ Cox <rsc@golang.org> wrote:
>> All I am saying is that I would keep them abstract. Personal preference.
>
> All I am saying is that they are already abstract.
> That's why we have os.FileInfo instead of syscall.Stat_t.
>
> Russ
>

[bradfitz, 2011-06-22 01:50:26]

Ping.  Can I check this in?  (after renaming it to capital FS)

r@ gave an LGTM.  Russ?  You're my http gatekeeper.

[rsc, 2011-06-22 02:04:31]

On Tue, Jun 21, 2011 at 21:50,  <bradfitz@golang.org> wrote:
> Ping.  Can I check this in?  (after renaming it to capital FS)

Please don't.

I think this is one function too many.  There's already
ServeFile and FileServer, which is already a little redundant.
Adding ServeFileFromFS expands the API too much.
I'd like to find a way to unify these behind an interface:
instead of enumerating multiple APIs there should be one
API that can take multiple implementations of a single
interface.

One possibility is to define:

// A Dir implements http.FileSystem using the native file
// system restricted to a specific directory tree.
type Dir string

... methods

and then we would make FileServer take a FileSystem
and rewrite http.FileServer(root, prefix) into
http.FileServer(http.Dir(root), prefix).  But you could
pass an alternate implementation to it too.

At that point I don't mind having
http.ServeFile(w, req, fs, name)

and a gofix to rewrite
http.ServeFile(w, req, name) -> http.ServeFile(w, req, http.Dir("/"), name).

Russ

[bradfitz, 2011-06-22 02:09:14]

Sounds like a plan.

On Tue, Jun 21, 2011 at 7:04 PM, Russ Cox <rsc@golang.org> wrote:

> On Tue, Jun 21, 2011 at 21:50,  <bradfitz@golang.org> wrote:
> > Ping.  Can I check this in?  (after renaming it to capital FS)
>
> Please don't.
>
> I think this is one function too many.  There's already
> ServeFile and FileServer, which is already a little redundant.
> Adding ServeFileFromFS expands the API too much.
> I'd like to find a way to unify these behind an interface:
> instead of enumerating multiple APIs there should be one
> API that can take multiple implementations of a single
> interface.
>
> One possibility is to define:
>
> // A Dir implements http.FileSystem using the native file
> // system restricted to a specific directory tree.
> type Dir string
>
> ... methods
>
> and then we would make FileServer take a FileSystem
> and rewrite http.FileServer(root, prefix) into
> http.FileServer(http.Dir(root), prefix).  But you could
> pass an alternate implementation to it too.
>
> At that point I don't mind having
> http.ServeFile(w, req, fs, name)
>
> and a gofix to rewrite
> http.ServeFile(w, req, name) -> http.ServeFile(w, req, http.Dir("/"),
> name).
>
> Russ
>

[bradfitz, 2011-06-27 18:48:46]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 19:28:37]

Hold on review.  Or consider it a checkpoint.

This needs tests and I don't like where the filepath.Clean is currently.


On Mon, Jun 27, 2011 at 11:48 AM, <bradfitz@golang.org> wrote:

> Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
> dsymonds@golang.org, r@google.com, gri@google.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**4629047/<http://codereview.appspot.com/4629047/>
>

[bradfitz, 2011-06-27 20:19:43]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 20:23:03]

PTAL.

hg mail email seems to be backed up.


On Mon, Jun 27, 2011 at 12:28 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Hold on review.  Or consider it a checkpoint.
>
> This needs tests and I don't like where the filepath.Clean is currently.
>
>
> On Mon, Jun 27, 2011 at 11:48 AM, <bradfitz@golang.org> wrote:
>
>> Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
>> dsymonds@golang.org, r@google.com, gri@google.com (cc:
>> golang-dev@googlegroups.com),
>>
>> Please take another look.
>>
>>
>>
http://codereview.appspot.com/**4629047/<http://codereview.appspot.com/4629047/>
>>
>
>

[rsc, 2011-06-27 20:31:35]

LGTM

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode26
src/pkg/http/fs.go:26: return os.Open(filepath.Join(string(d),
filepath.Clean("/"+name)))
This will return a non-nil File always.
Instead, use

f, err := os.Open...
if err != nil {
    return nil, err
}
return f, nil

The "/"+name is suspect.
If name is a filepath then it can have \ on Windows
so it should be string(filepath.Separator)+name.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode35
src/pkg/http/fs.go:35: // The File interface is the subset of *os.File methods
needed
// A File is returned by a FileSystem's Open method and can be
// served by the FileServer implementation.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode48
src/pkg/http/fs.go:48: return os.Open(name)
Same as above.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode228
src/pkg/http/fs.go:228: // FileServer returns a handler that serves the files
// FileServer returns a handler that serves HTTP requests
// with the contents of the file system rooted at root.
//
// To use the operating system's file system implementation,
// use http.Dir:
//
//     http.Handle("/", http.FileServer(http.Dir("/tmp")))

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode233
src/pkg/http/fs.go:233: func FileServer(fs FileSystem) Handler {
s/fs/root/

[bradfitz, 2011-06-27 20:50:35]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 20:50:42]

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode26
src/pkg/http/fs.go:26: return os.Open(filepath.Join(string(d),
filepath.Clean("/"+name)))
On 2011/06/27 20:31:35, rsc wrote:
> This will return a non-nil File always.

Oh, right.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode35
src/pkg/http/fs.go:35: // The File interface is the subset of *os.File methods
needed
On 2011/06/27 20:31:35, rsc wrote:
> // A File is returned by a FileSystem's Open method and can be
> // served by the FileServer implementation.

Done.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode48
src/pkg/http/fs.go:48: return os.Open(name)
On 2011/06/27 20:31:35, rsc wrote:
> Same as above.

Done.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode228
src/pkg/http/fs.go:228: // FileServer returns a handler that serves the files
On 2011/06/27 20:31:35, rsc wrote:
> // FileServer returns a handler that serves HTTP requests
> // with the contents of the file system rooted at root.
> //
> // To use the operating system's file system implementation,
> // use http.Dir:
> //
> //     http.Handle("/", http.FileServer(http.Dir("/tmp")))

Done.

http://codereview.appspot.com/4629047/diff/10005/src/pkg/http/fs.go#newcode233
src/pkg/http/fs.go:233: func FileServer(fs FileSystem) Handler {
On 2011/06/27 20:31:35, rsc wrote:
> s/fs/root/

Done.

[bradfitz, 2011-06-27 20:57:52]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 21:01:03]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 21:03:16]

Updated gofix to ignore "" prefix (like it did for "/") and gofix'd godoc.

On Mon, Jun 27, 2011 at 2:01 PM, <bradfitz@golang.org> wrote:

> Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
> dsymonds@golang.org, r@google.com, gri@google.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**4629047/<http://codereview.appspot.com/4629047/>
>

[rsc, 2011-06-27 21:11:41]

Let's require the paths to be /-separated.

http://codereview.appspot.com/4629047/diff/19002/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/19002/src/pkg/http/fs.go#newcode33
src/pkg/http/fs.go:33: // The FileSystem interface represents the virtual
filesystem subset
// A FileSystem implements access to a collection of named files.
// The elements in a file path are separated by slash ('/', U+002F)
// characters, regardless of host operating system convention.
type FileSystem interface {
    Open(name string) (File, os.Error)
}

[bradfitz, 2011-06-27 21:25:10]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-06-27 21:25:58]

On Mon, Jun 27, 2011 at 2:11 PM, <rsc@golang.org> wrote:

> Let's require the paths to be /-separated.
>

Done, I think.  Double-check it well, though... the host path stuff makes me
nervous.

[rsc, 2011-06-27 21:42:33]

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode28
src/pkg/http/fs.go:28: string(d),
filepath.Clean(string(filepath.Separator)+filepath.FromSlash(name))))
// Reject paths using the alternate separator already.
if filepath.Separator != '/' && strings.IndexRune(name, filepath.Separator) >= 0
{
    return nil, os.NewError("invalid character in file path")
}
f, err := os.Open(filepath.Join(string(d), filepath.FromSlash("/" + name)))

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode158
src/pkg/http/fs.go:158: ff, err := os.Open(index)
fs.Open?

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode233
src/pkg/http/fs.go:233: // serveFile wants a '/'-separated name, but ServeFile
takes a
I'm really sorry I ever introduced ServeFile.
Or I'm sorry I don't have a time machine to go back and stamp out \ names.
One or the other. 

The part about filepath.FromSlash being a no-op and passing
a filepath through as a path behind the interface's back bothers me
(see code added above).  How about this (and delete osFileSystem)?

serveFile(w, r, http.Dir(name), "/", false)

?

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode258
src/pkg/http/fs.go:258: serveFile(w, r, f.root, path.Clean(r.URL.Path), true)
I think you can just pass r.URL.Path here.
I don't see why it needs to be cleaned.

[bradfitz, 2011-06-27 22:16:03]

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go
File src/pkg/http/fs.go (right):

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode28
src/pkg/http/fs.go:28: string(d),
filepath.Clean(string(filepath.Separator)+filepath.FromSlash(name))))
On 2011/06/27 21:42:34, rsc wrote:
> // Reject paths using the alternate separator already.
> if filepath.Separator != '/' && strings.IndexRune(name, filepath.Separator) >=
0
> {
>     return nil, os.NewError("invalid character in file path")
> }
> f, err := os.Open(filepath.Join(string(d), filepath.FromSlash("/" + name)))
> 

done, with Clean

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode158
src/pkg/http/fs.go:158: ff, err := os.Open(index)
On 2011/06/27 21:42:34, rsc wrote:
> fs.Open?

Done.

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode233
src/pkg/http/fs.go:233: // serveFile wants a '/'-separated name, but ServeFile
takes a
On 2011/06/27 21:42:34, rsc wrote:
> I'm really sorry I ever introduced ServeFile.
> Or I'm sorry I don't have a time machine to go back and stamp out \ names.
> One or the other. 
> 
> The part about filepath.FromSlash being a no-op and passing
> a filepath through as a path behind the interface's back bothers me
> (see code added above).  How about this (and delete osFileSystem)?
> 
> serveFile(w, r, http.Dir(name), "/", false)
> 
> ?

Seems weird to use a filename as a directory (http.Dir) but I guess?  Do you
mean "" or "/" as the file?  It seems "" would work

http://codereview.appspot.com/4629047/diff/4012/src/pkg/http/fs.go#newcode258
src/pkg/http/fs.go:258: serveFile(w, r, f.root, path.Clean(r.URL.Path), true)
On 2011/06/27 21:42:34, rsc wrote:
> I think you can just pass r.URL.Path here.
> I don't see why it needs to be cleaned.

It doesn't for http.Dir (which has to also do it, as it has its own contract
that it doesn't escape its jail), but FileSystem in general should be called
with cleaned paths I think?

[bradfitz, 2011-06-27 22:16:14]

Hello r@golang.org, rsc@golang.org, gri@golang.org, adg@golang.org,
dsymonds@golang.org, r@google.com, gri@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-27 22:20:56]

LGTM

[bradfitz, 2011-06-27 22:26:43]

*** Submitted as http://code.google.com/p/go/source/detail?r=850d94a4feca ***

http: add FileSystem interface, make FileServer use it

Permits serving from virtual filesystems, such as files linked
into a binary, or from a zip file.

Also adds a gofix for:

http.FileServer(root, prefix) -> http.StripPrefix(prefix,
http.FileServer(http.Dir(root)))

R=r, rsc, gri, adg, dsymonds, r, gri
CC=golang-dev
http://codereview.appspot.com/4629047