swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
17.03k stars 6.03k forks source link

[typescript-angular2] No `Content-Type` and `Accept` set in headers #6006

Open keradus opened 7 years ago

keradus commented 7 years ago
Description

While generating the API client in typescript-angular2, the Content-Type and Accept headers are not set.

Swagger-codegen version

v2.2.2 and 2.2.3-20170703.140356-40 snapshot

Swagger declaration file content or url

not available

Command line used for generation

java -jar swagger-codegen-cli.jar generate -i swagger.json -l typescript-angular2

Steps to reproduce

generate the client, send any request via it, but it's also very visible with code itself.

as we see in template: https://github.com/swagger-api/swagger-codegen/blob/v2.2.2/modules/swagger-codegen/src/main/resources/typescript-angular2/api.mustache#L115-L127 or in generated code:

    public resourceFOOGETWithHttpInfo(authorization?: string, extraHttpRequestParams?: any): Observable<Response> {
        const path = this.basePath + '/FOO';

        let queryParameters = new URLSearchParams();
        let headers = new Headers(this.defaultHeaders.toJSON()); // https://github.com/angular/angular/issues/6845
        if (authorization !== undefined && authorization !== null) {
            headers.set('Authorization', String(authorization));
        }

        // to determine the Content-Type header
        let consumes: string[] = [
        ];

        // to determine the Accept header
        let produces: string[] = [
            'application/json'
        ];

        let requestOptions: RequestOptionsArgs = new RequestOptions({
            method: RequestMethod.Get,
            headers: headers,
            search: queryParameters,
            withCredentials:this.configuration.withCredentials
        });
        // https://github.com/swagger-api/swagger-codegen/issues/4037
        if (extraHttpRequestParams) {
            requestOptions = (<any>Object).assign(requestOptions, extraHttpRequestParams);
        }

        return this.http.request(path, requestOptions);
    }

the consumes and produces vars are generated, but not used.

Related issues

not aware of

Suggest a Fix

the consumes and produces vars shall be injected into headers

wing328 commented 7 years ago

@keradus can you try the TS Angular2 generator in 2.3.0 branch?

swagger-codegen|wing328-ts_angular2_typings⚡ ⇒ grep -R '#consumes' modules/swagger-codegen/src/main/resources/typescript-angular2/*
modules/swagger-codegen/src/main/resources/typescript-angular2/api.service.mustache:            {{#consumes}}
swagger-codegen|wing328-ts_angular2_typings⚡ ⇒ grep -R '#produces' modules/swagger-codegen/src/main/resources/typescript-angular2/*
modules/swagger-codegen/src/main/resources/typescript-angular2/api.service.mustache:            {{#produces}}
keradus commented 7 years ago

I SCA-ed produced on 2.3: https://github.com/swagger-api/swagger-codegen/blob/2.3.0/modules/swagger-codegen/src/main/resources/typescript-angular2/api.service.mustache#L182-L187 var is generated, yet not used. bug still there

keradus commented 7 years ago

ping @wing328 I would be really glad for help

wing328 commented 7 years ago

@keradus if you've time, we can work together to fix it.

keradus commented 7 years ago
- headers.set('Content-Type', 'application/json'); // assuming that `Content-Type:application/json` is already set in `defaultHeaders`, if not, then it shall stay here... and moved to `defaultHeaders` in separated PR

+ if (consumes.length) {
+      headers.set('Content-Type', consumes); 
+ }

+ if (produces.length) {
+      headers.set('Accept', produces); 
+ }

have no clue how to contribute to this project or build it locally... :( feel free to grab my hints and opening PR, i would appreciate it !

wing328 commented 7 years ago

cc @Vrolijkx @taxpon @sebastianhaas @kenisteward @TiFu to see if they've time to work on the enhancement.

sebastianhaas commented 7 years ago

This was never implemented. The problem I see with your fix is that it doesn't really handle cases where an endpoint produces/consumes multiple content types. As of now, we support application/json only, so you could as well statically add

headers.set('Accept', 'application/json');

here.

keradus commented 7 years ago

Damn...

  1. lack of respecting Content-Type shall be fixed even without changing Accept part, request should sent both headers, eg:
    Content-Type: multipart/form-data;
    Accept: application/json

    (we do know that data as we filled var properly)


  1. for Accept itself, you mean that it was not implemented, as all implementadion details do .json() on parsed response ? if so, it shall still apply proper header configured via swagger, like: application/json, application/x-javascript or application/vnd.github.v3+json api outcome will be parsed properly to json, yet servers require proper header to be set. hardcoded value makes client simply not working...

Of course, if non-json output is non-working, would be nice to put note about it to some readme.


Due to all above, please consider fixing the headers, even without adding support for non-json outcome

wing328 commented 7 years ago

We need to implement SelectHeaderContentType (example) and SelectHeaderAccept (example) in ApiClient similar to what we've done for other langauges (e.g. Ruby, PHP, C#).

keradus commented 7 years ago

@wing328 , are you following semver or not for this project? (if not, nothing bad, just asking). imo, if current autogenerated code is failing (due to that lack of headers set via ignoring the vars), it's bug, not feature.

as a side note, i will use newest version anyway, so I'm extra eager to use fixed version, regardless it's number

also, General: Question could be dropped imo. it's not a question anymore, it's bug

finally, @wing328, does milestone was removed, do maintainers plan to actually solve it in some predictable future? or it's waiting for community, if someone will do it it would be accepted, if not, issue won't be fixed? no bad feelings here, just need to know shall I wait for a fix or search for different solution

sebastianhaas commented 7 years ago

Maybe I can help answering some of your questions. Re versions, you can take a look at the table in the readme.

I agree with you that disrespecting content type can be considered a bug, I'll see if I can find time to provide a fix as @wing328 suggested, but I guess as always any PRs would be more than welcome. :)

kenisteward commented 7 years ago

Is there a reason why we can't just set the default headers property on every service ? I think I might be missing something

On Mon, Jul 17, 2017, 9:20 AM Sebastian Haas notifications@github.com wrote:

Maybe I can help answering some of your questions. Re versions, you can take a look at the table in the readme https://github.com/swagger-api/swagger-codegen#compatibility.

I agree with you that disrespecting content type can be considered a bug, I'll see if I can find time to provide a fix as @wing328 https://github.com/wing328 suggested, but I guess as always any PRs would be more than welcome. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/swagger-api/swagger-codegen/issues/6006#issuecomment-315753164, or mute the thread https://github.com/notifications/unsubscribe-auth/AMPLtfGBN-53SWKpmNvCgjuLiQFKQR2Gks5sO19_gaJpZM4OQO5W .

sebastianhaas commented 7 years ago

@kenisteward we do actually set a default for Content-Type here. So we could actually send formdata if it's form data etc. but that requires some additional logic.

We could set Accept to application/json as well, (not being right now) and since we only accept json at the moment, this is probably fine, but as @keradus suggested a few days ago, there might be json-compatible mimes that would work but we rule them out nonetheless, such as application/vnd.github.v3+json.

keradus commented 7 years ago

just to make sure on that, if server is expecting one of those json-compatible mime and don't get one, it may refuse the call, even if generated client would handle parsing output ;)

on the other side, supporting non-json output would be nice as well, if i expect text or binary as output, client is trying to json-deserialize anyway.

sebastianhaas commented 7 years ago

just to make sure on that, if server is expecting one of those json-compatible mime and don't get one, it may refuse the call, even if generated client would handle parsing output ;)

That is what I was trying to say :)

But what if the endpoint is capable of handling multiple Accepts, in that case we would have to implement a strategy which is what @wing328 was pointing out before.

wing328 commented 7 years ago

For other client generators, we've implemented a function isJsonMime to match JSON-compatibile MIME such as application/vnd.github.v3+json: https://github.com/swagger-api/swagger-codegen/blob/c66a0aaa07695276fc3fa6a24bb42d2176d0f5cc/modules/swagger-codegen/src/main/resources/Java/libraries/okhttp-gson/ApiClient.mustache#L577-L580

kenisteward commented 7 years ago

So I think I may have not got my point across. The original post was how they thought they couldn't send different headers for accepts and content type.

What I was meaning is why can't in their client usage code they set their default headers to what they expect / send and then just send that stuff?

Now I do see thought that even if you tell it you will send something other than Jon we just send json

On Mon, Jul 17, 2017, 10:37 AM wing328 notifications@github.com wrote:

For other client generators, we've implemented a function isJsonMime to match JSON-compatibile MIME such as application/vnd.github.v3+json: https://github.com/swagger-api/swagger-codegen/blob/c66a0aaa07695276fc3fa6a24bb42d2176d0f5cc/modules/swagger-codegen/src/main/resources/Java/libraries/okhttp-gson/ApiClient.mustache#L577-L580

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/swagger-api/swagger-codegen/issues/6006#issuecomment-315768545, or mute the thread https://github.com/notifications/unsubscribe-auth/AMPLtVLXKpH9KnuJULBdXxie4UCKcDR7ks5sO2yVgaJpZM4OQO5W .

macjohnny commented 7 years ago

See also https://github.com/swagger-api/swagger-codegen/pull/6295#issuecomment-333179969

macjohnny commented 7 years ago

See also https://github.com/swagger-api/swagger-codegen/issues/6588 setting the content-type header inappropriately can lead to other bugs / issues.