Add error handling to REST operations in Client
#23
Conversation
|
|
||
| import "fmt" | ||
|
|
||
| type PineconeError struct { |
There was a problem hiding this comment.
This might be unnecessary, but it's somewhat straightforward to adhere to the error interface while also adding additional metadata and information that callers can use if they want. Otherwise, the Error() output should represent the entirety of the error.
| return formatError(errMap) | ||
| } | ||
|
|
||
| func formatError(errMap errorResponseMap) error { |
There was a problem hiding this comment.
This is somewhat duplicative and I'm open to changing it. I couldn't really land on a good way of trying to represent things.
This basically just turns the map into a JSON string and stuffs it into the Msg in PineconeError.
pinecone/client.go
Outdated
| Details string `json:"details,omitempty"` | ||
| } | ||
|
|
||
| func handleErrorResponseBody(resBodyBytes []byte, statusCode int, errMsgPrefix string) error { |
There was a problem hiding this comment.
When testing I noticed that we don't always seem to be guaranteed the control.ErrorResponse shape for the body, which would lead to specific errors when trying to decode things.
What I tried to do here was attempt to decode into ErrorResponse, and if that fails we just parse the whole body out as a string.
pinecone/client.go
Outdated
| func (c *Client) ListIndexes(ctx context.Context) ([]*Index, error) { | ||
| res, err := c.restClient.ListIndexes(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer res.Body.Close() | ||
|
|
||
| resBodyBytes, err := io.ReadAll(res.Body) |
There was a problem hiding this comment.
Ran into EOF problems when trying to read the body in multiple places. To avoid that I went with reading the body initially and then passing []byte around. Let me know if this is inefficient or confusing in some way, maybe there's a better way to be doing this.
| @@ -27,6 +29,47 @@ func TestClient(t *testing.T) { | |||
| suite.Run(t, new(ClientTests)) | |||
| } | |||
|
|
|||
| func TestHandleErrorResponseBody(t *testing.T) { | |||
There was a problem hiding this comment.
These tests are attempting to validate the parsing of various http.Response bodies:
ErrorResponse- generic JSON
string
| @@ -285,6 +327,34 @@ func (ts *ClientTests) TestCreatePodIndex() { | |||
| require.Equal(ts.T(), name, idx.Name, "Index name does not match") | |||
| } | |||
|
|
|||
| func (ts *ClientTests) TestCreatePodIndexInvalidDimension() { | |||
There was a problem hiding this comment.
I wasn't sure how deep to go with these integration tests that intentionally cause errors. I can add more if they seem useful.
There was a problem hiding this comment.
Looks good.
There may be places here where you can avoid reading the entire body up front which might be beneficial in the non-error cases. Those would most likely be cases with large response bodies.
You can probably adjust a lot of the defer res.Body.Close() calls and explicitly close after the body read.
These early io.ReadAll blocking IO calls may impact performance. If they were minimized to error cases only, it might be better. I'm unsure if that's feasible though.
…checking statuscode to determine when we need to read to parse errors, or when we return responses. update unit tests
Good point, I refactored things to only decode when we're dealing with an error, or we've got something to return. This is similar to what you had setup before. 👍 |
Problem
We're currently not including the code with error responses from
Client. Additionally, we don't seem to be gracefully handling possibleErrorResponsebodies alongside more generic JSON and string responses.There's an issue tracking this problem here: #17
Ultimately, we want to surface more useful information to users through errors.
Solution
This PR focuses on improving the errors that are returned from REST operations in
Clientmethods.http.ResponseBodyonce, and then use the[]bytevalue to decode into either errors or response structs.decodeIndexanddecodeCollectionto take in[]byterather thanio.ReadCloser. Most of this was done to avoid reading the body to EOF.errorResponseMapwhich is used to build up the error information.decodeErrorResponse,handleErrorResponseBody, andformatErrorto deal with parsing error payloads and turning them into more readable output.PineconeErrorwhich extends theerrorinterface and wraps errors in a struct consumers are able to use if they'd like via reflection.Type of Change
Test Plan
Unit tests and a few integration tests have been added to validate
handleErrorResponseBodyand that we're returningPineconeErrorfrom failed network operations.