Runtime Client: Can't inspect swagger error response body unless DEBUG flag is `True`

[meomap]

Part of client code is like this
https://github.com/go-openapi/runtime/blob/master/client/runtime.go#L297

	res, err := r.do(ctx, client, req) // make requests, by default follows 10 redirects before failing
	if err != nil {
		return nil, err
	}
	defer res.Body.Close()

	if r.Debug {
		b, err2 := httputil.DumpResponse(res, true)
		if err2 != nil {
			return nil, err2
		}
		r.logger.Debugf("%s\n", string(b))
	}

With flag DEBUG=true, ioutil.nopCloser returned and consumer can unwrap ClientResponse.Body() to check for validation error message like this "{\"code\":602,\"message\":\"name in body is required\"}".
For client in production environment and I don't want DEBUG flag on, response body is closed, http.bodyEOFSignal returned and can't do nothing to check for validation error.

Maybe there's option to retain response body or turn response body into ioutil.nopCloser like httputil.DumpResponse did ?

[casualjim]

The error should be in the error type. I may be misunderstanding the issue though

[meomap]

Yes, I mean the error response from client which contains the validation error message.
To illustrate the point, I created a sample unittest here.
https://github.com/meomap/sampleswagger/blob/master/api/resource_test.go#L29

The server contains just a single path for POST request with required params
https://github.com/meomap/sampleswagger/blob/master/swagger.yaml

I run test with DEBUG=1:

DEBUG=true go test -v .
...
--- FAIL: TestPostEmpty (0.01s)
        Error Trace:    resource_test.go:41
        Error:          Swagger error detail "{\"code\":602,\"message\":\"body in body is required\"}"
FAIL
exit status 1
FAIL    github.com/meomap/sampleswagger/api     3.076s

Without debug flag:

go test -v .
...
=== RUN   TestPostEmpty
--- FAIL: TestPostEmpty (0.00s)
        Error Trace:    resource_test.go:41
        Error:          Swagger error detail ""
FAIL
exit status 1
FAIL    github.com/meomap/sampleswagger/api     3.075s

Because response body is closed as I said above, client's consumer have no way to know that request failed due to required parameters.

[tatsushid]

I met the same issue at writing a client. There is no way to get a proper error message if an API server returns an error in a response body without DEBUG flag. It's very painful to find out an issue in usual operation.

For me, @meomap suggestion that the function reads a response body and wraps it in io.NopCloser sounds reasonable. I think, usually, the body should only be touched at error handling. If it keeps the original Body open, it has to be closed at out of the function by a client developer even in most none error cases. It's not so friendly to a developer.

[casualjim]

I think the spec is wrong. If you define a default response you're good to go.
The point of this specification is that you get an idea of what the inputs and outputs are of the API. Errors are outputs

[tatsushid]

Ah, I see, got it. The spec should define an error response, that's true. Thanks for pointing it out.

But what do you think keeping the original body in the response struct? It is wrapped in ClientResponse interface and it provides Body() io.ReadCloser but it is useless because every time, the body returned by the fuction has already been closed and just returns an error. I've tried to use it from APIError.Response and found this.

I think keeping it is not so difficult, just moving if r.Debug line like following should be enough because httputil.DumpResponse replaces the original Body by io.NopCloser one internally.

--- a/client/runtime.go
+++ b/client/runtime.go
@@ -400,11 +400,11 @@ func (r *Runtime) Submit(operation *runtime.ClientOperation) (interface{}, error
        }
        defer res.Body.Close()

+       b, err2 := httputil.DumpResponse(res, true)
+       if err2 != nil {
+               return nil, err2
+       }
        if r.Debug {
-               b, err2 := httputil.DumpResponse(res, true)
-               if err2 != nil {
-                       return nil, err2
-               }
                r.logger.Debugf("%s\n", string(b))
        }

[casualjim]

For solving this particular item that is correct however consider the rest of the method.
When there are successful responses that return a stream with a terrabyte of data your process will need a terrabyte of memory to dump the response because it sticks it in a bytes.Buffer. And there would be no way to opt out.

[tatsushid]

I see, your concern is reasonable. I didn't think about that.

Thanks for taking time to look this.

[fjgmmg]

I've run into the same problem. Is there a solution? If the concern is the stream size, maybe perform a check before calling DumpResponse?

[casualjim]

streams are usually produced by Transfer-Encoding: chunked, without a content length specified.
The real solution to this is making sure your swagger spec has a default response defined for the operations. That way you will get the default error filled out.

[fjgmmg]

Thanks @casualjim , but what if you want to be a little more specific about what caused the error. Say, 400 Bad Request. There could be multiple reasons why it's a bad request. Shouldn't that be delivered in the Response Body? And shouldn't that be able to be read?

[casualjim]

You can define responses for each error type (status code) you want to return.

For a fairly complete example, see: https://github.com/go-openapi/kvstore/blob/master/swagger/swagger.yml#L103-L150

OpenAPI definitions describe the contract for your service, the more exhaustive it is the better informed your clients are.

[fjgmmg]

@calavera thank you! I thought we'd already had 400 in the swagger definition for those endpoints, but it turns out we did not. The default response is great suggestion. Works now!