Use code from SuccessResponse decorator as default status.

[devnev]

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you written unit tests?
• Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
• This PR is associated with an existing issue?

Closing issues

closes #723

If this is a new feature submission:

• Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

Although this seems to "fix" TSOA to have the actual response code match the spec when given through @SuccessResponse, it is also a breaking change, so I completely understand if a different solution would be preferred.

Test plan

Unit test shows the 202 value from the @SucessResponse on a controller that doesn't extend Controller. Also a test for cases where @SucessResponse has something other than a numeric status code.

[devnev]

A slightly more advanced approach would be to detect a Promise<TsoaSuccess<code, data, headers>> return type that would allow for custom headers too, with the response description coming from the @returns JSDoc tag. More complex to implement though, and possibly orthogonal to fixing the @SucessResponse handling.

[WoH]

This would also break the ability to use something like @SuccessResponse("2XX"). While I can see something like this behavior down the road, I currently can't merge this in the scope of anything but pretty fundamental breaking change.

[devnev]

This would also break the ability to use something like @SuccessResponse("2XX").

It doesn't, only numeric success response names are handled - the line status: name && /^\d+$/.test(name) ? parseInt(name, 10) : undefined ignores other cases.

[WoH]

This would also break the ability to use something like @SuccessResponse("2XX").

It doesn't, only numeric success response names are handled - the line status: name && /^\d+$/.test(name) ? parseInt(name, 10) : undefined ignores other cases.

Ah, good foresight. Missed that.

[devnev]

@WoH Is this still too much of a breaking change? If so, what about adding a configuration option to enable/disable this behaviour?

[WoH]

In order to merge this, we need a feature flag that we can deprecate, yes.

That does not mean the PR itself is fine, I'll have to take a look next weekend probably.

[devnev]

@WoH would you like me to go ahead with a feature flag in the config, or should I wait? If yes, any preferences on flag name?

[WoH]

@WoH would you like me to go ahead with a feature flag in the config, or should I wait? If yes, any preferences on flag name?

I don't really have a name in mind, my suggestion would be enforceResponseStatus.
But yes, I'd like to see this behind a feature flag before the next major.

[devnev]

@WoH feature flag added and tests refactored to verify behaviour with flag on vs off. I named the flag slightly differently as it doesn't really enforce anything, just changes the default.

[WoH]

PR overall looks very good. We could probably actually use decorators to tag the methods with the success response status code instead of parsing it, but your approach is more consistent with the current approach, so yours is probably preferable.

I think there is one assertion that I'm missing though: If you set the response status within the controller method, the success status should override that if I understand the original issue correctly (?), but in this PR, setStatus takes precedence. Either way, there are no assertions yet to make sure. Can you add a last test for that so the difference between this feature and before becomes more obvious?

[devnev]

I think there is one assertion that I'm missing though: If you set the response status within the controller method, the success status should override that if I understand the original issue correctly (?), but in this PR, setStatus takes precedence. Either way, there are no assertions yet to make sure. Can you add a last test for that so the difference between this feature and before becomes more obvious?

The original intent was for classes to be able to avoid the shared controller state, in which case there would be no setStatus and the precedence doesn't matter.

If I ignore setStatus entirely when the feature is enabled, I think that would that make it harder to migrate to use the feature? Enabling the feature would immediately change the behaviour of any method using setStatus to set a different status than what is in the @SuccessResponse. I'm not sure, it really could go either way...