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

Mechanism to avoid controller state and duplicating success annotations #723

Closed
2 of 4 tasks
devnev opened this issue Jun 5, 2020 · 3 comments · Fixed by #850
Closed
2 of 4 tasks

Mechanism to avoid controller state and duplicating success annotations #723

devnev opened this issue Jun 5, 2020 · 3 comments · Fixed by #850

Comments

@devnev
Copy link
Contributor

devnev commented Jun 5, 2020

Sorting

  • I'm submitting a ...

    • bug report
    • feature request
    • support request
  • I confirm that I

    • used the search to make sure that a similar issue hasn't already been submit

Expected Behavior

I can write handlers without storing request state on the controller, i.e. without the .getStatus/.setStatus methods, and without duplicating information in annotations.

Current Behavior

If I use @SuccessResponse to describe the result of a handler, I still need to create a status state on the controller and setStatus/getStatus/getHeaders methods to have responses actually return the correct status code. To avoid having to add stateful (and possibly concurrency-unsafe) methods to a controller, I've instead opted to use the @Res decorator for all responses, including the final return.

e.g.

@Route('foo')
class MyController {
  @Get('bar')
  bar(
    @Res() created: TsoaResponse<201, {}>,
  ): Promise<void> {
    return created(201, {})
  }
}

However, doing this still produces a default success response in the spec:

openapi: 3.0.0
paths:
    /foo/bar:
        get:
            operationId: Bar
            responses:
                '201':
                    description: ""
                    content:
                        application/json:
                            schema:
                                properties: {}
                                type: object
                '204':
                    description: 'No content'

To eliminate the extra response, I need to duplicate the success case in the annotations:

@Route('foo')
class MyController {
  @Get('bar')
  @SuccessResponse(201, 'created')
  bar(@Res() created: TsoaResponse<201, {}>): Promise<void> {
    return created(201, {});
  }
}

Possible Solution

If the default response code in the generated template was derived from the success response annotation, I wouldn't need the @Res callback, and regular return would work fine. I don't know what happens if there's multiple SuccessResponse annotations though - maybe this could cause tsoa to error out?

Another alternative would be to avoid the default response being generated, although that raises the question of what the generated code should do when no response callback was called. Maybe generate an internal error?

Context (Environment)

Version of the library: 3.1.0
Version of NodeJS: 10.19

  • Confirm you were using yarn not npm: [x]
@WoH
Copy link
Collaborator

WoH commented Jun 6, 2020

I think the idea is good, but I'd rather approach it in a different way. Not using the default type annotation for the default response woud be a major change and in fact one that would hurt a lot of existing users.
Using @Res for the default/success response is imho not the best option here. I understand the motivation, but it was explicitly introduced to handle typechecked alternative responses (vs. default/success responses) for that reason.

Instead, I'd suggest we promote the @SuccessResponse decorator to a functional one instead of a purely descriptive decorator and Introduce a @SuccessHeaders decorator that acts in a similar fashion.

Would you generally be interested in implementing this?

@WoH WoH added the help wanted label Jun 6, 2020
@devnev
Copy link
Contributor Author

devnev commented Jun 11, 2020

Would promoting @SuccessResponse be a backwards-incompatible change? I think it might cause the actual response codes of existing APIs to change from the default to whatever is in the @SuccessResponse annotation.

@WoH
Copy link
Collaborator

WoH commented Jun 16, 2020

Would probably be a breaking change, yes.
I could see an argument for a bug fix, but that's really far fetched.
Probably the best approach I can see right now is to work on deprecating Controller (especially making setStatus and setHeaders a noop and instead promote SuccessResponse and add SuccessHeaders. That would get us to a point where I'd feel comfortable to bump a major with (minor) breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants