[Typescript][Angular] Response headers are not available.

[HermenOtter]

Description

I want to access the response headers for an E-tag. For my current swagger yaml file, it doesn't generate anything to access it. This is the same issue it is supposedly fixed, but in my current generated code base I cannot find the withHttpInfo methods, are they removed in a newer version? Or am I doing something wrong?
@wing328

Swagger-codegen version

master (commit f78d958 ))

Swagger declaration file content or url

path:
    parameters:
    ............
    get:    
      .............
      produces:
      - "application/json"
      .............
      responses:
        200:          
          headers:
            ETag:
              type: string
          schema:
            ............         

 public get(......params..): Observable<any> {
        if (parm=== null || param=== undefined) {
            throw new Error('error.');
        }
        let headers = this.defaultHeaders;

        return this.httpClient.get<any>(`${this.basePath}...path...}`,
            {
                headers: headers,
                withCredentials: this.configuration.withCredentials,
            }
        );
    }

Suggestion

There is a way to fix it with the new httpclient.
@kenisteward

[kenisteward]

So it is possible with the addition of 4.3 http client that the generation of those method were removed for that type. I'll look into this when I get to my computer

…

On Wed, Nov 8, 2017, 4:03 AM Hermen Otter ***@***.***> wrote: @kenisteward <https://github.com/kenisteward> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6902 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLtX4lduTcABCAX47Xcx3p9anwRvP9ks5s0W58gaJpZM4QU8xm> .

[kenisteward]

@HermenOtter looking at the example you gave it looks like no fix is needed. All you do is subscribe to the observable it returns and then inspect the response. Am I missing something?

With the old http module because it returned the json() straight up we needed the extra function. Now with the http client that allows us to check out the headers without the need of that extra function.

[HermenOtter]

@kenisteward Thank you for your fast response.
The problem is: that the current method only returns the body and not the body and the headers.

this.service.getSubject(this.subjectId).subscribe(
        result => { this.subject = result; console.log(result.headers);

This logs: undefined.

I read that you first need to define the observe parameter as "response" to retrieve the headers and body.

As you can see the default value for observe? is "body".

However if you change the httpclient to return the "response", you still need to make decisions on how to structure the code, an extra method might still be required? Maybe the author of the 4.3 httpclient can give some insight on choices made?

@wing328 @bedag-moo any thoughts?

[wing328]

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

[kenisteward]

@HermenOtter what if we just allow a final argument that allows you to set that field.

I think what we can do to keep it simple is allow an extraclientoptions field and object.assign that without needing to generate a whole other function. By default we set return to body so we don't break current code using it and then per call you can set if you want the full response or not.

[HermenOtter]

@kenisteward I thought of that solution as well, in the first instance I thought returning the object would be a problem(because of the interface of the model), but I tested the solution and I was mistaken.

This works with a bloated if statement:

public getSubjectByNr(id: number, fullResponse: boolean = false): Observable<any> {
        if (id === null || id === undefined) {
            throw new Error('Required parameter subjectnr was null or undefined when calling getSubjectByNr.');
        }

        const headers = this.defaultHeaders;

        if (fullResponse) {
            return this.httpClient.get<any>(`${this.basePath}/subj/v1/subjecten/${encodeURIComponent(String(id))}`,
                {
                    observe: 'response',
                    headers: headers,
                    withCredentials: this.configuration.withCredentials,
                }
            );
        } else {
            return this.httpClient.get<any>(`${this.basePath}/subj/v1/subjecten/${encodeURIComponent(String(id))}`,
                {
                    headers: headers,
                    withCredentials: this.configuration.withCredentials,
                }
            );
        }
    }

Used as

 this.service.getSubjectByNr(this.subjectNr, true).subscribe(
        result => { this.subject = result.body; console.log(result.body); console.log(result.headers);  }
        , error => console.error(error));

I can confirm that the logged body and headers are correct.

[kenisteward]

@HermenOtter

Unfortunately, the reason it works is that we are returning Observable instead of Observable;

We could do something like Observable<ModelReturnType || Response> that way we don't break typings and still get the actual type to cast as.

Also, I was thinking something a bit more simple bloat wise.

2 options

// first option just let the user pass what they want back
public getSubjectByNr(id: number, observe: string = 'body'): Observable<any> {
        if (id === null || id === undefined) {
            throw new Error('Required parameter subjectnr was null or undefined when calling getSubjectByNr.');
        }

        const headers = this.defaultHeaders;

            return this.httpClient.get<any>(`${this.basePath}/subj/v1/subjecten/${encodeURIComponent(String(id))}`,
                {
                    headers: headers,
                    withCredentials: this.configuration.withCredentials,
                    observe
                }
            );
       
    }

second option:

// do like we do now and allow the user to overwrite the full http options except this will be for client
public getSubjectByNr(id: number, extraHttpClientOptions?: any): Observable<any> {
        if (id === null || id === undefined) {
            throw new Error('Required parameter subjectnr was null or undefined when calling getSubjectByNr.');
        }

        const headers = this.defaultHeaders;

       const reqOptions = {
            headers,
            withCredentials: this.configuration.withCredentials
}

          // in the extras you can overwrite whatever you want
         // extra options defined here https://angular.io/api/common/http/HttpClient
          Object.assign(reqOptions, extras);


            return this.httpClient.get<any>(`${this.basePath}/subj/v1/subjecten/${encodeURIComponent(String(id))}`, reqOptoins);
       
    }

I think the second way would be more suitable for a bigger audience. Either way, the fix would be easy and I believe I could have the fix in today If i got the time.

If we wanted to get even more spiffy we could allow it as a configuration value as well so the assignment value could end up being

  observe: observe || this.configuration.observe || 'body'

that way you could set your entire app to return the same thing if that mattered.

[HermenOtter]

@kenisteward First option is a good simple fix. However the second option gives power back to the developer and fixes the problem as well, it seems like a really nifty solution!

About a global configuration, if you can prioritize observe method parameter > global observe > body, as you stated above that would be great. Though the problem with that implementation is that it isn't as future proof as adding extraHttpClientOptions to the global configuration, for example when the angular http options change or you might have to add every option manually to the template if an user requests it when an issue pops up. What are your thoughts on adding the extraHttpClientOptions to global configuration?

[bedag-moo]

Ah, that's what that withHttpInfo method was for!

My apologies, I could not imagine why one might need this, so I didn't implement this method when writing the new code generator using the httpClient.

As for fixes: Kenis solutions are not quite type safe, and solution 1 would be a breaking change, because

const x: Observable<Issue> = issueService.getIssues();

would no longer compile because Observable<Response> is not assignable to Observable<Issue>.

I recommend an overloaded method signature, like the one the httpClient itself uses. To avoid a breaking change, the new parameter should be optional and default to body.

[kenisteward]

@bedag-moo

const x: Observable<Issue> = issueService.getIssues();

works if we define .getIssues() like:

getIssues() : Observable<Response | Issue>

which was in the preface of my solution :P

[kenisteward]

@bedag-moo @HermenOtter I would be interested in exploring the option of using
https://www.typescriptlang.org/docs/handbook/functions.html that angular uses as was pointed out. It may be a much more elegant solution to Union types.

[bedag-moo]

It does not work for me; Typescript 2.4.2 says:

src/app/edit-issue.component.ts(36,15): error TS2322: Type 'Observable<Issue | Response>' is not assignable to type 'Observable<Issue>'.
  Type 'Issue | Response' is not assignable to type 'Issue'.
    Type 'Response' is not assignable to type 'Issue'.
      Property 'id' is missing in type 'Response'.

Might you have tested with mocked up Issue and Response types which are mutually assignable?

[kenisteward]

@HermenOtter

What are your thoughts on adding the extraHttpClientOptions to global configuration?

If we implement it in Solution 2 from above we don't have to worry about having a global extraHttpClientOptions unless we care about setting something for every request created. (e.g. from my spiffy solution)

I'm honestly fine with it. I think it would be a dope idea to allow for a user to generically define what most requests should look like. That changes the spiffy solution to be this instead:

// do like we do now and allow the user to overwrite the full http options except this will be for client
public getSubjectByNr(id: number, extraHttpClientOptions?: any): Observable<any> {
     ...
     // options that are needed to be set in the fuction
     const reqOptions = { ...previouslySetOptions }
     // override the function options first with default then with passed in options
     Object.assign(reqOptions, this.configuration.extraHttpClientOptions, extraHttpClientOptions);

     return this.httpClient.get<any>(`${this.basePath}/subj/v1/subjecten/${encodeURIComponent(String(id))}`, reqOptoins);
}

Mind you I think we maybe should be called .get etc. with the model type expected if we aren't.

[kenisteward]

@bedag-moo

const x: Observable<Issue> = issueService.getIssues() as Observable<Issue>;

Fixes that but I think it would be better if we didn't have to put the as after every call haha.

then again

const x = issueService.getIssues() as Observable<Issue>;

gives you the exact thing you want with the same amount of typing / intellisense usage

[kenisteward]

@bedag-moo @HermenOtter
Overloaded function generated example (made this by hand for now to get the idea of how we'd do it)

getIssues(extraHttpOptions?: { 
    headers?: HttpHeaders|{[header: string]: string | string[]},
    observe?: 'body',
    params?: HttpParams|{[param: string]: string | string[]},
    reportProgress?: boolean,
    withCredentials?: boolean,
  }}): Observable<Array<Issue>>;
getIssues(extraHttpOptions?: { 
    headers?: HttpHeaders|{[header: string]: string | string[]},
    observe?: 'response',
    params?: HttpParams|{[param: string]: string | string[]},
    reportProgress?: boolean,
    withCredentials?: boolean,
  }): Observable<HttpResponse<Array<Issue>>>;
getIssues(extraHttpOptions?: { 
    headers?: HttpHeaders|{[header: string]: string | string[]},
    observe?: 'events',
    params?: HttpParams|{[param: string]: string | string[]},
    reportProgress?: boolean,
    withCredentials?: boolean,
  }): Observable<HttpEvent<Array<Issue>>>
getIssues(extraHttpOptions: any): Observable<any> {
        ...
     // options that are needed to be set in the fuction
     const reqOptions = { ...previouslySetOptionsLikeResponseType }
     // override the function options first with default then with passed in options
     Object.assign(reqOptions, this.configuration.extraHttpClientOptions, extraHttpClientOptions);

     return this.httpClient.get<any>(`${this.basePath}/issues`, reqOptions);
}

Note: We would have to dynamically generate whether we are returning blob / arraybuffer based on the client code. as such the reponseType that you see in the angular version should never be passed in and should be decided by the client generation. Also this would allow users to be able to get the full http events for uploading and downloading! Last, if we used the global extraOptions it would throw off the compiler knowing what type should be returned as that is not known until runtime.

Let me know what you guys think!

[bedag-moo]

The approach looks good to me, but I wonder whether we really need the ability to pass headers and parameters that do not appear in the swagger docs on a per-operation-basis? Off hand, I can't imagine when that would make sense, and would have simply done:

getIssues(observe: "response"): Observable<HttpResponse<Array<Issue>>>;
getIssues(observe: "body" = "body"): Observable<Array<Issue>> {
   // impl
}

But I'm ok with the extra flexible solution, too.

[kenisteward]

@bedag-moo I originally thought something like that as well but was wanting to keep it as generic as possible.

There are technically 3 observe values (bod, response, events). Because of that, I foresee in the future someone wanting to get the events so in the case of that we need to make sure we have an overload.

[HermenOtter]

@kenisteward I agree, all observe values should be supported, otherwise someone in the future will have to go through all this mess again. On the other hand I do agree the feature should not be over-engineered, making it messy or too complicated @bedag-moo

I believe if type safety can be maintained, we should choose for the extra flexible solution, I think it will pay off in the future. The trade-offs seem minimal. What would be the next step to implementing a solution? @kenisteward @bedag-moo

[kenisteward]

@bedag-moo actually it definitely makes sense not to pass headers and params.

simplified example

getIssues(extraHttpOptions?: { observe: 'body', reportProgress?: boolean }):  Observable<Array<Issue>>;
getIssues(extraHttpOptions:  { observe: 'response', reportProgress?: boolean }): Observable<HttpResponse<Array<Issue>>>;
getIssues(extraHttpOptions:  { observe: 'events' | HttpObserve, reportProgress?: boolean })): Observable<HttpEvent<Array<Issue>>>;
getIssues(extraHttpOptions: { observe: HttpObserve, reportProgress?: boolean } = {
    observe: 'body'
} ): any {
    //impl
}

So this will give you the correct typings all the way down.

If you pass nothing it lets you know you'll get body
If you pass body you'll get obs body
if you pass response you'll get obs response
if you pass events you'll get obs event
If you pass something that is not inside the type HttpObserve it will let you know the value you passed isn't that one. that is enabled by the 'events' | HttpObserve at the end

this also lets your set report events if you want them.

@HermenOtter Because all of the rest of the values are determined by the generated client it think it's fine saying we shouldn't pass those per function. random params can always be set in an interceptor since we are using the HttpClient Because of that I've updated the solutions to this.

Typescript Playground Here

If you change the getIssues call to the different possible values and no value the compiler let's you know what should and shouldn't be returned.

[bedag-moo]

Looks good to me :-)

[kenisteward]

@bedag-moo @HermenOtter I'm gonna work on this tomorrow/sunday. I'll open up the PR once i have something useful. I don't have any app that uses Angular 4.3 yet so i'll need you guys to test it.

[HermenOtter]

@kenisteward The example looks good, I would appreciate that implementation. I will certainly test it when your PR is available.

[kenisteward]

@HermenOtter @bedag-moo your wish is my command! PR #6955 opened for this feature. was pretty straight forward as I thought it would be. Has beautiful intellisense with the overloads.

@bedag-moo thanks for pointing out to use them! I'd love to make a PR for the old ones to take with withHttpInfo but I think more people may be using it than I think.

Note: had to change of the implementation a bit due to an issue with intellisense

[HermenOtter]

@kenisteward Thank you for the PR, I tested your implementation I have an error which I posted on your PR, which version of typescript are you using?

[kenisteward]

@HermenOtter should be fixed as of earlier

[HermenOtter]

@kenisteward I can confirm it compiles and works as expected for my application.