From cf4c7fd8933111b6e3cf44f73aa8c28c9122f35c Mon Sep 17 00:00:00 2001
From: Daniel Mieg <56156797+DanielMieg@users.noreply.github.com>
Date: Wed, 26 Aug 2020 16:45:09 +0200
Subject: [PATCH] Correction in abaputils (#1958)

* Fix

* Adapt errors

* Consider unexpected JSON

* defer closing the response body

* Add comments to explain function

* Improve assert statements semantically

* Change comment format due to CodeClimate

* Extract sub function
---
 pkg/abaputils/abaputils.go      | 48 +++++++++++++++++++++++++--------
 pkg/abaputils/abaputils_test.go | 44 ++++++++++++++++++++++++++++--
 2 files changed, 79 insertions(+), 13 deletions(-)

diff --git a/pkg/abaputils/abaputils.go b/pkg/abaputils/abaputils.go
index 59718eb5a..fd889c72c 100644
--- a/pkg/abaputils/abaputils.go
+++ b/pkg/abaputils/abaputils.go
@@ -131,30 +131,56 @@ func GetHTTPResponse(requestType string, connectionDetails ConnectionDetailsHTTP
 }
 
 // HandleHTTPError handles ABAP error messages which can occur when using OData services
+//
+// The point of this function is to enrich the error received from a HTTP Request (which is passed as a parameter to this function).
+// Further error details may be present in the response body of the HTTP response.
+// If the response body is parseable, the included details are wrapped arround the original error from the HTTP repsponse.
+// If this is not possible, the original error is returned.
 func HandleHTTPError(resp *http.Response, err error, message string, connectionDetails ConnectionDetailsHTTP) error {
 	if resp == nil {
 		// Response is nil in case of a timeout
 		log.Entry().WithError(err).WithField("ABAP Endpoint", connectionDetails.URL).Error("Request failed")
 	} else {
+
+		defer resp.Body.Close()
+
 		log.Entry().WithField("StatusCode", resp.Status).Error(message)
 
-		// Include the error message of the ABAP Environment system, if available
-		var abapErrorResponse AbapError
-		bodyText, readError := ioutil.ReadAll(resp.Body)
-		if readError != nil {
-			return readError
+		errorDetails, parsingError := getErrorDetailsFromResponse(resp)
+		if parsingError != nil {
+			return err
 		}
-		var abapResp map[string]*json.RawMessage
-		json.Unmarshal(bodyText, &abapResp)
+		abapError := errors.New(errorDetails)
+		err = errors.Wrap(abapError, err.Error())
+
+	}
+	return err
+}
+
+func getErrorDetailsFromResponse(resp *http.Response) (errorString string, err error) {
+
+	// Include the error message of the ABAP Environment system, if available
+	var abapErrorResponse AbapError
+	bodyText, readError := ioutil.ReadAll(resp.Body)
+	if readError != nil {
+		return errorString, readError
+	}
+	var abapResp map[string]*json.RawMessage
+	errUnmarshal := json.Unmarshal(bodyText, &abapResp)
+	if errUnmarshal != nil {
+		return errorString, errUnmarshal
+	}
+	if _, ok := abapResp["error"]; ok {
 		json.Unmarshal(*abapResp["error"], &abapErrorResponse)
 		if (AbapError{}) != abapErrorResponse {
 			log.Entry().WithField("ErrorCode", abapErrorResponse.Code).Error(abapErrorResponse.Message.Value)
-			abapError := errors.New(abapErrorResponse.Code + " - " + abapErrorResponse.Message.Value)
-			err = errors.Wrap(abapError, err.Error())
+			errorString = fmt.Sprintf("%s - %s", abapErrorResponse.Code, abapErrorResponse.Message.Value)
+			return errorString, nil
 		}
-		resp.Body.Close()
 	}
-	return err
+
+	return errorString, errors.New("Could not parse the JSON error response")
+
 }
 
 // ConvertTime formats an ABAP timestamp string from format /Date(1585576807000+0000)/ into a UNIX timestamp and returns it
diff --git a/pkg/abaputils/abaputils_test.go b/pkg/abaputils/abaputils_test.go
index 0233281b4..9627bd6dd 100644
--- a/pkg/abaputils/abaputils_test.go
+++ b/pkg/abaputils/abaputils_test.go
@@ -311,7 +311,7 @@ repositories:
 func TestHandleHTTPError(t *testing.T) {
 	t.Run("Test", func(t *testing.T) {
 
-		errorValue := "HTTP 400"
+		errorValue := "Received Error"
 		abapErrorCode := "abapErrorCode"
 		abapErrorMessage := "abapErrorMessage"
 		bodyString := `{"error" : { "code" : "` + abapErrorCode + `", "message" : { "lang" : "en", "value" : "` + abapErrorMessage + `" } } }`
@@ -327,7 +327,47 @@ func TestHandleHTTPError(t *testing.T) {
 
 		err := HandleHTTPError(&resp, receivedErr, message, ConnectionDetailsHTTP{})
 		assert.Error(t, err, "Error was expected")
-		assert.EqualError(t, err, fmt.Sprintf("%s: %s - %s", errorValue, abapErrorCode, abapErrorMessage))
+		assert.EqualError(t, err, fmt.Sprintf("%s: %s - %s", receivedErr.Error(), abapErrorCode, abapErrorMessage))
+		log.Entry().Info(err.Error())
+	})
+
+	t.Run("Non JSON Error", func(t *testing.T) {
+
+		errorValue := "Received Error"
+		bodyString := `Error message`
+		body := []byte(bodyString)
+
+		resp := http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: 400,
+			Body:       ioutil.NopCloser(bytes.NewReader(body)),
+		}
+		receivedErr := errors.New(errorValue)
+		message := "Custom Error Message"
+
+		err := HandleHTTPError(&resp, receivedErr, message, ConnectionDetailsHTTP{})
+		assert.Error(t, err, "Error was expected")
+		assert.EqualError(t, err, fmt.Sprintf("%s", receivedErr.Error()))
+		log.Entry().Info(err.Error())
+	})
+
+	t.Run("Different JSON Error", func(t *testing.T) {
+
+		errorValue := "Received Error"
+		bodyString := `{"abap" : { "key" : "value" } }`
+		body := []byte(bodyString)
+
+		resp := http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: 400,
+			Body:       ioutil.NopCloser(bytes.NewReader(body)),
+		}
+		receivedErr := errors.New(errorValue)
+		message := "Custom Error Message"
+
+		err := HandleHTTPError(&resp, receivedErr, message, ConnectionDetailsHTTP{})
+		assert.Error(t, err, "Error was expected")
+		assert.EqualError(t, err, fmt.Sprintf("%s", receivedErr.Error()))
 		log.Entry().Info(err.Error())
 	})
 }