WIP: Extend WebSession to support range requests. #2887
Conversation
|
@zenhack How do you feel about this issue and WIP? It was specifically mentioned as a pain point recently. |
| struct PutContent { | ||
| # TODO(apibump): Remove this and replace it with `PostContent` (renamed to `Content`). | ||
|
|
||
| mimeType @0 :Text; |
There was a problem hiding this comment.
A bit of a tangent, but: would it actually break anything to delete PutContent per the comment? the layout is exactly the same so it should be wire compatible. The type IDs are different, but it's hard for me to imagine that anything uses that.
There was a problem hiding this comment.
It'd be wire-compatible, but not source-compatible. Some apps could have their builds broken. Maybe we don't care that much...
There was a problem hiding this comment.
I doubt very many apps actually use this directly, and for the ones that do it's a trivial fix. But probably doesn't make sense to change it as part of this patch.
| position @2 :UInt64; | ||
| } | ||
|
|
||
| fullSize :union { |
There was a problem hiding this comment.
As far as I can tell, this is only valid in responses. Maybe we should remove it, since we only currently use the Range struct in requests?
There was a problem hiding this comment.
Huh. It's been a long time since I wrote this, but I wonder if I had intended to include a Range under Response.content but forgot... otherwise I'm not sure how the Content-Range header is supposed to be constructed for the response.
There was a problem hiding this comment.
See the comment you wrote below -- looks like the idea was to just insist that the app returns the range that was actually asked for, so the supervisor only needs to check the status code.
There was a problem hiding this comment.
But how is the overall size communicated? I guess the final Content-Range header would always end up ending in /*?
BTW, what makes you say that fullSize is not valid for requests? It doesn't appear in a GET request (Range header), but a PUT that is uploading a range (Content-Range header) could include it, right? (However, I'm not sure if it's actually worth supporting range PUTs...)
There was a problem hiding this comment.
Yes, I was only thinking about GET requests. I guess I don't feel that strongly about this.
| # for, sandstorm-http-bridge will throw an exception instead.) | ||
|
|
||
| # This seems to fit better here than in the 3xx range | ||
| # TODO(apibump): Only the three status codes above should actually be used. The status codes |
3 participants
This is just the protocol change for now. Someone additionally needs to update
proxy.jsandsandstorm-http-bridge.c++and perhapsbridge-proxy.c++.