[Typescript] Add Headers to Request #10406

ksvirkou-hubspot · 2021-09-16T08:04:26Z

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

ksvirkou-hubspot · 2021-09-16T08:49:25Z

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov

amakhrov · 2021-09-22T18:17:38Z

modules/openapi-generator/src/main/resources/typescript/api/api.mustache

+        const requestContext = _config.baseServer.makeRequestContext(
+            localVarPath,
+            HttpMethod.{{httpMethod}},
+            _config.baseServer.getHeaders()


looks like you always pass baseServer.headers to the baseServer.makeRequestContext call.
looks redundant - why don't baseServer.makeRequestContext does it internally - automatically?

amakhrov

@ksvirkou-hubspot thanks for the proposal! Could you provide an example of how you would use the new api to make a request with some custom headers?

ksvirkou-hubspot · 2021-09-24T11:44:03Z

@ksvirkou-hubspot thanks for the proposal! Could you provide an example of how you would use the new api to make a request with some custom headers?

    const configuration = createConfiguration({..})

    configuration.baseServer.setHeaderParam('User-Agent', 'test')

    let testApi = new TestApi(configuration)
    let response = TestApi.method()

@amakhrov

amakhrov · 2021-09-24T16:29:58Z

let testApi = new TestApi(configuration)
let response = TestApi.method()

Ah, I see. So the use case is setting some headers for a group of requests (maybe even across the whole app) - rather overriding headers for a specific single request you make.

amakhrov · 2021-09-24T17:26:02Z

modules/openapi-generator/src/main/resources/typescript/http/servers.mustache

        return this.variableConfiguration
    }

+    public setHeaders(headers: Headers): void {


nit: Any thoughts about changing the name?
getHeaders return all headers - quite expected
But setHeaders does not set all headers, instead it's an additive operation merging new headers with existing ones

Consequently, it's not possible to remove a header previously added to the server configuration.

I mean - it resembles the existing setVariables method, true. Not sure if that interface was well thought through, though (e.g. getConfiguration instead of getVariables indicates otherwise :) )

You are right
I think It is better to change the function
@amakhrov

public setHeaders(headers: Headers): void { this.headers = headers; }

What do you think?

or we can rename it to mergeHeaders or addHeaders

addHeaders sounds reasonable!

Done
@amakhrov

ksvirkou-hubspot · 2021-09-27T08:31:30Z

let testApi = new TestApi(configuration)
let response = TestApi.method()
Ah, I see. So the use case is setting some headers for a group of requests (maybe even across the whole app) - rather overriding headers for a specific single request you make.

yes it is necessary for us

amakhrov

LGTM

macjohnny · 2021-10-04T16:49:05Z

@ksvirkou-hubspot can you please fix the ci failure?

ksvirkou-hubspot · 2021-10-05T07:17:58Z

@ksvirkou-hubspot can you please fix the ci failure?

I think I can't fix it (It isn't the first PR to Open API Generator and every time I see this errors)
But I ll merge master to this branch (Maybe this will help)

merge master

ksvirkou-hubspot · 2021-10-05T08:37:47Z

@ksvirkou-hubspot can you please fix the ci failure?

Done
@amakhrov

macjohnny · 2021-10-05T08:48:38Z

@ksvirkou-hubspot please take a look at the circle CI failure:
https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/13101/workflows/7896a7d0-be2c-4691-80da-020d58867b06/jobs/26742

1) PetApi
       uploadFile:
     Error: HTTP-Code: 415
Message: Unknown API Status Code!
Body: "{\"code\":415,\"type\":\"unknown\"}"
      at new ApiException (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/exception.ts:12:9)
      at PetApiResponseProcessor.<anonymous> (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/PetApi.ts:625:15)
      at step (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:46:23)
      at Object.next (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:27:53)
      at fulfilled (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:18:58)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

the current master at f5e8f54 seems to build correctly: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/13100/workflows/a8cc5b7e-e4b4-48b6-ac4a-13288c3ac369/jobs/26741

bodograumann · 2021-10-05T10:59:28Z

Wouldn‘t it make more sense to write a Middleware to set these headers?

ksvirkou-hubspot · 2021-10-05T12:15:04Z

Wouldn‘t it make more sense to write a Middleware to set these headers?

Maybe
but when I checked code of this generator (typescript) I decided to do it by this way because it does not require rewriting of existing code
@bodograumann

ksvirkou-hubspot · 2021-10-06T16:09:52Z

@ksvirkou-hubspot please take a look at the circle CI failure: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/13101/workflows/7896a7d0-be2c-4691-80da-020d58867b06/jobs/26742

1) PetApi
       uploadFile:
     Error: HTTP-Code: 415
Message: Unknown API Status Code!
Body: "{\"code\":415,\"type\":\"unknown\"}"
      at new ApiException (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/exception.ts:12:9)
      at PetApiResponseProcessor.<anonymous> (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/apis/PetApi.ts:625:15)
      at step (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:46:23)
      at Object.next (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:27:53)
      at fulfilled (/home/circleci/OpenAPITools/openapi-generator/samples/openapi3/client/petstore/typescript/builds/default/dist/apis/PetApi.js:18:58)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

the current master at f5e8f54 seems to build correctly: https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/13100/workflows/a8cc5b7e-e4b4-48b6-ac4a-13288c3ac369/jobs/26741

I've just run this test on my local machine and It works correctly
Could you re-run tests?

@macjohnny

bodograumann · 2021-10-07T07:51:32Z

I ran the tests locally and found the same 415 error on uploadFile. This error does not occur on master.
Make sure you are following the correct steps: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests

Not sure why it is happening, but Content-Type is set after your change as application/json in the fetch call for uploadFile, where previously it was not set explicitely, but automatically determined by fetch. (Cf. https://github.com/ksvirkou-hubspot/openapi-generator/blob/feature/typescriptAddHeaders/samples/openapi3/client/petstore/typescript/builds/default/http/isomorphic-fetch.ts#L14)

Furthermore you should add some tests of your own to (at least) document the new feature.

atanasiuk-hubspot · 2021-10-26T09:51:01Z

Hi @bodograumann, I'm trying to repro failing tests with ./run-in-docker.sh mvn integration-test, but it works well for me. Am I missing something?

bodograumann · 2021-10-26T13:14:46Z

According to the documentation you need to do:

cd samples/openapi3/client/petstore/typescript/tests/default
mvn integration-test

I don’t know why these tests are not automatically executed when running mvn integration-test on the top-level.
Can you help here @macjohnny ?

merge master

ksvirkou-hubspot · 2021-11-26T13:43:03Z

Hey everyone
I ve fix all tests
could you merge it?
@amakhrov @macjohnny @bodograumann

bodograumann · 2021-11-26T13:57:11Z

Works now afaics. Still think this should be done in a Middleware instead though.

ksvirkou-hubspot · 2021-11-26T14:13:25Z

Works now afaics. Still think this should be done in a Middleware instead though.

I didn’t create a new construction
I only updated existing code base
I my opinion it is no need to make something new because it requires rewriting of existing code base
and more over this design was approved earlier

bodograumann · 2021-11-26T14:19:21Z

I didn’t want to start a new discussion.
Not sure you know about middlewares though. They are an already existing mechanism in the generated code.
You can define a middleware in your own code and register it when instantiating the api client.
It can inspect the request and add headers before sending easily.

ksvirkou-hubspot · 2021-11-26T14:29:05Z

ok
Could you merge it?
or if I have to fix something write me please
@macjohnny

macjohnny · 2021-11-28T09:11:57Z

@TiFu what is your opinion here?

ksvirkou-hubspot · 2021-12-10T13:35:56Z

Any Updates?
@macjohnny @TiFu

ksvirkou-hubspot · 2021-12-23T21:22:34Z

Any Updates?
@macjohnny @TiFu @amakhrov @bodograumann

macjohnny · 2021-12-24T15:57:48Z

Sorry for the long delay.
One question though: could this also be achieved by adding an approriate middleware, which could be done with the currently generated code?

ksvirkou-hubspot · 2022-01-03T12:26:01Z

Yes we can do it
but Could you give me an example (where it is implemented)?
CC @macjohnny @TiFu @amakhrov @bodograumann

amakhrov · 2022-01-04T05:33:56Z

@ksvirkou-hubspot I guess it could be something like this (using your original code snippet as the basis):

const configuration = createConfiguration({
  middleware: [{
    pre: context => {
      context.setHeaderParam('User-Agent', 'test');
      return of(context);
    }
  }],
   ... other config params
})

let testApi = new TestApi(configuration)
let response = TestApi.method()

TiFu · 2022-01-15T10:46:24Z

Sorry for the delay!

Given that this can easily be implemented in a Middleware (as demonstrated by @amakhrov), I would suggest to not merge this PR. I think this use case is a bit of a poster child for why we added the Middleware option.

macjohnny · 2022-01-15T15:33:14Z

Thanks @TiFu
Will close this PR then.
@ksvirkou-hubspot would you like to add @amakhrov s example to the readme?

ksvirkou-hubspot added 2 commits September 15, 2021 17:58

add headers

29f1e49

add samples

5f26bd5

amakhrov reviewed Sep 22, 2021

View reviewed changes

update function make request

01410ba

generate samples

ff5de88

amakhrov reviewed Sep 24, 2021

View reviewed changes

ksvirkou-hubspot added 2 commits October 1, 2021 15:05

rename setHeaders to addHeaders

f0e3247

generete samples

bfb51d1

amakhrov approved these changes Oct 4, 2021

View reviewed changes

Merge pull request #1 from OpenAPITools/master

fe1bcd8

merge master

macjohnny added the Client: TypeScript label Oct 5, 2021

macjohnny added this to the 5.3.0 milestone Oct 5, 2021

wing328 modified the milestones: 5.3.0, 5.3.1 Oct 25, 2021

Merge pull request #3 from OpenAPITools/master

1f0c1d7

merge master

ksvirkou-hubspot and others added 6 commits November 9, 2021 13:34

Merge pull request #4 from OpenAPITools/master

90bb539

merge master

clone headers for RequestContext

e29ae3d

regenerate tests

cc34f89

Merge pull request #5 from OpenAPITools/master

4013b16

merge master

Merge pull request #6 from OpenAPITools/master

0b64f3b

merge master

fix abstract class

935fa84

wing328 modified the milestones: 5.3.1, 5.4.0 Dec 29, 2021

macjohnny closed this Jan 15, 2022

Uh oh!

[Typescript] Add Headers to Request #10406

[Typescript] Add Headers to Request #10406

Uh oh!

Conversation

ksvirkou-hubspot commented Sep 16, 2021 • edited by macjohnny Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

ksvirkou-hubspot commented Sep 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amakhrov left a comment

Choose a reason for hiding this comment

Uh oh!

ksvirkou-hubspot commented Sep 24, 2021

Uh oh!

amakhrov commented Sep 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksvirkou-hubspot commented Sep 27, 2021

Uh oh!

amakhrov left a comment

Choose a reason for hiding this comment

Uh oh!

macjohnny commented Oct 4, 2021

Uh oh!

ksvirkou-hubspot commented Oct 5, 2021

Uh oh!

ksvirkou-hubspot commented Oct 5, 2021

Uh oh!

macjohnny commented Oct 5, 2021

Uh oh!

bodograumann commented Oct 5, 2021

Uh oh!

ksvirkou-hubspot commented Oct 5, 2021

Uh oh!

ksvirkou-hubspot commented Oct 6, 2021

Uh oh!

bodograumann commented Oct 7, 2021

Uh oh!

atanasiuk-hubspot commented Oct 26, 2021

Uh oh!

bodograumann commented Oct 26, 2021

Uh oh!

ksvirkou-hubspot commented Nov 26, 2021

Uh oh!

bodograumann commented Nov 26, 2021

Uh oh!

ksvirkou-hubspot commented Nov 26, 2021

Uh oh!

bodograumann commented Nov 26, 2021

Uh oh!

ksvirkou-hubspot commented Nov 26, 2021

Uh oh!

macjohnny commented Nov 28, 2021

Uh oh!

ksvirkou-hubspot commented Dec 10, 2021

Uh oh!

ksvirkou-hubspot commented Dec 23, 2021

Uh oh!

macjohnny commented Dec 24, 2021

Uh oh!

ksvirkou-hubspot commented Jan 3, 2022

Uh oh!

amakhrov commented Jan 4, 2022

ksvirkou-hubspot commented Sep 16, 2021 •

edited by macjohnny

Loading