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 an option to explicitly type SuccessResponse #387

Closed
WoH opened this issue Aug 7, 2019 · 10 comments
Closed

Add an option to explicitly type SuccessResponse #387

WoH opened this issue Aug 7, 2019 · 10 comments

Comments

@WoH
Copy link
Collaborator

WoH commented Aug 7, 2019

Usually, the SuccessResponse uses the return type of the annotated function to determine the type to be documented by swagger. However, there are cases when I want to return a type union A | B documented as SuccessResponse<A>() and Response<B> from a controller.

In these cases, the function body returns a type union of A | B to get full TS support.
In order to document this behavior using TSOA, it should be possible to override the default return type using the SuccessResponse decorator (i.e. @SuccessResponse<A>(...)).

Alternatives considered:

  • You can't use @Response<A>(200, ...) since the default behavior of TSOA is to overwrite that with the default SuccessResponse here.
  • Returning type A from the controller function and returning B as any feels bad

Syntax: @SuccessResponse<OverrideType>(...)

This does not address returning a type union within one response documented as oneOf by default.

Related PR, opened an issue for discussion.

@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 7, 2019

If you wouldn’t mind, could you please give a real world example of why the api would return different types?

I saw your example in the tests but it’s hard to understand why someone would want to do this.

Please provide the explanation in a non-technical / user-story-centric manner. In other words, pretend I know nothing about TypeScript or Swagger, but I just want to get some kind of business goal accomplished.

@WoH
Copy link
Collaborator Author

WoH commented Aug 9, 2019

One example:
As a user I'd want to know why my requests fail. As a developer, when I know I can't handle a request, I want to not just throw an error and exit but have a well defined and documented response through swagger (with example).
Writing typescript, I'd want to retrieveModel(...conditions): Model | ModelNotFound.
In swagger, both these things should be documented, the 200 with the model and the 404 ModelNotFound with specific reasons why the model was not found.

@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 9, 2019

Thank you so much for explaining your use case. Your goal of defining a union of response types should be handled through #393 (which we're taking submissions on. If you'd like to submit that help, we would love the contribution 😍). I describe that more in detail here: #369 (comment)

As for why I'm closing this issue, if a user of tsoa wants to "explicitly type the SuccessResponse" they should do that with the return type of the controller function. To do it any other way would prevent TypeScript from validating that the route is actually returning the type that we're communicating in the Swagger model.

@dgreene1 dgreene1 closed this as completed Aug 9, 2019
@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 9, 2019

Your original PR gave me an idea that could be closer to your original intention. So if you wanted to get swagger output that resembles this:

      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Model'
        # These two error responses have the same schema
        '404':
          description: Not Found
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ModelNotFound'

we could extend @Response to allow an explicit type. However, I'd have a few conditions before I would accept the PR:

  • The type T in @Response<T> would have to be one of the return types in the union that the route is returning. So we'd have to have validation that the function can in fact return the type. Note that since this depends on the route returning a union of types, we should probably wait until someone is willing to take on Seeking contributions to add anyOf and oneOf #393.
  • I don't think we should allow someone to modify the return type for success, so we should have some validation that no one has a decorator like this @Response<T>(200, 'Success')

So yea, I'm open to ideas on how to accomplish this. Ultimately @WoH, I want you and others to be able to accomplish your needs. But I have to make sure that it's accomplished in a way that keeps the library compatible with swagger and OpenAPI 3.0. So I'm going to leave this PR closed until #393 is finished. But we can keep chatting if you and others would like to continue to discuss the optimal solution. :)

@WoH
Copy link
Collaborator Author

WoH commented Aug 9, 2019

Thanks for your response.
Here is why the way I understand your approach might not work:
If #393 were to be added, that should probably document Model | ModelNotFound as swagger's anyOf with response code 200.

paths:
  responses:
    '200':
      description: Success
      content:
        application/json:
          schema:
            anyOf:
              - $ref: '#/components/schemas/Model'
              - $ref: '#/components/schemas/ModelNotFound'

I want to split both onto different http status codes.
Since this is done in code using this.setStatus(), I believe it will be hard to infer.
As you correctly identified, my swagger.yml should look like this:

paths:
  responses:
    '200':
      description: Success
      content:
        application/json:
          $ref: '#/components/schemas/Model'

    '404':
      description: Not Found
        content:
          application/json:
            schema:
            $ref: '#/components/schemas/ModelNotFound'

As for extending @Response, that is the already the current behavior.

I don't think we should allow someone to modify the return type for success, so we should have some validation that no one has a decorator like this @Response<T>(200, 'Success')

This is allowed, unchecked, but will be ignored, because the SuccessResponse will be added here.

I know this is not preferable but might be needed. The only other approach I could see is exposing a TsoaError to be extended by the developer that has a status code property that will be documented and used.

@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 9, 2019

Good point about the success response containing the error model. That’s not a good choice on my part. So maybe the solution is to use @Response<T> in combination with the error middleware approach defined in #382 (comment). This is making me realize we probably need something added to the readme.

this is allowed unchecked

Modifying the type for the success code will be prevented after #119 is fixed since it’s the best way to fix the duplicate code error in Swagger.

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented Apr 3, 2020

I'm kind of running into this problem at the moment, in that I want to return multiple types depending on the status code.
Forgive me if this is already solved but it's not easy to figure that out from the docs and searching issues didn't give me anything besides the middleware and catching errors.

But throwing an error isn't type checked and arguably exceptions aren't the right concept for statuses that might be pretty common.

It feels unsatisfying to set the type of the 200 status using the return type, but be unable to use the return type to set types for other statuses.
Additionally, if I can set the type for other errors, why can't I set it for 200?
At least in OpenAPI (where granted I'm not an expert so I could be wrong) it seems all error statuses are treated equally.

At any time I can use setStatus to set the status of the response and then return an object of some other type than agreed upon with @Response, right?

Why not make the status part of the return value?
Could there be a way of instead typing the route function such that it returns an object which represents a status code and an object of the model for that type?

Something like:

interface Response<S extends number, T> {
  status: S;
  content: T;
}
public async getCode(
    @Request() req: koa.Request,
    @Body() body: RequestBody
  ): Promise<Response<200, SuccessfulBody> | Response<400, ErrorReport> | Response<403, AuthorizationError>> {
  ..
}

It might potentially? be possible with some magic to deconstruct the return type into an OpenAPI spec for that route.
If that's not possible than at least it'd be cool to set the type for 200 and add a middleware that interprets the Response<..> | Response<..> ... value and sets body.

At the very least might it not make sense to include a version of the aforementioned middleware that understands a single error type, so at least the user can throw that type and have it be interpreted, without having to include a middleware (EDIT: from a github comment).

I'd be happy to contribute all or any of this by the way, if I knew it would be welcome.

Thanks for the work on the library, btw, was exactly what I was looking for and I love it so far.

@WoH
Copy link
Collaborator Author

WoH commented Apr 3, 2020

I'd like to head what you think about this: #617. I don't know koa very well, is there a similar option for that framework maybe?

@fgnass
Copy link
Contributor

fgnass commented May 13, 2020

Now that #393 is finished can we take another look at this issue? I really like @michaelbeaumont's suggestion of making the status code part of the controller's return type. But since that would be a breaking change I would equally welcome a typed @SuccessResponse<T> or the possibility to use @Response<T>(200) to overwrite the success type. Is there anything I can do to move this forward?

@WoH
Copy link
Collaborator Author

WoH commented May 13, 2020

Hey there. I believe we can even do better than that.
See #617, I assume this would cover your use case aswell.

Right now, the best way to imitate that is to return the SuccessResponse as the return type and throw/catch for alternative responses.

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

4 participants