-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
net/http: add ServeFileFS, FileServerFS, NewFileTransportFS #51971
Comments
When I wanted the equivalent of ServeFSFile, I bodged it together by converting the fs.FS to an http.FileSystem with http.FS, and then calling http.ServeContent with the resulting http.File objects. It would have been easier if I could have skipped all that. |
There are two suggestions here. (1) Add ServeFSContent(w, r, info, content) next to ServeContent(w, r, name, modtime, content), where info replaces name+modtime and content is loosened from ReadSeeker to Reader. This seems like a mistake to me. In particular, ReadSeeker is not only there for finding the size. It is also there to serve range requests, and your clients will be very unhappy if you are serving large files and can't seek to serve the range requests. So I don't think we should change Reader to ReadSeeker. That leaves only name+modtime, and writing info.Name(), info.ModTime() instead of info seems like a small price to pay for having just one function instead of two. (2) Add ServeFSFile(w, r, fsys, name) next to ServeFile(w, r, name). The alternative today is to do the open yourself and then call ServeContent. This seems like a more plausible place where we could make things better. I would suggest calling it ServeFS instead of ServeFSFile though. |
This proposal has been added to the active column of the proposals project |
Based on the name, I would expect http.ServeFS to be the equivalent of http.FileServer(http.FS(fsys)). ServeFSFile is a bit clearer. |
Retitled to be just about ServeFSFile. Does anyone object to adding that? |
Right now, we have If we add I think if we add |
Yeah, as long as you unconditionally type assert to a ReadSeeker. Reading between the lines of these comments, it sounds to me like you're saying it's reasonable that code which wants to serve For some reason, when I wrote this proposal I had it in my head that embed.FS files are not seekable (which is not the case). So I'm basically fine with saying that only seekable file systems are allowed here, in which case:
I updated the proposal description. |
@neild that direction sounds great to me. (This proposal came out of some exercises where I wrote a bunch of code that uses both net/http and io/fs but tries to avoid mentioning http.File{System} at all. To my mind, they're inherently deprecated due to the existence of io/fs, even if they aren't marked as such.) To me, adding
I think I see what you're getting at, but to be precise, there are no |
I need to reread all this and think more carefully about it. It's clear there's a can of worms we should try to avoid opening. |
IMO IMO the machinery |
While we're at it, I'm also inclined to think that I understand that we can't change [edit] For context, RFC 7231 says about
(emphasis mine) Which IMO both supports the claim that |
Sorry for the delay. I was confused by the comment, and it took me a while to page everything to understand it. In doing so I realized my confusion was caused by some typos that I have corrected in the original and in this quote. (They were mentions of http.File and fs.File that should have been http.FileSystem and fs.FS.) I agree with the comment, now that I understand it. For naming I think we should put the FS at the end of the name like we did in template.ParseFS, os.DirFS, and so on. So that would be http.ServeFileFS, http.FileServerFS, and http.NewFileTransportFS. Specifically:
Thoughts? |
@rsc that sounds great to me. My original proposal was trying to be as minimal as possible and only |
Then the new API is:
Retitled. Does anyone object to adding this API? |
I think it is better to move these functions to separate package, like Then: package httpfs
func ServeFile(w http.ResponseWriter, r *http.Request, fsys fs.FS, name string)
func FileServer(root fs.FS) http.Handler
func NewFileTransport(fsys fs.FS) http.RoundTripper |
We're not going to move just these three into a separate package. http.FS is already in net/http. |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
Is anyone working on this for 1.21? |
@carlmjohnson It's on my todo list but I'm having trouble finding the time. You (or anyone else) should feel free to take it if you want. |
These new apis are analogous to ServeFile, FileServer and NewFileTransport respectively. Fixes golang#51971
These new apis are analogous to ServeFile, FileServer and NewFileTransport respectively. The main difference is that these functions operate on an fs.FS. Fixes golang#51971
These new apis are analogous to ServeFile, FileServer and NewFileTransport respectively. The main difference is that these functions operate on an fs.FS. Fixes golang#51971
Change https://go.dev/cl/513956 mentions this issue: |
Change https://go.dev/cl/549198 mentions this issue: |
For #51971 For #61679 Change-Id: Ie7b44201a9c40f5563c6d6051d22ae807ad0480d Reviewed-on: https://go-review.googlesource.com/c/go/+/549198 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Damien Neil <[email protected]> Reviewed-by: Mauri de Souza Meneguzzo <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]>
For golang#51971 For golang#61679 Change-Id: Ie7b44201a9c40f5563c6d6051d22ae807ad0480d Reviewed-on: https://go-review.googlesource.com/c/go/+/549198 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Damien Neil <[email protected]> Reviewed-by: Mauri de Souza Meneguzzo <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]>
Update 2: The final agreed-upon API is in this comment:
Update: The current proposal is to add the function
ServeFSFile
tonet/http
. This is an analogue toServeFile
which interoperates withio/fs
file systems.Below is the original proposal.
Abstract
To better interoperate with
io/fs
, I propose adding two functions tonet/http
.ServeFSFile
, theio/fs
-based analogue toServeFile
.ServeFSContent
, theio/fs
-based analogue toServeContent
.Background
The
net/http
package provides three built-in ways of serving files:ServeFile
, which serves a file by nameServeContent
, which serves a file from anio.ReadSeeker
and some additional metadataFileSystem
, which is turned into aHandler
usingFileServer
These were written before the
io/fs
package existed and do not work with those interfaces.As part of adding
io/fs
,net/http
gainedFS
which converts anio.FS
into aFileSystem
.However,
ServeFile
andServeContent
have nofs.FS
-based equivalents.Proposal
ServeFSFile
ServeFile
lets the caller easily serve the contents of a single file from the OS file system.ServeFSFile
lets the caller do the same for anfs.FS
.Both of these functions take a filename. The name passed to
ServeFile
is OS-specific; the name passed toServeFSFile
follows theio/fs
convention (slash-separated paths).ServeFSContent
ServeContent
is a lower-level function intended to serve the content of any file-like object. Unfortunately, it is not compatible withio/fs
.ServeContent
takes anio.ReadSeeker
; seeking is used to determine the size of the file. Anfs.File
is not (necessarily) aSeeker
. However, thefs.FileInfo
interface provides the file's size as well as name and modification time.Therefore, instead of
we can pass in
The behavior of
ServeFSContent
is otherwise the same asServeContent
:Questions
Should these functions instead be implemented outside the standard library?
It is not trivial to implement these functions outside of
net/http
. The proposed functions are building blocks upon which other functionality can be built; it is not possible to write these functions simply in terms of the existingnet/http
API.The
ServeFile
andServeContent
functions do quite a lot of subtle work (path cleaning, redirects, translating OS errors to HTTP responses, content-type sniffing, and more). Implementing this proposal outside ofnet/http
requires either copying a lot of its internal code or reimplementing a good amount of functionality (some of which comes with security implications).I believe that we should add these proposed functions to
net/http
so that it supportsio/fs
just as well as it supports OS files.Should
ServeFSContent
have a different signature?We could simplify the signature of
ServeFSContent
by having it take anfs.File
:and then
ServeFSContent
would callf.Stat
itself.That's not entirely satisfying; it seems to be unusual to pass an
fs.File
around separately from anfs.FS
, andClose
is not used.Another option would be to pass in all the fields explicitly. (This is the same as
ServeContent
except that instead of aReadSeeker
we pass in the size.) Since this is now notio/fs
-specific at all, I gave it a new name:The text was updated successfully, but these errors were encountered: