From 764831dcbfe2f8d811b8dcc67363394960359427 Mon Sep 17 00:00:00 2001
From: Jamil Shamy <4977827+jamlo@users.noreply.github.com>
Date: Tue, 8 Oct 2024 10:51:16 -0400
Subject: [PATCH] Improve API error handling (#4607)

This PR tries to fix the "FromHttpResponse" function. Returning 2 errors
from a function can be confusing and uncommon in Golang, especially if
the function only returns these 2 errors.

The new code will return one error, which is always an APIError, and it
also improves the error messaging wording, adding more details in the
output.

Several more tweaks can be done to the current error message handling if
needed.

Note: this is identical to [PR
4565](https://github.com/bacalhau-project/bacalhau/pull/4565), though
created from a branch, and not a fork.
---
 pkg/publicapi/apimodels/error.go  | 19 ++++++++++++-------
 pkg/publicapi/client/v2/client.go | 27 ++++++++-------------------
 2 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/pkg/publicapi/apimodels/error.go b/pkg/publicapi/apimodels/error.go
index 6d2bc98bfd..842fe5734e 100644
--- a/pkg/publicapi/apimodels/error.go
+++ b/pkg/publicapi/apimodels/error.go
@@ -2,7 +2,6 @@ package apimodels
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -67,23 +66,29 @@ func (e *APIError) Error() string {
 }
 
 // Parse HTTP Resposne to APIError
-func FromHttpResponse(resp *http.Response) (*APIError, error) {
-
+func GenerateAPIErrorFromHTTPResponse(resp *http.Response) *APIError {
 	if resp == nil {
-		return nil, errors.New("response is nil, cannot be unmarsheld to APIError")
+		return NewAPIError(0, "API call error, invalid response")
 	}
 
 	defer resp.Body.Close()
 
 	body, err := io.ReadAll(resp.Body)
 	if err != nil {
-		return nil, fmt.Errorf("error reading response body: %w", err)
+		return NewAPIError(
+			resp.StatusCode,
+			fmt.Sprintf("Unable to read API call response body. Error: %q", err.Error()))
 	}
 
 	var apiErr APIError
 	err = json.Unmarshal(body, &apiErr)
 	if err != nil {
-		return nil, fmt.Errorf("error parsing response body: %w", err)
+		return NewAPIError(
+			resp.StatusCode,
+			fmt.Sprintf("Unable to parse API call response body. Error: %q. Body received: %q",
+				err.Error(),
+				string(body),
+			))
 	}
 
 	// If the JSON didn't include a status code, use the HTTP Status
@@ -91,7 +96,7 @@ func FromHttpResponse(resp *http.Response) (*APIError, error) {
 		apiErr.HTTPStatusCode = resp.StatusCode
 	}
 
-	return &apiErr, nil
+	return &apiErr
 }
 
 // FromBacError converts a bacerror.Error to an APIError
diff --git a/pkg/publicapi/client/v2/client.go b/pkg/publicapi/client/v2/client.go
index 18f687ed34..95979028be 100644
--- a/pkg/publicapi/client/v2/client.go
+++ b/pkg/publicapi/client/v2/client.go
@@ -74,18 +74,12 @@ func (c *httpClient) Get(ctx context.Context, endpoint string, in apimodels.GetR
 		return apimodels.NewUnauthorizedError("invalid token")
 	}
 
-	var apiError *apimodels.APIError
 	if resp.StatusCode != http.StatusOK {
-		apiError, err = apimodels.FromHttpResponse(resp)
-		if err != nil {
-			return err
+		if apiError := apimodels.GenerateAPIErrorFromHTTPResponse(resp); apiError != nil {
+			return apiError
 		}
 	}
 
-	if apiError != nil {
-		return apiError
-	}
-
 	defer resp.Body.Close()
 
 	if out != nil {
@@ -116,18 +110,12 @@ func (c *httpClient) write(ctx context.Context, verb, endpoint string, in apimod
 		return apimodels.ErrInvalidToken
 	}
 
-	var apiError *apimodels.APIError
 	if resp.StatusCode != http.StatusOK {
-		apiError, err = apimodels.FromHttpResponse(resp)
-		if err != nil {
-			return err
+		if apiError := apimodels.GenerateAPIErrorFromHTTPResponse(resp); apiError != nil {
+			return apiError
 		}
 	}
 
-	if apiError != nil {
-		return apiError
-	}
-
 	if out != nil {
 		if err := decodeBody(resp, &out); err != nil {
 			return err
@@ -362,12 +350,13 @@ func (c *httpClient) interceptError(ctx context.Context, err error, resp *http.R
 				WithCode(bacerrors.UnauthorizedError)
 		}
 
-		apiError, apiErr := apimodels.FromHttpResponse(resp)
-		if apiErr == nil {
+		apiError := apimodels.GenerateAPIErrorFromHTTPResponse(resp)
+		if apiError != nil {
 			return apiError.ToBacError()
 		}
 
-		return bacerrors.Wrap(apiErr, "server error").
+		return bacerrors.New("server error").
+			WithHTTPStatusCode(http.StatusInternalServerError).
 			WithCode(bacerrors.InternalError)
 	}