allow consumers to add headers to responses #276
Conversation
dropshot/src/handler.rs
Outdated
| #[derive(Default)] | ||
| pub struct RequestContextMutableState { | ||
| response_headers: | ||
| Vec<(http::header::HeaderName, http::header::HeaderValue)>, |
There was a problem hiding this comment.
Might it make more sense for this to be a mapping? Or are multiple headers with the same name allowed?
There was a problem hiding this comment.
I guess I could see it either way. If it was a mapping would your proposal be that we return an error if there's a collision?
There was a problem hiding this comment.
I believe multiple headers with the same name are allowed in HTTP, and it looks like hyper's representation is a multimap. You could even use that here.
I'd suggest we provide some combination of:
- "set" header -- replaces any previous value (corresponds to
HeaderMap::insert) - "append" header -- adds a new header (if you use this multiple times, you'll get multiple values for this header in the response -- corresponds to
HeaderMap::append)
The former is less error-prone for most cases, but the latter is more general. I don't have much preference about which or both.
dropshot/src/handler.rs
Outdated
| #[derive(Default)] | ||
| pub struct RequestContextMutableState { | ||
| response_headers: | ||
| Vec<(http::header::HeaderName, http::header::HeaderValue)>, |
There was a problem hiding this comment.
I believe multiple headers with the same name are allowed in HTTP, and it looks like hyper's representation is a multimap. You could even use that here.
I'd suggest we provide some combination of:
- "set" header -- replaces any previous value (corresponds to
HeaderMap::insert) - "append" header -- adds a new header (if you use this multiple times, you'll get multiple values for this header in the response -- corresponds to
HeaderMap::append)
The former is less error-prone for most cases, but the latter is more general. I don't have much preference about which or both.
dropshot/src/handler.rs
Outdated
| */ | ||
| pub fn add_response_header( | ||
| self: Arc<Self>, | ||
| header: http::header::HeaderName, |
There was a problem hiding this comment.
nit, and please feel free to ignore: I see that HeaderMap::insert() takes IntoHeaderName, and I wonder if that's not more general in a useful way. We could even expose a &mut HeaderMap directly to people and avoid dealing with a bunch of these issues ourselves.
There was a problem hiding this comment.
That would make it much easier to use. Good idea.
There was a problem hiding this comment.
I agree with Dave here. Let HeaderMap handle issues of conversion, duplicate values, etc.
| pub fn add_response_header( | ||
| self: Arc<Self>, | ||
| header: http::header::HeaderName, | ||
| value: http::header::HeaderValue, |
There was a problem hiding this comment.
What do we want to do if somebody tries to set a header that Dropshot really should own (like maybe "date" or "transfer-encoding")? I can imagine:
- silently ignore it
- let them clobber it
- panic
- make this function return a
Result, even though it feels like this is something that could be detectable at compile time, except that I don't see how to do that
I hate all of these. What do you think?
There was a problem hiding this comment.
I think we let them clobber it. It's either going to let consumers do something ... tricky/sketchy, or be a foot-gun, but it's a foot-gun that would--I think--require careful aiming. In other words, I think it might be a useful escape hatch and is unlikely to cause mysterious errors.
| let funcparams = Extractor::from_request(Arc::clone(&rqctx)).await?; | ||
| let future = self.handler.handle_request(rqctx, funcparams); | ||
| future.await | ||
|
|
||
| future.await.map(|mut response| { |
There was a problem hiding this comment.
Discussed offline. Summary: this only sets the header on success. That's probably okay (maybe better) for our expected use case but we should rename these functions for clarity.
A subtle case that came up: suppose we set the ETag header when looking up an object, then did an authz check and decided you couldn't see it, and sent a 404 with the ETag header. We'd leak information. (This isn't a problem if the behavior here is to only set these headers on success.)
|
closing in favor of #283 |
This it to unblock @bnaecker
@davepacheco I can see other ways of doing this (as we discussed); this seemed the least invasive, but I'm happy to weave this in more seamlessly if it seems objectionable.