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

Add support for typed headers in the response #77

Open
Joe-Dunleavy opened this issue Sep 28, 2022 · 7 comments
Open

Add support for typed headers in the response #77

Joe-Dunleavy opened this issue Sep 28, 2022 · 7 comments
Labels
enhancement New feature or request open-api-spec

Comments

@Joe-Dunleavy
Copy link
Contributor

Joe-Dunleavy commented Sep 28, 2022

Example response object:

MicrosoftTeams-image

@Joe-Dunleavy Joe-Dunleavy changed the title Add support for returning headers in the response Add support for headers in the response Sep 28, 2022
@ColinEberhardt
Copy link
Collaborator

Interestingly this is not supported by openapi-generator OpenAPITools/openapi-generator#12855

@ColinEberhardt
Copy link
Collaborator

The pet store API has an example which returns headers as part of the response:

    "/user/login": {
      "get": {
        "tags": ["user"],
        "summary": "Logs user into the system",
        "description": "",
        "operationId": "loginUser",
        "parameters": [
          {
            "name": "username",
            "in": "query",
            "description": "The user name for login",
            "required": false,
            "schema": { "type": "string" }
          },
          {
            "name": "password",
            "in": "query",
            "description": "The password for login in clear text",
            "required": false,
            "schema": { "type": "string" }
          }
        ],
        "responses": {
          "200": {
            "description": "successful operation",
            "headers": {
              "X-Rate-Limit": {
                "description": "calls per hour allowed by the user",
                "schema": { "type": "integer", "format": "int32" }
              },
              "X-Expires-After": {
                "description": "date in UTC when token expires",
                "schema": { "type": "string", "format": "date-time" }
              }
            },
            "content": {
              "application/xml": { "schema": { "type": "string" } },
              "application/json": { "schema": { "type": "string" } }
            }
          },
          "400": { "description": "Invalid username/password supplied" }
        }
      }
    },

Currently our TypeScript generator creates a method with the following signature:

async loginUser(username?: string, password?: string): Promise<string>

How do we return the information provided by the headers? This is tricky in the above example because the string type is not extensible. I can think of a few options:

  1. We wrap every response in an object that provides both the headers and the 'payload'. For example Promise<Response<string>>, where Response has headers and (generic) payload properties
  2. For every response, for extensible result types (objects), we add a headers property with this information, and for non-extensible types (primitives), we wrap them in a similar fashion to (1).
  3. We follow (2), but only apply this logic for methods that return header properties.

I'm leaning towards (3) as it has zero impact on APIs that don't return headers, and keeps the return types as simple as possible.

Thoughts?

@gkocak-scottlogic
Copy link
Contributor

I agree choosing option (2) won't have many implications in the API but I don't find it structurally correct to expose headers to the Model classes (e.g. having the headers property for the Pet object sounds a bit odd.).

I also find the selective wrapping of (3) creates inconsistencies in the API for the end user. (e.g. one API endpoint returning Response<string> while one another string doesn't sound right to me and can be confusing.).

I would either go with option (1) or approach from another angle where we store the additional header information into the ApiClientX and expose it to the end-user via there (which itself sounds structurally imperfect though.).

@ColinEberhardt
Copy link
Collaborator

Thanks for the feedback @gkocak-scottlogic - you're right, none of the 3 options are great.

I like your alternative approach. Headers are often used in APIs for more general metadata, e.g. rate limits, paging etc. Perhaps having an API where you can request the most recently returned headers would make sense.

In this example, that could look like:

const user: string = await apiUser.loginUser("bob", "password");

// retrieve the cached headers from the most recent request
const callsPerHour = apiUser.headers["X-Rate-Limit"];

@gkocak-scottlogic
Copy link
Contributor

I think I like the way it looks in the example. It's definitely a solid option. We can go through that if everyone agrees on it.

@OiNutter
Copy link

OiNutter commented Mar 3, 2023

A few thoughts based on our experiences with swagger-typescript-api:

  • Having headers available definitely something I can see developers finding useful, think it's better for them to have access but not use it than to not be able to get at them. For example you may want to check the TTL on your token to know if it needs refreshing
  • swagger-typescript-api goes the wrapper route of having an HttpResponse class that returns the body as a data property, this works pretty well for us. We generally use a thin wrapper interface over the generated clients to handle setting up the client with the correct authorisation headers and the methods give a consistent interface even if endpoints change. We usually just return the data property from those so our consuming components just get the bits we care about but we can insert any token refresh type middleware in there as well if we want.
  • I'd be concerned about race conditions with the approach @ColinEberhardt's suggested of having a method to get the most recent requests headers. I think the wrapper class is less risky, you could limit the headers on that to the ones documented, or make it just include all valid headers.

@ColinEberhardt
Copy link
Collaborator

Thanks for the feedback @OiNutter that was very useful, based on this, and from having had a look at various other API generators, I've taken the route of creating a response object that hosts both the data and headers.

This feature has been added to all generators. Keeping this issue open, the generated client return headers, but don't apply any of the type information present within the schema.

@ColinEberhardt ColinEberhardt changed the title Add support for headers in the response Add support for typed headers in the response Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request open-api-spec
Projects
None yet
Development

No branches or pull requests

5 participants