io/fs: add func Sub(fsys FS, dir string) FS

@zikaeroh

Originally posted by @zikaeroh in #41191 (comment)

Trying this out now; one wart with the go:embed directive is that if I embed build/*, the filenames still have the prefix build/. If I want to then serve that directory via http.FS, there's no easy way to add the prefix that's required to access them if needed (without writing a wrapper, which then hits the problem of needing to list out every potential method that the FS may have...).

e.g.:

//go:embed build/*
var buildDir embed.FS

// Serve some SPA build dir as the app; oops, needs to be build/index.html
http.Handle("/", http.FileServer(http.FS(buildDir)))

// or

//go:embed static/*
var staticDir embed.FS

// Oops; needs to have a static prefix.
http.Handle("/static/*, http.StripPrefix("/static", http.FileServer(http.FS(staticDir))))

// Could be this, but only because the prefix happens to match:
http.Handle("/static/*, http.FileServer(http.FS(staticDir)))

I know the intent is that one could write go:embed foo/* bar/* baz.ext and get all of those files, but I think it's going to be very common to simply embed a directory and serve it as static assets via the http package. I expect this to be a gotcha as people switch from things like http.Dir("static") or pkger.Dir("/internal/web/static") where the prefix is already handled, to the new embed.FS.

I'm not really sure how to file this, as it's sort of an interplay with embed, io/fs, and net/http.

My comment on @zikaeroh's comment:

I think the best way to resolve this is by adding a general purpose fs.WithPrefix helper that creates a new FS that is restricted to the given subdirectory prefix. The example above would become:

//go:embed build/*
var buildDir embed.FS

http.Handle("/", http.FileServer(http.FS(fs.WithPrefix("build", buildDir))))

I think this should be in FS so that it can implement optional interfaces as they're invented. I think it will have general applicability for things like creating a zipfile FS and restricting it to a subdirectory and whatnot.

The main issue with this approach is the whole optional method thing; the moment you use this helper all additional FS methods are lost. To do it "correctly", you end up having to do what httpsnoop had to do to deal with the optional interface problem with ResponseWriter, or if the ErrNotSupported value becomes standard, make io/fs somehow implement every possible method (because you couldn't just embed the result of a new fs.WithPrefix and add on without losing the extra methods entirely). This all was brought up during the design of io/fs on Reddit and the issue thread.

For net/http, this doesn't matter, since http.FS only uses Open, but for more generic uses of io/fs, this might matter more. I mentioned in a followup comment that I could achieve the same thing with an AddPrefix counterpart to StripPrefix in net/http (#41191 (comment)), which might work better, but doesn't solve the overall problem of "I can't subtree an FS".

I'm still not certain if this is a "problem" with how static serving works in net/http (the awkward set of needing all of the prefix strippers to modify Request), or the general problem that the design of io/fs doesn't lend itself to subtreeing if you want retain efficiency. Maybe the later isn't 100% true, though, since who knows how any given FS would really want to handle prefixes.

In the comments on the io/fs proposal, as I understood it, the consensus was that the "right" way to deal with the optional interfaces was to implement them all and then return ErrNotSupported if the underlying FS didn't have the method in question.

Forgive me, I meant ErrUnsupported (#41198), not ErrNotSupported (which is a different thing...). Note that while ErrUnsupported is accepted, it's not implemented and not used in io/fs.

@zikaeroh

@zikaeroh The general recommendation for optional methods is to add them all and fall-back to the appropriate helper in io/fs if you can't implement them specifically. That helper should then do the correct thing (i.e. type-assert OR fallback-implementation OR return ErrUnsupported). That it was not necessary to use ErrUnsupported (or equivalent) in io/fs so far is not a reason it couldn't be used.

If there is an optional interface for which this approach doesn't work, details would be very useful (probably in a separate bug) because it would point to a serious design issue which should be discussed before go 1.16 is released. I don't know of any such instance yet, but that doesn't mean it doesn't exist. In particular, I don't see any problems in interactions with optional interfaces I know about and a hypothetical StripPrefixFS.

I think this proposal is a good idea and I would very much like to see it happen. To me, this is a fundamental primitive of composability for file systems and I would like to see it in io/fs - just like io contains MultiReader, TeeReader and other fundamental composability primitives of I/O-streams.

@rsc

/cc @rsc

My initial instinct was that this would be useful because it could add more safety when trying to "sandbox" parts of the filesystem. But then I remembered that in the io/fs draft design .. is forbidden already. In theory this seems like a pretty easy to implement proposal (string concatenation and all the stuff needed to handle optional methods, right?) Is there some subtle reason this should be in the standard lib?

(apologies for poorly formatted example; written directly in browser.)

type WPFS struct {
   inner  FS
   prefix string
}

func (fs WPFS) Open(p string) (File, error) {
   return fs.inner.Open(inner.prefix + "/" + p)
}

// same thing but for ReadFile, Stat, ReadDir, Glob, and maybe Rename, OpenFile?

func WithPrefix(f FS, p string) WPFS {
   return WPFS{f, p}
}

Is there some subtle reason this should be in the standard lib?

No, not a subtle reason. The non-subtle reason is what I mentioned above: It's a fundamental primitive of composition, so making it available is akin to other top-level functions in the io package, for example.

An alternative name could be Chroot

When we talked about adding this during the proposal discussion on Reddit, I was thinking it could be called just fs.Sub, as in sub-tree. Chroot is a bit too obscure (but accurate!), and WithPrefix is maybe too much about the mechanics (and maybe inaccurate! The argument to Open is without the prefix).

I was thinking we could put this off until the next release and get more experience with io/fs, but I agree that it is a critical piece to have to use with embedding.

Does anyone object to adding func Sub(fsys FS, dir string) FS to io/fs and also the corresponding SubFS interface?

I went to implement this, and it does seem like we need the error result if only for diagnosing invalid arguments properly.
So that's what the CL has.

Change https://golang.org/cl/274856 mentions this issue: io/fs: add Sub

io/fs: add Sub

Navigation Menu

Repository navigation

io/fs: add func Sub(fsys FS, dir string) FS #42322

15 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

io/fs: add func Sub(fsys FS, dir string) FS #42322

Description

Activity

zikaeroh commented on Nov 1, 2020

earthboundkid commented on Nov 1, 2020

zikaeroh commented on Nov 1, 2020

Merovius commented on Nov 1, 2020

toothrot commented on Nov 2, 2020

frioux commented on Nov 4, 2020

Merovius commented on Nov 4, 2020

icholy commented on Nov 10, 2020

rsc commented on Nov 11, 2020

15 remaining items

rsc commented on Dec 2, 2020

gopherbot commented on Dec 2, 2020

shibumi commented on Apr 18, 2021

Merovius commented on Apr 18, 2021

shibumi commented on Apr 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions