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

Method with several status codes generates only one return #31

Open
psamusev opened this issue Nov 12, 2021 · 7 comments
Open

Method with several status codes generates only one return #31

psamusev opened this issue Nov 12, 2021 · 7 comments

Comments

@psamusev
Copy link

psamusev commented Nov 12, 2021

I have method that describe DELETE operation.
Responses array of swagger may look like

      responses:
        '200':
          description: Removed response with body
          content:
            application/json:
              schema:
                type: object
        '204':
          description: Removed response
        '404':
          description: Time record is not exist
          content:
            application/json:
              schema:
                type: object

When I generates it with @openapi-generator-plus/[email protected], I have only one return and the rest are throws

						if (mimeType === 'application/json') {
							return response.json() as any;
						}
						throw response;
					}
					if (response.status === 204) {
						throw response;
					}
					if (response.status === 404) {
						if (mimeType === 'application/json') {
							throw response;
						}
						throw response;
					}

Expected behavior: Following the standart of status code, as for me, only 500th and 400th error codes should throw response object - the res should just return it

@karlvr
Copy link
Owner

karlvr commented Nov 15, 2021

@psamusev thanks for this issue. You're quite right. Currently the only way the client code knows how to return an alternative response is to throw it.

A better approach would be to have the return type be a union type, e.g. A | B. That's a reasonable solution in TypeScript, although sometimes it might be difficult to tell what the type is safely. An alternative would be to incorporate the status code into the response, like { status: 200; body: A } | { status: 204; body: B } in these cases, which would enable the caller to know exactly what happened at the expense of the API status codes leaking into the client code, which is a bit gross.

We also need the same solution in Java... which I think means we need a wrapper class that can contain multiple alternative types.

Then we leave 4XX and 5XX to be throws.

I'm interested in others thoughts about the best way to handle these multiple responses.

@psamusev
Copy link
Author

psamusev commented Nov 15, 2021

@karlvr I'm now thinking in a different way. What if we return/throw only known statuses (i.e. if spec contains 200 and 404) we add a handler for 200 and throw for 400 and for the rest we just return a result(instead of throwing) it in some generic form. And then you indeed can have union type of known statues and some generic type with status, as you described and body or even whole response object.

I.e. for example that I've described the return type can look like

ReturnTypeOf200 | { status: number; res: Response}

So the rule can be following

  1. Return/throw results for responses that are in spec. (for 4xx, 5xx throw)
  2. For the rest always return generic type. (even it's 4xx, 5xx). The user then decide what to do with unknown statues

@psamusev
Copy link
Author

Hi, @karlvr, any progress on this?

@karlvr
Copy link
Owner

karlvr commented Dec 3, 2021

@psamusev yes, this approach is what I’ve used in https://github.com/karlvr/openapi-generator-plus-express-passport/blob/main/templates/apiTypes.hbs. It works nicely with TypeScript and good type definitions. I’ve started a wiki page with my wish list for api clients, based largely around the work on the Swift client. Have a read of the wiki to see what I have in mind.

I’d like to achieve all of these things with the typescript client API and I don’t expect it to be too hard, but it will be a major change. I think I’ll prepare a 1.0 release of everything before doing it, and then make a v2, or a new generator package.

karlvr added a commit that referenced this issue Jan 24, 2022
@karlvr
Copy link
Owner

karlvr commented Jan 24, 2022

@psamusev I've started a @openapi-generator-plus/typescript-fetch-client-generator2 project that has strongly-typed responses along the lines above. It's a work in progress, but if you're keen to check it out that would be great.

Responses from the API will now look like:

{
  status: 200,
  contentType: "...",
  body: {},
  headers: undefined
}

Now it only throws if there is a network issue or an unrecognised response... basically situations that represent runtime errors that are probably not recoverable except to retry.

@karlvr
Copy link
Owner

karlvr commented May 7, 2022

@psamusev just a bump to see if you've had a chance to check out @openapi-generator-plus/typescript-fetch-client-generator2?

@psamusev
Copy link
Author

Hi, @karlvr . No, I didn't test it yet. We've done the product where the usage of your library was required and currently it's just in maintenance mode only.

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

No branches or pull requests

2 participants