Multiple response types not supported

[jayvdb]

It is not possible currently to have two response types due to the assert at
https://github.com/oxidecomputer/progenitor/blob/1b942d1/progenitor-impl/src/method.rs#L1118-L1120

I wonder if it is easier and acceptable to slightly improve the situation by allowing one response body type, and one or more additional responses that do not include a body.

e.g. the following fails the assertion, but the second response has no content

          responses:
            '200':
              description: Success
              content:
                application/json:
                  schema:
                    $ref: '#/components/schemas/User'
            # Definition of all error statuses
            default:
              description: Unexpected error

[augustuswm]

I started on an implementation for supporting multiple responses types and came up with a couple options.

Currently each method has some fixed T for its ResponseValue, and that T can be one of:

• A JSON deserializable type
• An empty body, represented as ()
• A stream of bytes
• A reqwest::Upgraded connection

To get to multiple response types a method needs to be able to return a type that may be (at runtime) any of these types. In basic cases where the options are only multiple JSON deserialized types, all of the following are going to be similar for the user of the generated client, and so more of the focus is on problematic endpoints. Also while most of this is focused on the response values, the same patterns can apply to Error responses.

Composing Types

One option is to represent these as composable types, where Option<T> represents a potentially empty body and an upgradable response is represented as:

// Represents a response that may be upgraded
pub enum Upgradable<T> {
    Upgrade(reqwest::Upgraded),
    Value(T),
}

Imagining a method that can return an empty body, an upgraded connection, or a deserialized body (GetRecordResponse), it would look like:

pub async fn get_records<'a>(
  &'a self,
) -> Result<ResponseValue<Upgradable<Option<types::GetRecordsResponse>>>, Error<GetRecordError>> {
  unimplemented!()
}

This is a fairly straightforward solution to implement, and is easy (though tedious) to extend with new composing types. I think the experience for an end user of the client though is not great when handling particularly messy endpoints. The optional body case of ResponseValue<Option<Inner>, Error<Option<InnerError>>>, is fairly intuitive. But an endpoint with a return value that starts looking like ResponseValue<ByteStream<Option<Inner>>, Error<Option<InnerError>>> is pretty verbose and is asking the caller to unpack the full type on every call.

Progenitor Enum

There are two options for enums here, and for an end user they are close to equivalent. In both cases the goal is to represent all of the possible inner: T types as a enums. One path to implementing this is to give progenitor the responsibility of generating the enum. In this model, typify only knows about types that are deserializable.

// This type is generated by typify
#[derive(Deserialize)]
pub enum GetRecordsResponse {
  // ...
}

// This type is generated by progenitor
pub enum GetRecordsInner<GetRecordsResponse> {
  Bytes(ByteStream),
  Data(GetRecordsResponse),
  Empty,
  Upgrade(reqwest::Upgraded),
}

pub async fn get_records<'a>(
  &'a self,
) -> Result<ResponseValue<GetRecordsInner<GetRecordsResponse>>, Error<GetRecordError>> {
  unimplemented!()
}

It would be expected that the progenitor generated enum only contains the variants that are relevant to a given method. I think this construction is a little easier for a user of the generated client to interact with than the compositional version. Each variant path here is at a fixed depth across all methods, meaning that the handling for all methods would look roughly the same (modulo the possible variants).

A few major downside to this approach:

• Progenitor is now also responsible for generating types, and typify no long has the full context of the generated types.
• A lot of "effectively" duplicated code. Imagine two endpoints that can return a JSON body or a redirect with an empty body. They both generate a *Inner enum that are effectively equivalent.
• The single response case (non-enum) either has a extraneous *Inner enum (to keep the response patterns that same across methods).

Typify Enum

I think this provides the best API for users of the generated client, but would require likely the most effort to implement. The core idea is to take the enum from the progenitor option, flatten it, and generate it via typify. In practice it would look like:

// This type is generated by typify
#[derive(Deserialize)]
pub struct Record {
  // ...
}

// This type is generated by typify
pub enum GetRecordsResponse {
  Bytes(ByteStream), // Injected via with_conversion
  Record(Record),
  // Multiple other possible deserializable types
  Empty, // TODO: Unsure how to implement
  Upgrade(reqwest::Upgraded), // Injected via with_conversion
}

pub async fn get_records<'a>(
  &'a self,
) -> Result<ResponseValue<GetRecordsResponse>, Error<GetRecordError>> {
  unimplemented!()
}

Implementing this requires some integration with typify. In particular typify by default uses a standard set of derives, and the *Response types no longer always satisfy them. Progenitor would then be responsible for outputting individual from_response implementations:

impl ResponseValue<GetRecordResponse> {
  pub async fn from_response(response: Response) -> Result<Self, Error<GetRecordError>> {
    let status = response.status();
    let headers = response.headers().clone();
    let inner: GetRecordResponse = match status {
      101u16 => GetRecordResponse::Upgrade(response.upgrade().await.map_err(Error::InvalidResponsePayload)?),
      200u16 => GetRecordResponse::Record(response.json().await.map_err(Error::InvalidResponsePayload)?),
      204u16 => GetRecordResponse::Empty,
      // ... continue for any other status codes
      400u16 => return Err(Error::ErrorResponse(GetRecordError::from_response(response).await?)),
      500u16 => return Err(Error::ErrorResponse(GetRecordError::from_response(response).await?)),
    };

    Ok(Self {
      inner,
      status,
      headers,
    })
  }
}

Similar to the progenitor enum case, there is some duplication here that I'm unsure how to get rid of. There are also some open questions around if typify can currently generate the enum as presented, specifically the ::Empty case that does not carry a value.

Overall I believe this is the best of these three options, though there may be other options I'm not thinking of.

[jayvdb]

I think there is another possible option that is much simpler to implement & use, but would not able to handle some stranger APIs.

First there needs to be better handling of multiple responses, that vary only by response status code. i.e. OpenAPI's responses.default needs first class support, and these should be the Err(..). The spec is clear this is intended for error scenarios. IMO reasonable assumptions can be made, like these errors cannot be a bytestream. The body must be empty or an object with a sane schema. Ideally any OpenAPI which has multiple 4xx/5xx codes which are effectively a default, having the same body, should be able to be represented using the same approach as a default. One exception to this would be if there is only a default: and no other response types, it might be sensible to put it into the Ok(..) instead of the Err(..)

With error response schemas out of the way, if there are still multiple response types, instead of merging all of the possible response values into one function, each possible value has its own function. i.e. the caller must know what response type they are expecting. I believe this is a safe assumption for sane APIs, where something like GET /foos/ may have a bunch of query params that determine whether it returns a Foo or a FooList or something else. In a sensible API, the caller knows which endpoint params will trigger each response type.

This approach would mean the calling code is more readable & more resilient, due to it clearly saying what it expects from the function being invoked, less unpacking of the Result returned, and it does not break when a new response type is added to the endpoints spec since the code has explicitly declared that it only cares about one of the response type.

This could also be achieved with generics, so it is the same function name, with the caller invoking the function with the type expected.

This approach doesn't preclude a more robust unified function (like Augustus's proposal above) also being generated at a later time. Then the caller can choose to invoke either the functions which have a single response type, if they are only expecting that type, or use the unified function if they dont know what response type to expect, and thus need to have unpacking code which explores all the options, and breaks at compile time if new response types are added to the endpoint definition.

[antifuchs]

I've stumbled across this issue also, trying to generate an api client for the jellyfin API - that one is a little more cursed, as its 200 (and some other) responses can be specified to return different options on JSON-formatted responses (specifically, for capitalization preferences in JSON object key names). E.g.,

{
  "paths": {
    "/Branding/Configuration": {
      "get": {
        "tags": [
          "Branding"
        ],
        "summary": "Gets branding configuration.",
        "operationId": "GetBrandingOptions",
        "responses": {
          "200": {
            "description": "Branding configuration returned.",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/BrandingOptions"
                }
              },
              "application/json; profile=\"CamelCase\"": {
                "schema": {
                  "$ref": "#/components/schemas/BrandingOptions"
                }
              },
              "application/json; profile=\"PascalCase\"": {
                "schema": {
                  "$ref": "#/components/schemas/BrandingOptions"
                }
              }
            }
          }
        }
      }
    }
  }
}

I feel this is really a ridiculous amount of customizability (especially since the capitalizations do not change even when given options!) - and given that, my current thinking is that maybe it would be fine to allow users to specify the content type they want to consider the default?

[felixauringer]

I am running in the same problem. It's not only a problem when using default in the responses object or when having different content types but also if there are different status codes of which only some return a body. Here is a minimal example:

{
  "openapi": "3.0.0",
  "paths": {
    "/v1/health": {
      "get": {
        "operationId": "health",
        "responses": {
          "400": {
            "description": "400"
          },
          "404": {
            "description": "404",
            "content": {
              "application/json": {
                "schema": {}
              }
            }
          }
        }
      }
    }
  },
  "info": {
    "title": "Test API",
    "version": "0.0.0"
  }
}

(I removed the schema for brevity.)

I think that such API definitions are rather common and I would be really happy about a fix. 🙂
How is your implementation going @augustuswm?

[augustuswm]

I unfortunately have been working on other projects in the recent time. I am looking to get back to this though when I pick back up on trying to get a generated GitHub client put together.

[seddonm1]

Hi. Similar problems with the Ory Kratos spec available at https://github.com/ory/kratos/blob/master/.schema/openapi.json. It is a shame be openapi-generator works but I much prefer the Progenitor/Dropshot ecosystem. Leaving this here if anyone needs test data.

[bradhe]

Just for priortization sake, we're looking to switch to progenitor from openapi-generator as well due to build tooling integration; however, generation also barfs on our API (https://api.tower.dev/v1/openapi-3.0.yaml, as of this writing in Gist) for the same reason.