Fix premature response.Body closing in case of "binary" encoding #117
Conversation
| @@ -774,7 +785,12 @@ func addFromBody(response *http.Response, value *reflect.Value, field reflect.St | |||
| var iVal interface{} | |||
| switch encoding { | |||
| case "binary": | |||
| value.Set(reflect.ValueOf(response.Body)) | |||
| byteArr, e := ioutil.ReadAll(response.Body) | |||
There was a problem hiding this comment.
for string/binary in open api 2.0, it means a stream "octet-stream" (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md), a stream might not be able to fully loaded at this time, it might still streaming from server. So I am a bit concern here about load the stream to a buffer. (BTW, it works in kubeconfig case).
I am thinking to return the stream back to user without close it if it is a binary, thoughts?
There was a problem hiding this comment.
Hi @jasonyin , yes that was also my thinking at the beginning, and I just saw later that the implementation is also used for object storage calls for example. So yes I think not closing the stream in these cases is the correct way. Should I change the implementation like that?
There was a problem hiding this comment.
that would be great, just wondering how do you plan to do it. There's a auto generated method common.ClostBodyIfValid in every operations, I am thinking to change the template for it. If you have better ways to handle it, just let me know, thanks!
There was a problem hiding this comment.
Is the method common.ClostBodyIfValid autogenerated, or the callsites or both?
There was a problem hiding this comment.
all common code are not autogen, just other files using it.
There was a problem hiding this comment.
The getObject call in the objectstorage client does not have that close body call. It looks like to me, that it somehow already solved in some part of the generator. Maybe the same solution should be applicable here as well?
There was a problem hiding this comment.
@bonifaido , I think you can use this change to unblock yourself for now. I am going to do a fix on generation part which will be release out on next Thursday.
There was a problem hiding this comment.
Great, thanks @jasonyin, than we will wait for the fix next Thursday, until then we will use this fix in our branch.
There was a problem hiding this comment.
cool, I am closing this PR now.
…o master
Squashed commit of the following:
commit 07f2040253c385a2a6f61316f0584e9b9ea60075
Author: Mathias Ricken <mathias.ricken@oracle.com>
Date: Tue Jul 24 14:34:00 2018 -0700
Copying add_or_update_spec script from preview to master.
In case, when the HTTP response's body holds the response in "binary" encoding, for example in
CreateKubeconfig'sCreateKubeconfigResponsethe body has to be read before closing the Body in the response reading withdeferconstructs.I have created a
CloseableBuffertype to preserve compatibility with existingResponsetypes.Fixes: #116 and probably more responses are affected.