Skip to content
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

Handle byte streams for both success and error responses #35

Merged
merged 2 commits into from
Mar 15, 2022
Merged

Conversation

ahl
Copy link
Collaborator

@ahl ahl commented Mar 13, 2022

When an OpenAPI spec reports that errors may be unstructured, generated clients need a way to express this. Currently they just use () which is obviously imprecise.

There were three options I considered:

  1. Add another Error variant alongside ErrorResponse(ResponseValue<E>)--something like UnstructuredErrorResponse(Response)--which would have the raw response
  2. Change Error<T> which currently takes any type T to take either a ResponseValue<T> or a Response
  3. Add a new type ByteStream to represent the unstructured response body; this could be used within a ResponseValue<ByteStream>

Each of these has its drawbacks. 1 requires clients to consider two--essentially identical--error variants, only one of which would only be relevant for a given API call. 2 imposes the knowledge of the structure of Error types more onerously on mock clients; it requires exposing a trait to treat ResponseValue<T> and Response generically which can also lead to confusing error messages. 3 doesn't give access to rest of the Response structure, extracting only the streaming body via Response::bytes_stream().

After prototyping all three and trying them out with omicron, I chose 3. 1 felt pretty janky with the two nominally conflicting Error variants; 2 felt easy to get wrong and seemed to expose more implementation detail than was desirable; 3 actually had some nice properties of unifying type and status code handling of success and error responses. With 3, we figure out the type of the response T--either a generated type, a ByteStream, or ()--and then the success case is a ResponseValue<T> and the error case is an Error<T>. This has a pleasant symmetry to it.

In Dropshot, the OpenAPI description for an endpoint whose response is completely free-form uses the default response type and the */* content type: there's nothing specific Dropshot can say about the response codes, content type, schema, etc. In those cases, the return value for the corresponding generated client method will be Response<ResponseValue<ByteStream>, Error<ByteStream>> where the only thing distinguishing the success and error cases is the response code that the endpoint applied to its custom response.

For oxidecomputer/dropshot#297, with this PR a method with a streaming response but error types would have a return type that looks like this: Response<ResponseValue<ByteStream>, Error<types::Error>>.

Note that both @leftwo and @iliana have hit conditions applicable to this PR.

@davepacheco
Copy link

That sounds like a good approach!

Copy link

@leftwo leftwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (along with latest dropshot) solves the problem I was having!

@@ -0,0 +1,59 @@
pub use progenitor_client::{ByteStream, Error, ResponseValue};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are expected output for test comparison purposes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahl ahl merged commit ea80069 into main Mar 15, 2022
@ahl ahl deleted the unknown-mime branch March 15, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants