From 1bd240ce799a682a55c0d259cb53b54a3bf17464 Mon Sep 17 00:00:00 2001
From: jamlo <jamilshamy@gmail.com>
Date: Tue, 8 Oct 2024 09:59:33 -0400
Subject: [PATCH] Slightly improve API error handling

This commit 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 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.
---
 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)
 	}