[api] Return a shapely error for unexpected response #16743
Conversation
api/error_unexpected_response.go
Outdated
| func (e UnexpectedResponseError) HasAdditional() bool { return e.additional != nil } | ||
| func (e UnexpectedResponseError) Additional() error { return e.additional } | ||
| func NewUnexpectedResponseError(src UnexpectedResponseErrorSource, opts ...UnexpectedResponseErrorOption) UnexpectedResponseError { | ||
| new := src() |
There was a problem hiding this comment.
| new := src() | |
| new := src() |
please don't shadow keywords
api/error_unexpected_response.go
Outdated
| u.expected = make([]int, len(s)) | ||
| copy(u.expected, s) |
There was a problem hiding this comment.
one line with https://pkg.go.dev/golang.org/x/exp/slices#Clone
api/error_unexpected_response.go
Outdated
| // FromStatusCode is the "thinnest" source for an UnexpectedResultError. It | ||
| // will attempt to resolve the status code to status text using a resolving | ||
| // function provided inside of the NewUnexpectedResponseError implementation. |
There was a problem hiding this comment.
The first sentence doesn't add any value, just start with the second sentence.
| const mockNamespaceBody = `{"Capabilities":null,"CreateIndex":1,"Description":"Default shared namespace","Hash":"C7UbjDwBK0dK8wQq7Izg7SJIzaV+lIo2X7wRtzY3pSw=","Meta":null,"ModifyIndex":1,"Name":"default","Quota":""}` | ||
|
|
||
| func TestUnexpectedResponseError(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
the testutil.Parallel helper gives us a convenient hook for manipulating parallel behavior - please use it so that we can control these tests down the line if we need to
(same for all uses of t.Parallel)
| grabber := portal.New(t) | ||
| ports := grabber.Grab(1) |
There was a problem hiding this comment.
By default this will use the same 127.0.0.1 address as the portal.Grabber being used by testutil.TestServer. We should either configure this one to use a different address, or reactor both callers to use a common Grabber like we do for the non-api packages in https://github.com/hashicorp/nomad/blob/main/ci/ports.go#L15-L20
api/error_unexpected_response.go
Outdated
|
|
||
| // FromHTTPResponse read an open HTTP response, drains and closes its body as | ||
| // the data for the UnexpectedResponseError. | ||
| func FromHTTPResponse(resp *http.Response) UnexpectedResponseErrorSource { |
There was a problem hiding this comment.
The callsite of this would look like
err := api.FromHTTPResponse(resp)
which doesn't convey much about what the function does or returns - seems like this should have a name like ErrorFromHTTPResponse ?
There was a problem hiding this comment.
FromHTTPResponse(resp) doesn't return error, it makes a required function parameter that NewUnexpectedResponseError(...) consumes to populate the error it's building. The expectation is that you call NewUnexpectedResponseError(FromHTTPResponse(resp)).
Is there a better conventional for naming the UnexpectedResponseErrorSource funcs? Also, since their is internal to the API package, maybe they should be unimported so Go API consumers don't think that they can/should use them.
| // test log output | ||
| func testLogRequestHandler(t *testing.T, h http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| t := t |
There was a problem hiding this comment.
are you sure this shallow copy is necessary? Unlike for, the parent t isn't overwritten by subsequent callers, they all get their own t
This allows users to perform additional status-based behavior by rehydrating the error using `errors.As` inside of consumers.
This gives us a place to track an error that happens during the creation of an UnexpectedResponseError
This adds a testServer implementation and some http.Handlers that can test behaviors that are difficult or unreasonable to add to the real HTTP API server. For example, the `closingHandler` intentionally provides partial results to test the `additional` error pathway.
Motivation
Currently the majority of the Nomad Go API methods use the
api.Client's generic methods like query, putQuery, etc. which wrap the response with arequireOKto report unexpected status errors. However, this results in 404's also appearing as unexpected responses. Additionally, each function that handled unexpected response errors implemented their own error string creation (even though it was identical behavior) risking drift between implementations.This PR seeks to do the following:
Create a structured error type - In considering feedback to feat: show warning if policy doesn't exist #16437, I wanted to be able to group the warnings by unexpected response error type, which would have depended on brittle string matches to implement. Providing an error that can be cast to a structured error would reduce the fragility around screen scraping and make the proposed warning code mode resilient.
Use a common error pathway - Prevent risk of drift by using the same implementation
Some considerations in this PR:
A main goal was to maintain the structure of the current error message. This is the reason that go errors stored in the UnexpectedResponseError are not printed out in the error string—it would have changed
api.websocket()'s error text. However, rather than being dropped, it is now retained inside of the UnexpectedResponseError and can be consulted if necessary.If a go error occurs that prevents normal reading of the response.Body it is recorded in
err;HasError()=>trueandError()=>err.Any errors that occur as a consequence of creating the error are recorded in
additional;HasAdditional()=>trueandAdditional() => additional.UnexpectedResponseErrors that have
additionalset add the following text when their String() func is called. This additional information in the error string is helpful, since the highest likelihood of encountering an error while making anUnexpectedResponseErrorusingFromHTTPResponse()is during theio.Copycall.