RabbitMQ MGMT API error reasons are not returned for some requests.

[niclic]

Something I noticed while working on #150.

What is the issue?

All PUT and DELETE requests use the executeRequest function in client.go. When the RabbitMQ Management API returns an HTTP error response, some details get discarded (i.e. the error reason). There is no parsing of the response body in this function.

For example:

curl -i -u guest:guest \
-X PUT http://localhost:15672/api/parameters/federation-upstream/%2f/bigwig \
-H "content-type:application/json" \
-d \
'{
  "value": {
    "uri": "amqp://server-name",
    "prefetch-count": "1000"
  }
}'
HTTP/1.1 400 Bad Request

{
  "error": "bad_request",
  "reason": "Validation failed\n\nprefetch-count should be a number, actually was <<\"1000\">>\n"
}

This currently is returned as a http.Response that represents the HTTP error: 400 Bad Request. The error is nil.

In contrast, all GET requests use executeAndParseRequest, which does parse HTTP response body to ResponseError, preserving the reason for the error. This was done in #87.

---

Why should we consider changing this behaviour?

I don't think this is by design (i.e. encapsulation), so considering:

a) the error reasons returned from the RabbitMQ Management API might be useful.
b) the HTTP response body is parsed for all GET requests anyway.

Would it make sense to have the same behaviour for all methods?

---

What is the proposed fix?

Because all methods ultimately call executeRequest anyway, it is simply a matter of promoting the error parsing code inexecuteAndParseRequest to executeRequest.

You can see an example of what this change could look like in this commit. I extracted the existing parsing logic into a function called parseResponseErrors.

Interestingly, after making this change, I wasn't expecting to see test failures (other than for the two tests I wrote to handle this scenario). However, three other tests failed. Some logic was missing and the underlying Management API errors were ignored, including validation errors.

Let me know if you consider this a worthwhile change and I can push the branch into a PR for review.

[michaelklishin]

It definitely sounds like a major usability annoyance. I'm all for returning more relevant information to the user.

[niclic]

Closed by #152