add response types for various 300-level response codes #451
Conversation
dropshot/src/handler.rs
Outdated
| impl From<HttpResponseFoundNoContent> for HttpHandlerResult { | ||
| fn from(response: HttpResponseFoundNoContent) -> HttpHandlerResult { | ||
| const STATUS_CODE: http::StatusCode = | ||
| <HttpResponseFoundNoContent as HttpCodedResponse>::STATUS_CODE; |
There was a problem hiding this comment.
It'd be neat if there were a way to make sure this is the type of response here. It's easy to make a copy/paste error here. But I don't know how to reference an associated constant from a particular value's type (or, more precisely, a trait that that type impls).
|
I'm using these new interfaces in oxidecomputer/omicron#1757, in case you want to see what it looks like. |
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
This looks great. Thanks for doing it.
Related: I have no idea what I'd do in progenitor to handle this. reqwest claims to do redirections for us... in which case we'd have no idea what kind of data we might get back. My inclination is--I think--to omit operations that have a 3xx response code, but if you have any thoughts I'd love to hear them.
Hmm. In an ideal world, I don't think we want to skip them altogether. I think it would be useful to be able to call the login endpoints from a generated client. I expect those to return 303s. In these cases, it would be fine to ignore the Location header and treat the response code is a general success code. That's probably true for most 303s, since the point is usually to show a confirmation page or the like. A 302 or 307 is a little different. The server is telling you "this thing is over there -- try again there". On the one hand, I don't think any of the operations in Nexus that returns these codes is that useful to all from a progenitor client. (They're basically the endpoints that redirect a user to a web page to sign in.) On the other hand, I think it's plausible we'd want to use these for other purposes. Examples:
But I see the problem that the generated client doesn't know what it's going to get back. Maybe we could add some other bit of metadata in the OpenAPI spec for the operation id that we might redirect you to? That won't really help for the case where we add a 302/307 for compatibility reasons. In that case, presumably the 302/307 wouldn't be in the openapi spec at all, there'd be some 200-level response instead, and the expectation would presumably be that you're getting that 200-level response content at the new URL. I can see how this gets pretty messy. |
|
Maybe we can just generate a client function with a completely generic response value. |
My goal here is to add
HttpResponse*types that you can use to have your Dropshot endpoint return each of these 300-level response codes:@ahl I'm interested in a bunch of your thoughts here. The way I've done this, unlike the other response codes, these are functions that return
HttpResponseHeaders. That way, the OpenAPI spec reflects theLocationheader. It's a little weirdly not-parallel to the existing response types but I think the result is pretty good: you have to provide a location, we take care of putting it in the right place, and it lands in the spec.On the specific response codes: these are all ways to redirect the client to a different URL. People have traditionally used 302. The MDN docs I linked to (and the linked HTTP specs) point out that you should really use 307 if you want the browser to basically retry this request at the other URL (which means the HTTP method and request body should be re-used for the second request), and you should really use 303 if you want the browser to make a "GET" to the other URL. 303 seems intended for the case where, say, you're completing a
POSTorPUTand you want to redirect somebody to a confirmation page.About the
Locationheader and body: I believe HTTP does not require that responses with these status codes have aLocationfield, but I think at least for our use cases we should always have one, so I made it required here. HTTP recommends a "small hypertext document" that conveys the gist of the redirect. This seems irrelevant for an API so I just made these have an empty body.Use case: This came up for me in omicron's login endpoints. These currently construct
Response<Body>by hand, which is error-prone (and there are errors) and also doesn't describe the result in the OpenAPI spec. I want to make these use a first-class dropshot-provided response type. There are two cases: when youGETsome login-related URLs, you may get redirected to the single sign-on page at another server (the external authentication provider). In this case, any of these response codes is probably fine because the request came in as a GET (which has nobody) and you want it to be redirected as a GET (with no body). 303 seems most precise. The other case is when youPOSTto an endpoint that accepts credentials, and thePOSTquery parameters or request body also provide a URL that you're supposed to be redirected to on successful login. (The usual flow is that you access resource X, that sends you over to login, then you login, and we want to send you back to X.) This should probably be a 303, since we want you toGETthis URL, notPOSTit.