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
16.98k stars 6.03k forks source link

Empty response body handling in typescript-angular2 #4962

Open aacanakin opened 7 years ago

aacanakin commented 7 years ago
Description

Empty response 200 OK codes are not handled by auto generated api code.

Swagger-codegen version

2.2.2

Swagger declaration file content or url
Command line used for generation

language: typescript-angular2

Steps to reproduce

Build an api response just give 200 as http code but no response body

Suggest a Fix

Here's the current api.mustache at line 45;

return response.json()

It should be;

return response.json() || {};
wing328 commented 7 years ago

@aacanakin thanks for reporting the issue. Please submit a PR so that we can review the fix.

kenisteward commented 7 years ago

@wing328 The change is running tests currently and is a non breaking change so I PR'd on master. Check the PR @5585

wing328 commented 7 years ago

Thanks for the fix by @kenisteward

@aacanakin please pull the latest master to give it a try.

macgyver2k commented 7 years ago

please re-open; this does not work for me, and probaply for no one. when the .json() method is called on an empty body, an exception is thrown. checking for existing Content first should solve the issue, and works with my Code.

return response.arrayBuffer().byteLength > 0 ? response.json() : {};

wing328 commented 7 years ago

@macgyver2k do you mind filing a PR with the suggested fix?

macgyver2k commented 7 years ago

@wing328 yes i will do it.

kenisteward commented 7 years ago

@wing328 @macgyver2k So I have done more research into this. The way it works for angular 2 is based on this code: https://github.com/angular/angular/blob/4.0.0/packages/http/src/body.ts#L13-L36

what this does is if there is data in the body and that data is a string or an array buffer it attempts to parse it as json. so that means doing if _body is null / undefined or even {} no errors will be thrown!

In the case that _body is set to a string that is NOT parseable into a json format (e.g. the headers for the request are not set to accept json or the server is not set to serve json OR you are requesting a source that isn't json) then .json() will cause an error!

Because of this, even your solution @macgyver2k won't work if the data isn't parsable to json! We need another solution for this or we need to make the restriction that you can only use the API to parse JSON.

We could do a try/catch and in case there is a failure parsing just return response.text() instead. .json() will still work as long as it is not an arraybuffer or string though. Let me know what you guys think before I work on a full PR for it.

cc @Vrolijkx Would like your opinion as well.

Vrolijkx commented 7 years ago

Can't we just check on response.text().lenght and only call .json() if greater as 0?

kenisteward commented 7 years ago

@Vrolijkx No. That's what @macgyver2k which is why i made the other post

What happens when .text() returns something that is not json? like xml/csv/somethingelse? it's going to try to parse that text as json and then get the exception. Looking at their code all they do is check to see if it is a string. It does not check to see if there is valid json.

On Angular's part it would probably be better to do a Json.parse and then silently handling the exception by simply returning _body like they do already.

macgyver2k commented 7 years ago

@kenisteward @Vrolijkx i gues we must not call the .json() method if we do not expect json Response. in other words, only call .json() if we expect json content.

for me it seems wrong to let .json() pass non-json contents without throwing. if you call it, you have to expect it. dont expect csv when calling .json(). this would have to be some logic on top of Body Content as string/text.

can this be distinguished by using the metadata of the api specs?

kenisteward commented 7 years ago

@macgyver2k I've opened an issue with them about this . Basically if you call in() with anything other than an array buffer or string you get none Jon back which seems counter intuitive . I've given them my suggestion

kenisteward commented 7 years ago

@macgyver2k try catch around the call if we get a failure just simply return .text() otherwise it will pass back non string stuff.

macgyver2k commented 7 years ago

@kenisteward Basically if you call in() with anything other than an array buffer or string you get none Jon back which seems counter intuitive

for me it looks clearly from a perspective of a .toObject() method, then what to expect as a result is either:

since .json() returns a js object, those states are valid here. the method acts like:

but since we can interpret the method to be like .objectFromJson() meaning:

@kenisteward the second kind seems to be the one you expect, i would prefer that, too.

macgyver2k commented 7 years ago

im not that familiar with the mustache templating, but i will try to implement a version, where the expected content types are explicitly handled in a switch, calling .json() only when Content type is 'text/json' or 'application/json'.

kenisteward commented 7 years ago

@macgyver2k I can agree to that.

ghost commented 7 years ago

Such a pain with this error. I'm using ng2-http-interceptor and when responses return empty, although successful, they go to the catch operator method. I had to evaluate very strict now.

keradus commented 7 years ago

also, please be aware, that if method output is generated as Observable<Array<Foo>>, then:

response.json() || {};

is violating the output, as it shall be response.json() || []; instead

Splaktar commented 7 years ago

I've opened an issue with them about this

@kenisteward do you have a link to the issue that you opened in the Angular repo?

StefanRein commented 7 years ago

An API response with no content body has a status code of 204.

Just return on backend side with a: 204 - No Content Then this will be handled correct:

if (response.status === 204) {
    return undefined;
} else {
    return response.json() || {};
}
savelievser commented 6 years ago

I have a problem because of the "|| {}". In my case API method returns integer. And everything works until the result is zero. It's not logical to return empty object instead of zero or other primitive types values that evaluates to "false" (such as boolean "false" or empty string).

kenisteward commented 6 years ago

@Splaktar this is really late but

https://github.com/angular/angular/issues/16735

this is the issue i opened with them.

MSAJJAN-EEOC commented 6 years ago

return (<any>res)._body === '' ? {} : res.json();

worked for me

lnkhanh commented 6 years ago

Hope this helps you

this.http.post(this.settings.APIUrl, postData, { headers: headers }) .map((res: any) => { if (/^[\],:{}\s]*$/.test(res.text().replace(/\\["\\\/bfnrtu]/g, '@'). replace(/"[^"\\\n\r]*"|true|false|null|-?\d+(?:\.\d*)?(?:[eE][+\-]?\d+)?/g, ']'). replace(/(?:^|:|,)(?:\s*\[)+/g, ''))) { return res.json(); } else { return { success: false, message: 'API failed to respond!', errorString: res.text() }; } }) .subscribe( data => { console.log('OK') }, err => { console.log('Failed!'); }, () => console.log('Successfully!!!') ); });

dfdumaresq commented 6 years ago

I'm having this problem too in React using OpenAPI spec version: v1 in apiV1MessagesGet, where response.json() is being returned on valid return codes when the content is text. (We are returning HTML from a db message.) I would like to modify the mustache template to provide a switch which will return response.text() as suggested by macgyver2k. Not sure where to look first...

Could someone please provide a hint on how to do this?