Add error handling to REST operations in Client (#23)

## Problem We're currently not including the code with error responses from `Client`. Additionally, we don't seem to be gracefully handling possible `ErrorResponse` bodies 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 `Client` methods. - Decode the `http.Response` `Body` once, and then use the `[]byte` value to decode into either errors or response structs. - Update `decodeIndex` and `decodeCollection` to take in `[]byte` rather than `io.ReadCloser`. Most of this was done to avoid reading the body to EOF. - Add `errorResponseMap` which is used to build up the error information. - Add `decodeErrorResponse`, `handleErrorResponseBody`, and `formatError` to deal with parsing error payloads and turning them into more readable output. - Add `PineconeError` which extends the `error` interface and wraps errors in a struct consumers are able to use if they'd like via reflection. ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan Unit tests and a few integration tests have been added to validate `handleErrorResponseBody` and that we're returning `PineconeError` from failed network operations. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1207208972408344
pinecone-io · Jun 7, 2024 · b471e0c · b471e0c
1 parent c7eda0a
commit b471e0c
Show file tree

Hide file tree

Showing 3 changed files with 193 additions and 59 deletions.
diff --git a/pinecone/client.go b/pinecone/client.go
@@ -107,24 +107,6 @@ func (c *Client) IndexWithAdditionalMetadata(host string, namespace string, addi
 	return idx, nil
 }
 
-func (c *Client) extractAuthHeader() map[string]string {
-	possibleAuthKeys := []string{
-		"api-key",
-		"authorization",
-		"access_token",
-	}
-
-	for key, value := range c.headers {
-		for _, checkKey := range possibleAuthKeys {
-			if strings.ToLower(key) == checkKey {
-				return map[string]string{key: value}
-			}
-		}
-	}
-
-	return nil
-}
-
 func (c *Client) ListIndexes(ctx context.Context) ([]*Index, error) {
 	res, err := c.restClient.ListIndexes(ctx)
 	if err != nil {
@@ -133,7 +115,7 @@ func (c *Client) ListIndexes(ctx context.Context) ([]*Index, error) {
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("unexpected status code: %d", res.StatusCode)
+		return nil, handleErrorResponseBody(res, "failed to list indexes: ")
 	}
 
 	var indexList control.IndexList
@@ -222,12 +204,7 @@ func (c *Client) CreatePodIndex(ctx context.Context, in *CreatePodIndexRequest)
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusCreated {
-		var errResp control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errResp)
-		if err != nil {
-			return nil, fmt.Errorf("failed to decode error response: %w", err)
-		}
-		return nil, fmt.Errorf("failed to create index: %s", errResp.Error.Message)
+		return nil, handleErrorResponseBody(res, "failed to create index: ")
 	}
 
 	return decodeIndex(res.Body)
@@ -262,12 +239,7 @@ func (c *Client) CreateServerlessIndex(ctx context.Context, in *CreateServerless
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusCreated {
-		var errResp control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errResp)
-		if err != nil {
-			return nil, fmt.Errorf("failed to decode error response: %w", err)
-		}
-		return nil, fmt.Errorf("failed to create index: %s", errResp.Error.Message)
+		return nil, handleErrorResponseBody(res, "failed to create index: ")
 	}
 
 	return decodeIndex(res.Body)
@@ -281,12 +253,7 @@ func (c *Client) DescribeIndex(ctx context.Context, idxName string) (*Index, err
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusOK {
-		var errResp control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errResp)
-		if err != nil {
-			return nil, fmt.Errorf("failed to decode error response: %w", err)
-		}
-		return nil, fmt.Errorf("failed to describe idx: %s", errResp.Error.Message)
+		return nil, handleErrorResponseBody(res, "failed to describe index: ")
 	}
 
 	return decodeIndex(res.Body)
@@ -300,12 +267,7 @@ func (c *Client) DeleteIndex(ctx context.Context, idxName string) error {
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusAccepted {
-		var errResp control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errResp)
-		if err != nil {
-			return fmt.Errorf("failed to decode error response: %w", err)
-		}
-		return fmt.Errorf("failed to delete index: %s", errResp.Error.Message)
+		return handleErrorResponseBody(res, "failed to delete index: ")
 	}
 
 	return nil
@@ -319,7 +281,7 @@ func (c *Client) ListCollections(ctx context.Context) ([]*Collection, error) {
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("unexpected status code: %d", res.StatusCode)
+		return nil, handleErrorResponseBody(res, "failed to list collections: ")
 	}
 
 	var collectionsResponse control.CollectionList
@@ -343,7 +305,7 @@ func (c *Client) DescribeCollection(ctx context.Context, collectionName string)
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("unexpected status code: %d", res.StatusCode)
+		return nil, handleErrorResponseBody(res, "failed to describe collection: ")
 	}
 
 	return decodeCollection(res.Body)
@@ -367,13 +329,7 @@ func (c *Client) CreateCollection(ctx context.Context, in *CreateCollectionReque
 	defer res.Body.Close()
 
 	if res.StatusCode != http.StatusCreated {
-		var errorResponse control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errorResponse)
-		if err != nil {
-			return nil, err
-		}
-
-		return nil, fmt.Errorf("failed to create collection: %s", errorResponse.Error.Message)
+		return nil, handleErrorResponseBody(res, "failed to create collection: ")
 	}
 
 	return decodeCollection(res.Body)
@@ -386,14 +342,26 @@ func (c *Client) DeleteCollection(ctx context.Context, collectionName string) er
 	}
 	defer res.Body.Close()
 
-	// Check for successful response, consider successful HTTP codes like 200 or 204 as successful deletion
 	if res.StatusCode != http.StatusAccepted {
-		var errResp control.ErrorResponse
-		err = json.NewDecoder(res.Body).Decode(&errResp)
-		if err != nil {
-			return fmt.Errorf("failed to decode error response: %w", err)
+		return handleErrorResponseBody(res, "failed to delete collection: ")
+	}
+
+	return nil
+}
+
+func (c *Client) extractAuthHeader() map[string]string {
+	possibleAuthKeys := []string{
+		"api-key",
+		"authorization",
+		"access_token",
+	}
+
+	for key, value := range c.headers {
+		for _, checkKey := range possibleAuthKeys {
+			if strings.ToLower(key) == checkKey {
+				return map[string]string{key: value}
+			}
 		}
-		return fmt.Errorf("failed to delete collection '%s': %s", collectionName, errResp.Error.Message)
 	}
 
 	return nil
@@ -473,6 +441,69 @@ func decodeCollection(resBody io.ReadCloser) (*Collection, error) {
 	return toCollection(&collectionModel), nil
 }
 
+func decodeErrorResponse(resBodyBytes []byte) (*control.ErrorResponse, error) {
+	var errorResponse control.ErrorResponse
+	err := json.Unmarshal(resBodyBytes, &errorResponse)
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode error response: %w", err)
+	}
+
+	if errorResponse.Status == 0 {
+		return nil, fmt.Errorf("unable to parse ErrorResponse: %v", string(resBodyBytes))
+	}
+
+	return &errorResponse, nil
+}
+
+type errorResponseMap struct {
+	StatusCode int    `json:"status_code"`
+	Body       string `json:"body,omitempty"`
+	ErrorCode  string `json:"error_code,omitempty"`
+	Message    string `json:"message,omitempty"`
+	Details    string `json:"details,omitempty"`
+}
+
+func handleErrorResponseBody(response *http.Response, errMsgPrefix string) error {
+	resBodyBytes, err := io.ReadAll(response.Body)
+	if err != nil {
+		return fmt.Errorf("failed to read response body: %w", err)
+	}
+
+	var errMap errorResponseMap
+	errMap.StatusCode = response.StatusCode
+
+	// try and decode ErrorResponse
+	if json.Valid(resBodyBytes) {
+		errorResponse, err := decodeErrorResponse(resBodyBytes)
+		if err == nil {
+			errMap.Message = errorResponse.Error.Message
+			errMap.ErrorCode = string(errorResponse.Error.Code)
+
+			if errorResponse.Error.Details != nil {
+				errMap.Details = fmt.Sprintf("%+v", errorResponse.Error.Details)
+			}
+		}
+	}
+
+	errMap.Body = string(resBodyBytes)
+
+	if errMap.Message != "" {
+		errMap.Message = errMsgPrefix + errMap.Message
+	}
+
+	return formatError(errMap)
+}
+
+func formatError(errMap errorResponseMap) error {
+	jsonString, err := json.Marshal(errMap)
+	if err != nil {
+		return err
+	}
+	baseError := fmt.Errorf(string(jsonString))
+
+	return &PineconeError{Code: errMap.StatusCode, Msg: baseError}
+}
+
 func buildClientBaseOptions(in NewClientBaseParams) []control.ClientOption {
 	clientOptions := []control.ClientOption{}
 
@@ -546,3 +577,8 @@ func minOne(x int32) int32 {
 	}
 	return x
 }
+
+func PrettifyStruct(obj interface{}) string {
+	bytes, _ := json.MarshalIndent(obj, "", "  ")
+	return string(bytes)
+}
diff --git a/pinecone/client_test.go b/pinecone/client_test.go
@@ -3,7 +3,10 @@ package pinecone
 import (
 	"context"
 	"fmt"
+	"io"
+	"net/http"
 	"os"
+	"reflect"
 	"strings"
 	"testing"
 
@@ -27,6 +30,47 @@ func TestClient(t *testing.T) {
 	suite.Run(t, new(ClientTests))
 }
 
+func TestHandleErrorResponseBody(t *testing.T) {
+	tests := []struct {
+		name         string
+		responseBody *http.Response
+		statusCode   int
+		prefix       string
+		errorOutput  string
+	}{
+		{
+			name:         "test ErrorResponse body",
+			responseBody: mockResponse(`{"error": { "code": "INVALID_ARGUMENT", "message": "test error message"}, "status": 400}`, http.StatusBadRequest),
+			statusCode:   http.StatusBadRequest,
+			errorOutput:  `{"status_code":400,"body":"{\"error\": { \"code\": \"INVALID_ARGUMENT\", \"message\": \"test error message\"}, \"status\": 400}","error_code":"INVALID_ARGUMENT","message":"test error message"}`,
+		}, {
+			name:         "test JSON body",
+			responseBody: mockResponse(`{"message": "test error message", "extraCode": 665}`, http.StatusBadRequest),
+			statusCode:   http.StatusBadRequest,
+			errorOutput:  `{"status_code":400,"body":"{\"message\": \"test error message\", \"extraCode\": 665}"}`,
+		}, {
+			name:         "test string body",
+			responseBody: mockResponse(`test error message`, http.StatusBadRequest),
+			statusCode:   http.StatusBadRequest,
+			errorOutput:  `{"status_code":400,"body":"test error message"}`,
+		}, {
+			name:         "Test error response with empty response",
+			responseBody: mockResponse(`{}`, http.StatusBadRequest),
+			statusCode:   http.StatusBadRequest,
+			prefix:       "test prefix",
+			errorOutput:  `{"status_code":400,"body":"{}"}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := handleErrorResponseBody(tt.responseBody, tt.prefix)
+			assert.Equal(t, err.Error(), tt.errorOutput, "Expected error to be '%s', but got '%s'", tt.errorOutput, err.Error())
+
+		})
+	}
+}
+
 func (ts *ClientTests) SetupSuite() {
 	apiKey := os.Getenv("PINECONE_API_KEY")
 	require.NotEmpty(ts.T(), apiKey, "PINECONE_API_KEY env variable not set")
@@ -51,7 +95,6 @@ func (ts *ClientTests) SetupSuite() {
 	// named a UUID. Generally not needed as all tests are cleaning up after themselves
 	// Left here as a convenience during active development.
 	//deleteUUIDNamedResources(context.Background(), &ts.client)
-
 }
 
 func (ts *ClientTests) TestNewClientParamsSet() {
@@ -285,6 +328,34 @@ func (ts *ClientTests) TestCreatePodIndex() {
 	require.Equal(ts.T(), name, idx.Name, "Index name does not match")
 }
 
+func (ts *ClientTests) TestCreatePodIndexInvalidDimension() {
+	name := uuid.New().String()
+
+	_, err := ts.client.CreatePodIndex(context.Background(), &CreatePodIndexRequest{
+		Name:        name,
+		Dimension:   -1,
+		Metric:      Cosine,
+		Environment: "us-east1-gcp",
+		PodType:     "p1.x1",
+	})
+	require.Error(ts.T(), err)
+	require.Equal(ts.T(), reflect.TypeOf(err), reflect.TypeOf(&PineconeError{}), "Expected error to be of type PineconeError")
+}
+
+func (ts *ClientTests) TestCreateServerlessIndexInvalidDimension() {
+	name := uuid.New().String()
+
+	_, err := ts.client.CreateServerlessIndex(context.Background(), &CreateServerlessIndexRequest{
+		Name:      name,
+		Dimension: -1,
+		Metric:    Cosine,
+		Cloud:     Aws,
+		Region:    "us-west-2",
+	})
+	require.Error(ts.T(), err)
+	require.Equal(ts.T(), reflect.TypeOf(err), reflect.TypeOf(&PineconeError{}), "Expected error to be of type PineconeError")
+}
+
 func (ts *ClientTests) TestCreateServerlessIndex() {
 	name := uuid.New().String()
 
@@ -310,6 +381,12 @@ func (ts *ClientTests) TestDescribeServerlessIndex() {
 	require.Equal(ts.T(), ts.serverlessIndex, index.Name, "Index name does not match")
 }
 
+func (ts *ClientTests) TestDescribeNonExistentIndex() {
+	_, err := ts.client.DescribeIndex(context.Background(), "non-existent-index")
+	require.Error(ts.T(), err)
+	require.Equal(ts.T(), reflect.TypeOf(err), reflect.TypeOf(&PineconeError{}), "Expected error to be of type PineconeError")
+}
+
 func (ts *ClientTests) TestDescribeServerlessIndexSourceTag() {
 	index, err := ts.clientSourceTag.DescribeIndex(context.Background(), ts.serverlessIndex)
 	require.NoError(ts.T(), err)
@@ -462,3 +539,12 @@ func deleteUUIDNamedResources(ctx context.Context, c *Client) error {
 
 	return nil
 }
+
+func mockResponse(body string, statusCode int) *http.Response {
+	return &http.Response{
+		Status:     http.StatusText(statusCode),
+		StatusCode: statusCode,
+		Body:       io.NopCloser(strings.NewReader(body)),
+		Header:     make(http.Header),
+	}
+}
diff --git a/pinecone/errors.go b/pinecone/errors.go
@@ -0,0 +1,12 @@
+package pinecone
+
+import "fmt"
+
+type PineconeError struct {
+	Code int
+	Msg  error
+}
+
+func (pe *PineconeError) Error() string {
+	return fmt.Sprintf("%+v", pe.Msg)
+}