Feat: Response header(Fix #770) #857

jackey8616 · 2020-12-01T09:52:29Z

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 #770 .

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

This RP introduce SuccessResponse and Response decorator with a generic type to define header struture.

SuccessResponse wasn't follow with any generic type, so in this PR, is not a big deal.
but Response is already provide a generic type T which specify the example structure,
usage of Response before this PR is @Response<ExampleClass>:
In some situation like need header class but not example class, the decorator define will be @Response<null, HeaderClass>.

Test plan

Test spec is coming out at path.[status code].headers field.

Add SuccessResponse & Response decorator's overloading. Add headers field in tsoa#Response interface. Add Header type in swagger for both OA2.0 & 3.0.

WoH · 2020-12-21T22:41:35Z

Can you extend the parsing to the third argument of @Res() response: TsoaResponse<Status, Body, Headers>. I added the argument for type checking and anticipation of a PR adding this.

E: See here and here.

Add statements to support headers field. Add controller for tests and test cases.

jackey8616 · 2020-12-26T15:48:30Z

@WoH I've done the support for TsoaResponse,
Please review. thank you.

tests/unit/swagger/schemaDetails3.spec.ts

tests/unit/swagger/schemaDetails.spec.ts

tests/fixtures/controllers/responseHeaderController.ts

packages/cli/src/swagger/specGenerator3.ts

packages/runtime/src/decorators/response.ts

packages/cli/src/swagger/specGenerator2.ts

packages/cli/src/swagger/specGenerator3.ts

packages/runtime/src/decorators/response.ts

Add missing common response header with header object instead of header class. Edit some HeaderClass's description more readable. Replace all exsitence examine with content examine in test cases.

Add optional field in all header class / object at test controllers. FIx incorrect statement with undefined result at schemaDetail#L197 & shcmeaDetail3#L444. Add field exsitence examine at OAI2.0 & required field in OAI3.0.

Add else statement to throw error with invalid header type. Add incorrect response header controller for test cases. Add test case in both OAI2.0 & 3.0.

Remove optional overrloading for SuccessResponse & Response type. Edit SuccessResponse & Response type's generic type more readable.

WoH · 2020-12-29T16:21:54Z

3 more general thoughts:

We may want to add type aliases in the future, currently, that will fail
This relates to both Use code from SuccessResponse decorator as default status. #850 and Support for response headers in generated OpenAPI spec #770 which initially suggested the Header docs API: Currently, this PR extends the @SuccessResponse decorator arguments in a descriptive fashion. This means, there's no guarantee that the response headers actually match the documentation. Use code from SuccessResponse decorator as default status. #850 changes that for the response status, I'd argue there's a lot of times, where the header could be set using @SuccessHeader({'Cache-Control', 'none'}). What do you think? Would also be interested in @devnev 's take here. While I don't think we should do that in this PR, if we anticipate that functionality, we should change the descriptive API to not add an Argument to @SuccessResponse and instead add @SuccessHeader. @jackey8616 was there a specific reason why there's a deviation from the API suggested in Support for response headers in generated OpenAPI spec #770?
I think we should type the header property of the Tsoa.Response more strictly, since it can only contain a refObject/nestedObjectLiteral. Then we can catch other types when getting the header type (here and here) and use the GenerateMetadataError early to catch issues and print the offending node.

WoH · 2020-12-30T15:01:57Z

@jackey8616 I'd love to add some commits that address some of the things I laid out myself if you don't mind. Would you like to give me commit access to the PR or should I send you diffs?

jackey8616 · 2020-12-30T15:03:57Z

@WoH I turned it on!

WoH · 2020-12-30T17:36:39Z

Remaining TODOs:

OAS3: explode and style, OAS2: collectionFormat (?)
Clean up tests: Both OAS2 and OAS3 specs should test all the new features, there are discrepancies rn
Discuss @SuccessResponse<Header>() vs. SuccessHeader<Header>() (?)
Support aliases, unions, intersections (?)

This ensures we never try to generate a spec based on invalid metadata. This enables us to point out the issue more precisely and link the offending node.

jackey8616 · 2020-12-31T15:30:17Z

@WoH

@jackey8616 was there a specific reason why there's a deviation from the API suggested in #770?

IMHO, header must be used along with a response in swagger, thus, they should be good if we let it be optional parameter at Response, SuccessResponse or TsoaResponse.

Since SuccessResponse and SuccessHeader decorator is implicitly contains a 2xx status.
When programmer not using default status as it decorator gives, these two decorator may let the usage logic become fuzzy.

WoH · 2020-12-31T16:11:01Z

Since SuccessResponse and SuccessHeader decorator is implicitly contains a 2xx status.
When programmer not using default status as it decorator gives, these two decorator may let the usage logic become fuzzy.

I don't think I can follow. Can you give me an example?

jackey8616 · 2021-01-01T15:08:50Z

Since SuccessResponse and SuccessHeader decorator is implicitly contains a 2xx status.
When programmer not using default status as it decorator gives, these two decorator may let the usage logic become fuzzy.

I don't think I can follow. Can you give me an example?

If we want to let success status code to be 204,
we need to set both in SuccessResponse & SuccessHeader.
and it seems a little bit bother to set 204 code twice.

@SuccessResponse(204, 'some descriptions doesn\'t want be omit.')
@SuccessHeader<HeaderClass>(204, {
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

WoH · 2021-01-01T19:13:16Z

Since SuccessResponse and SuccessHeader decorator is implicitly contains a 2xx status.
When programmer not using default status as it decorator gives, these two decorator may let the usage logic become fuzzy.

I don't think I can follow. Can you give me an example?

If we want to let success status code to be 204,
we need to set both in SuccessResponse & SuccessHeader.
and it seems a little bit bother to set 204 code twice.
@SuccessResponse(204, 'some descriptions doesn\'t want be omit.')
@SuccessHeader<HeaderClass>(204, {
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

But do we? What's the problem with

@SuccessResponse(204, 'some descriptions doesn\'t want be omit.')
@SuccessHeader<Header>({
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

jackey8616 · 2021-01-02T08:59:29Z

But do we? What's the problem with

@SuccessResponse(204, 'some descriptions doesn\'t want be omit.')
@SuccessHeader<Header>({
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

I think its no problem expect it runs different logic compares with SuccessResponse, which requires pre-check exists response defined by it, further more, it also effect by existence of SuccessResponse decorator, which may cause complex logic of SuccessHeader IMHO.

I've surveyed #723, and I don't see any actual reason to introduce SuccessHeader.

WoH · 2021-01-03T06:21:07Z

I think its no problem expect it runs different logic compares with SuccessResponse, which requires pre-check exists response defined by it, further more, it also effect by existence of SuccessResponse decorator, which may cause complex logic of SuccessHeader IMHO.

I've surveyed #723, and I don't see any actual reason to introduce SuccessHeader.

I hope I get where you're coming from now. For this PR, the goal is only to document the headers on the default / success response. Admittedly, you can do that both with the API you implemented and @SuccessHeaders<Header>()>.
However, pulling out the header portion into it's own decorator allows us to extend the usage more easily in the future. That requires 2 additional steps, one of which will be already done in #850, because it allows tsoa to know what the default/success status code is. Based on this, we are able to infer when a default response is returned and are able to append the headers as provided in an argument to @SuccessHeaders({}).

I'm looking specifically at this:

@SuccessHeader<Header>({
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

Here, Header is the type used for documentation, the actual argument will be set as headers automatically.
If we use the @SuccessResponse, my worry was that this would be something like:

@SuccessResponse<Header>(undefined, undefined, {
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

However, we can obviously do type/arg checks so we can do:

@SuccessResponse<Header>({
  linkA: 'http://google.com',
})
public async handler(): Promise<void> {
  return;
}

So it seems to come down to preference. For my personal taste setting the headers as a separate annotation feels more intuitive, but if I can't really point a finger as to why, your approach is probably equally fine.

devnev · 2021-01-07T23:05:52Z

Should the solution for success headers generalise to headers with dynamic values?

WoH · 2021-01-09T17:45:43Z

@jackey8616 I rebased and this lgtm now. Can you take a look and confirm?

WoH · 2021-01-10T18:55:25Z

Should the solution for success headers generalise to headers with dynamic values?

You'd still have to set them using setHeaders, but you'd be able to declare them (as optional properties on the Header interface).

jackey8616 added 5 commits December 1, 2020 16:18

chore: Add type in specGenerator2#buildOperation

6e53911

feat: Add & modify header relative variable types

e226073

Add SuccessResponse & Response decorator's overloading. Add headers field in tsoa#Response interface. Add Header type in swagger for both OA2.0 & 3.0.

test: Add test cases

ce68675

feat: Add missing common response implementation

a751ac7

test: Add common response test cases

da81ba3

feat: Add headers support for TsoaResponse

27813b4

Add statements to support headers field. Add controller for tests and test cases.