Feat: Added WriteContent to huma context in order to support reading from streams directly into http response #36
Conversation
danielgtaylor
left a comment
There was a problem hiding this comment.
Thanks for adding this feature! Left a couple comments, let me know what you think! Overall I think this is great, just trying to nail down a few details and keep the context interface as small/simple as possible.
context.go
Outdated
| // WriteContent requests. If set, WriteContent will add it as the | ||
| // Last-Modified header and will properly respond to Modified request | ||
| // headers. | ||
| SetContentLastModified(modTime time.Time) |
There was a problem hiding this comment.
I wonder if this should just be a required argument to WriteContent which you can set to e.g. time.Now() or time.Time{} if you don't care about it. It feels weird having it be a separate method on the interface which is only for WriteContent, especially when:
- The underlying
http.ServeContentrequires it as input - Any request could have a
Last-Modifiedheader and unless you are usingctx.WriteContentthen callingctx.SetContentLastModified(...)will in fact do nothing even if you expect the header to get set.
context.go
Outdated
| // explicitly rather than introduce this into the method signature as name | ||
| // is not applicable to every ReadSeeker. Leaving this blank or setting the | ||
| // Content-Type on the request disables this functionality anyway. | ||
| http.ServeContent(c.ResponseWriter, c.r, "", c.modTime, content) |
There was a problem hiding this comment.
I think passing a name for content-type analysis here would be reasonable. Huma handlers must declare the Content-Type header if they want to set it, but WriteContent could be the thing that actually sets it. Then you don't duplicate all that mimetype logic you probably don't care a ton about. Then you can still manually override if you want but the default is to just handle it for you.
Your responses.ServeContent already pre-declare the Content-Type header, so this should work great together.
The signature could be WriteContent(name string, content io.ReadSeeker, lastModified time.Time) or something along those lines.
There was a problem hiding this comment.
I guess both of your comments speak to me not finding the ServeContent function signature to be very intuitive. It has a lot of code paths that kick only when a particular field is the default value which to me seems not very user friendly at all. I was trying to make it as simplified as possible and provide only 1 way to interact with the function. Point taken that this might end up forcing people to reimplement the same mimetype logic that ServeContent was already doing, so I can add it back and provide comments, but I just wish there was a clearer way to handle this.
This is useful for large data requests, especially ones that need to be chunked into multiple range requests like direct video streaming. Since
http.ServeContentalready does exactly this and meticulously follows the associated RFCs, I decided to write this as a wrapper on top of that function.