http.Response.Body() closed in Do() regardless of value of v

[dtuck9]

Describe the bug
In attempting to read the response body from the return http.Response from client.Do() when v is nil, the following error is received:

http2: response body closed

To Reproduce
Steps to reproduce the behavior:

1. Create a new client (artifactory.NewClient("artifactory url", nil))
2. Create a new request (client.NewRequest(http.MethodGet, "path", nil))
3. Execute the request (client.Do(ctx, request, nil))

Expected behavior
A readable response.Body that has yet to be closed.

[dillon-giacoppo]

Thanks for contributing back! The behaviour here needs to be carefully added. Since responses are always returned, they should either always require closing, or always not require closing. Otherwise the code in callers requires an if condition making it verbose. The assumption of the client was that if the body was required it would always be parsed into the correct struct. What is your use case where this is not true?

[dillon-giacoppo]

From looking at the Golang http docs

On error, any Response can be ignored. A non-nil Response with a non-nil error only occurs when CheckRedirect fails, and even then the returned Response.Body is already closed.

So the behaviour you have requested seems standard. However, this will require cutting a new major release which is why I just wanted to confirm when the response body is required. I use the lib in a terraform provider and only have the request so I can check the status code for some errors.

[dtuck9]

Hi @dillon-giacoppo, thank you for the quick reply!

For context, I am pulling a non-json-encoded file down from Artifactory. Per the godocs of the artifactory.Client.Do() function itself:

If v is nil then the body is not read and can be manually parsed from the response

So it sounded like the body should be readable from the returned response (and as you mentioned, is pretty standard). I'll get around it for the time being by passing in a io.Writer, but it would be great if the godocs could be clarified in the meantime.

[dillon-giacoppo]

I have been passing in an io.Writer myself which I realise might be an anti-pattern. I will include your changes in the next v2 release which has support for the new API. Whats the endpoint this is for? It feels like this is something the client should handle itself, are you using the client to actually download artefacts? I would most definitely use the official artifactory cli for that. This library was designed to cover the gaps in "meta" configuration i.e creating repos. It was unintended that the client would be directly accessed so I would be keen to add support for what you are trying to do and ideally mark the client as internal.

[dtuck9]

Yes, I'm downloading artifacts via a script to verify test coverage in a CI pipeline. I'll poke around the JFrog client then if I'm using this in an unintended way.

Thanks for the help!