Allow easy differentiation by status codes

[happenslol]

Description

It should somehow be possible to figure out the http status code that was returned (or at least differentiate between 2xx/4xx/5xx) for responses.

It's recommended to use the default response to group multiple error codes that have the same structure. Even though default responses are completely broken at the moment (see #490), in the previous version that didn't show this error I would get something like Response | Error, which is hard to work with if both are just objects with varying keys.

There's multiple ways to go about this I think, depending on the framework used (fetch, axios, etc.). Most comfortable for typescript would probably be to have a discriminated union with the status code as the discriminator. Otherwise, 4xx/5xx codes throwing would be fine too, but then it should be possible to configure the code generation to assume default responses only on 4xx/5xx codes.

[mrlubos]

Agree with this. Error codes and responses are a long-standing issue, so unlikely to be fixed soon. Do you have an example of how you're looking to use error responses and how would the ideal solution look?

[Hookyns]

I miss this feature!

I would expect something like this: Stackblitz demo

It's a fork of your stackblitz demo from docs. I removed few files so it's a little bit more clean.

What I changed:

• I added file /Proposal.ts with TS type (ApiResponsesFor<>) that creates discriminated union from $OpenApiTs endpoint description.
• I changed return type of the addPet method in the PetService so it use the new ApiResponsesFor<> type.
• I added file /App.ts with example usage.

That new type just transforms this:

export type $OpenApiTs = {
    '/pet': {
        post: {
            req: {
                requestBody: Pet;
            };
            res: {
                200: Pet;
                400: ApiValidationProblemDetails;
                405: unknown;
                500: ApiProblemDetails;
            };
        };
    };
};

to this:

// ApiResponsesFor<$OpenApiTs['/pet']['post']>
type TypeCreatedByApiResponsesFor = 
    | {
        status: 200;
        response: Pet;
    }
    | {
        status: 400;
        error: ApiValidationProblemDetails;
    }
    | {
        status: 405;
        error: unknown;
    }
    | {
        status: 500;
        error: ApiProblemDetails;
    };

FYI: @mrlubos

[mrlubos]

This looks really nice and seems like the direction I'd like to go in @Hookyns! The problem with the current version is that to add this feature as described in this issue, you'd need to introduce some breaking changes. Since that seems unavoidable, I'd much rather address this in the upcoming clients than have people go through multiple breaking upgrades before we get to v1

[Hookyns]

Yeah, it's definitely a breaking change, but it may be under some config value for example?! So if you enable this new option the service will return this type instead.

---

About "clients"... If I understand correctly you are working on the "clients" and it's gonna be a new major version with breaking changes. It's still WIP and it's not published yet. Am I right? I'm sorry I'm a new guy so I have no clue about the current status. 😄 What's an ETA? Weeks, months, year(s)?

[mrlubos]

@Hookyns hopefully not years :) You're correct about the rest. Would you want to test drive the clients before they're fully ready? Which client are you using, Fetch API?

[Hookyns]

@mrlubos if it's somehow partly implemented and at least one client is working, yeah, why not. 🙂

[happenslol]

Sorry for not responding earlier - what @Hookyns posted is exactly what I envisioned. I'm not sure if I'd include 4xx/5xx status codes, since that would make the happy path very wordy. In my opinion, ideally there would be a union where I can match on status (see the example above) if there are multiple 2xx responses, and just the response itself for a single response. Similarly for the error type.

[mrlubos]

Update: this will be addressed as part of clients. They will return both data and error keys, in this issue we're interested only in error. If multiple error types exist, this will be a union of their types. You could then narrow errors with type guards and handle as you see fit. I don't think status should be included in the type union since technically any status could return another union, in which case you'd have to use type guards anyway. Thoughts?

[happenslol]

Update: this will be addressed as part of clients. They will return both data and error keys, in this issue we're interested only in error. If multiple error types exist, this will be a union of their types. You could then narrow errors with type guards and handle as you see fit. I don't think status should be included in the type union since technically any status could return another union, in which case you'd have to use type guards anyway. Thoughts?

I think status is really important, both for errors and success responses, as long as the schema defines multiple possible status codes. It's possible that either a 403 or a 500 is returned without any response body by the server, and without a status it would not be possible to differentiate them.

[Hookyns]

Update: this will be addressed as part of clients. They will return both data and error keys, in this issue we're interested only in error. If multiple error types exist, this will be a union of their types. You could then narrow errors with type guards and handle as you see fit. I don't think status should be included in the type union since technically any status could return another union, in which case you'd have to use type guards anyway. Thoughts?

Not a good idea IMHO. I think APIs are status-oriented, and so is OpenApi. The https://github.com/hey-api/openapi-ts/issues/467 is also about status.

As @happenslol also pointed out, you don't even have to specify schema, but you have to specify status.

The fact you can use oneOf, allOf, anyOf does not change anything on the fact that it is always specified per status. If you use eg. oneOf then you'll probably have to use type guards but only in this case and it's not that usual; you will have a bad developer experience in 95 % of cases because of the rest that is not even affected (you'll have to use type guards anyway).

[Hookyns]

@happenslol

Sorry for not responding earlier - what @Hookyns posted is exactly what I envisioned. I'm not sure if I'd include 4xx/5xx status codes, since that would make the happy path very wordy. In my opinion, ideally there would be a union where I can match on status (see the example above) if there are multiple 2xx responses, and just the response itself for a single response. Similarly for the error type.

You can start by positive statuses. 😄

if (result.status === 200) {
    // 🎉 
} else {
    // Handle general error or "switch" over specific status codes
}

But I would use IsOk or assertOk type-guards. I created fork of that Stackblitz example.

export function isOk<TAll extends { status: number }>(
    result: TAll
): result is PickOk<TAll> {
    return result.status >= 200 && result.status < 300;
}

export function assertOk<TAll extends { status: number }>(
    result: TAll
): asserts result is PickOk<TAll> {
    if (result.status < 200 || result.status >= 300) {
        throw new ApiResultError(
            result as unknown as ErrorApiResponse<number, unknown>
        );
    }
}

isOk()

const petResult = await PetService.addPet({
    requestBody: {
        name: 'Bob',
        photoUrls: [],
    },
});

if (isOk(petResult)) {
    // Narrowed; only status 200 with response: Pet left
    console.log(petResult.response satisfies Pet);
} else {
    /* Handle error in general or go through all the statuses */
}

assertOk()

const petResult = await PetService.addPet({
    requestBody: {
        name: 'Bob',
        photoUrls: [],
    },
});

assertOk(petResult);

// Narrowed; only status 200 with response: Pet left
console.log(petResult.response satisfies Pet);

The PickOk type that extracts positive results from the discriminated union.

type OkStatus<TStatus extends number> = `${TStatus}` extends `2${infer _}`
    ? TStatus
    : never;

export type PickOk<TUnion extends { status: number }> = TUnion extends {
    status: infer TStatus;
}
    ? TStatus extends OkStatus<TStatus & number>
        ? TUnion
        : never
    : never;

[mrlubos]

@happenslol Let me clarify that you'll also have access to the full response object which will resolve https://github.com/hey-api/openapi-ts/issues/477

@Hookyns what if the endpoint returns also 204 which is success but there's no data? What would the code look like?

[Hookyns]

@Hookyns what if the endpoint returns also 204 which is success but there's no data? What would the code look like?

@mrlubos It depends on the OpenApi spec and the use-case...

API "schema" (idk how to call it):

export type $OpenApiTs = {
    '/pet': {
        post: {
            req: {
                requestBody: Pet;
            };
            res: {
                200: Pet;
                204: null;
                400: ApiValidationProblemDetails;
                500: ApiProblemDetails;
            };
        };
    };
};

Service:

export function addPet(
    data: $OpenApiTs['/pet']['post']['req']
): CancelablePromise<ApiResponsesFor<$OpenApiTs['/pet']['post']>>;

Where ApiResponsesFor<$OpenApiTs['/pet']['post']> is just an alias for:

type PostPetResult = ApiResponse<200, Pet> | ApiResponse<204, null> | ApiResponse<400, ApiValidationProblemDetails> | ApiResponse<500, ApiProblemDetails>;

Resolved/expanded type looks like this:

type PostPetResult = 
    | {
        status: 200;
        response: Pet;
        raw: Response; // I added this bcs of #477
    }
    | {
        status: 204;
        response: null;
        raw: Response;
    }
    | {
        status: 400;
        error: ApiValidationProblemDetails;
        raw: Response;
    }
    | {
        status: 500;
        error: ApiProblemDetails;
        raw: Response;
    };

Usage

const data = {
    name: 'Bob',
    photoUrls: [],
};
const petResult = await addPet({
    requestBody: data,
});

if (isOk(petResult)) {
    /*
    Narrowed to:
    {
        status: 200;
        response: Pet;
        raw: Response;
    }
    | {
        status: 204;
        response: null;
        raw: Response;
    }

    which is also this from some point of view
    {
        status: 200 | 204;
        response: Pet | null;
        raw: Response;
    }
    */
    // ... do something,.. IDK what bcs I would never do POST endpoint that returns both 200 and 204
    return petResult.status === 200 ? petResult.response : data; // maybe something like this? I rly don't know what should be the use-case of such endpoint
} else {
    /* Handle error in general or go through all the statuses */
}

assert variant

const data = {
    name: 'Bob',
    photoUrls: [],
};
const petResult = await addPet({
    requestBody: data,
});

assertOk(petResult);

/*
Narrowed to:
{
    status: 200;
    response: Pet;
    raw: Response;
}
| {
    status: 204;
    response: null;
    raw: Response;
}
*/

// ... do something,..

[mrlubos]

Hey all, check out the new Fetch API client and see if you can live with union error types? Here's the demo, but Petstore doesn't have good errors to see it in action

[lsdch]

That is great news, thanks for releasing this feature! :pray:

Would you consider modifying the RequestResult type so that we don't have to check both error and data for undefined (or use type assertions) ? I'm thinking something like

type RequestResult<Data = unknown, Error = unknown> = Promise<{
   error: undefined;
    data: Data;
    request: Request;
    response: Response;
} | {
    error: Error;
    data: undefined;
    request: Request;
    response: Response;
}>;

so that :

async function handle(req: RequestResult<string, string>) {
  const { error, data } = await req
  if (error === undefined) {
    // data is `string`
  } else {
    // data is undefined
  }
}

---

I also wonder what is the purpose of the reject clause for request promises with the new API. Before this release, it was used to handle errors responses. Is there any point to keep writing .catch() callbacks now or should we ignore them altogether ?

[mrlubos]

I'll have a look at those types. For reject, I'd need to have an example to play around with. I'm still considering maybe making it optional for errors to throw?

[lsdch]

I don't have a specific use-case in mind for using reject, just wanted to make sure it has no intended purpose in the new API. Seems like it is actually better to avoid it altogether, since the reject callback can not be properly typed.

[lsdch]

Gave some more thoughts on improving type safety of request results. Here is a proposal that fulfills the use-case described by @Hookyns, and correctly infers response body type depending on HTTP status

type Content<S extends number, D = unknown, E = unknown> = {
  error?: E,
  data?: D
  response: Response & { status: S }
  status: S // Cannot use `response.status` as discriminator due to typescript limitation
}
type SuccessContent<S extends number, D> = Content<S, D, undefined> & { data: D };
type ErrorContent<S extends number, E> = Content<S, undefined, E> & { error: E };

type RequestResult<
  Success extends SuccessContent<number, unknown>,
  Error extends ErrorContent<number, unknown>,
> = Promise<(Success | Error) & { request: Request }>;

Type names could be better, but you get the idea.

Toy example :

async function handle(req: RequestResult<
  SuccessContent<200, string> | SuccessContent<201, number> | SuccessContent<204, undefined>,
  ErrorContent<500, string> | ErrorContent<422, Record<string, string>>
>) {
  const { error, data, status, response } = await req
  if (error === undefined) {
    switch (status) {
      case 200:
        response.status // inferred as: 200
        data // inferred as: string
        break;
      case 201:
        data // inferred as: number

        break
      default:
        data // inferred as: undefined
        break;
    }
  } else {
    data // inferred as: undefined
    if (status === 422)
      error // inferred as: Record<string, string>
    else
      error // inferred as: string
  }
}

This also works as expected when using a single switch statements over status to handle all possible cases (results or errors)