Remove usage of log.Fatal from httpclient.go; return an error object instead (FF-1934) (#36)

* Remove usage of log.Fatal from httpclient.go; return an error object instead (FF-1934)

* add comment

Author: leoromanovsky

eppoclient/configurationrequestor.go

@@ -40,11 +40,15 @@ func (ecr *experimentConfigurationRequestor) GetConfiguration(experimentKey stri
 }
 
 func (ecr *experimentConfigurationRequestor) FetchAndStoreConfigurations() {
-	result := ecr.httpClient.get(RAC_ENDPOINT)
+	result, err := ecr.httpClient.get(RAC_ENDPOINT)
+	if err != nil {
+		fmt.Println("Failed to fetch RAC response", err)
+		return
+	}
 	var wrapper racResponse
 
 	// Unmarshal JSON data directly into the wrapper struct
-	err := json.Unmarshal([]byte(result), &wrapper)
+	err = json.Unmarshal([]byte(result), &wrapper)
 	if err != nil {
 		fmt.Println("Failed to unmarshal RAC response JSON", result)
 		fmt.Println(err)

eppoclient/httpclient.go

@@ -1,8 +1,8 @@
 package eppoclient
 
 import (
+	"fmt"
 	"io"
-	"log"
 	"time"
 
 	"net/http"
@@ -33,10 +33,13 @@ func newHttpClient(baseUrl string, client *http.Client, sdkParams SDKParams) *ht
 	return hc
 }
 
-func (hc *httpClient) get(resource string) string {
+func (hc *httpClient) get(resource string) (string, error) {
 	url := hc.baseUrl + resource
 
-	req, _ := http.NewRequest(http.MethodGet, url, nil)
+	req, err := http.NewRequest(http.MethodGet, url, nil)
+	if err != nil {
+		return "", err // Return an empty string and the error
+	}
 	req.Header.Set("Content-Type", "application/json; charset=UTF-8")
 
 	q := req.URL.Query()
@@ -46,18 +49,31 @@ func (hc *httpClient) get(resource string) string {
 	req.URL.RawQuery = q.Encode()
 
 	resp, err := hc.client.Do(req)
-
 	if err != nil {
-		log.Fatal(err)
+		// from https://golang.org/pkg/net/http/#Client.Do
+		//
+		// An error is returned if caused by client policy (such as
+		// CheckRedirect), or failure to speak HTTP (such as a network
+		// connectivity problem). A non-2xx status code doesn't cause an
+		// error.
+		//
+		// We should almost never expect to see this condition be executed.
+		return "", err // Return an empty string and the error
 	}
+	defer resp.Body.Close() // Ensure the response body is closed
 
 	if resp.StatusCode == 401 {
 		hc.isUnauthorized = true
+		return "", fmt.Errorf("unauthorized access") // Return an error indicating unauthorized access
+	}
+
+	if resp.StatusCode >= 500 {
+		return "", fmt.Errorf("server error: %d", resp.StatusCode) // Handle server errors (status code > 500)
 	}
 
 	b, err := io.ReadAll(resp.Body)
 	if err != nil {
-		log.Fatalln(err)
+		return "", fmt.Errorf("server error: unreadable body") // Return an empty string and the error
 	}
-	return string(b)
+	return string(b), nil // Return the response body as a string and nil for the error
 }

eppoclient/httpclient_test.go

@@ -0,0 +1,86 @@
+package eppoclient
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestHttpClientGet(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/test":
+			w.WriteHeader(http.StatusOK)
+			_, _ = w.Write([]byte(`OK`))
+		case "/unauthorized":
+			w.WriteHeader(http.StatusUnauthorized)
+			_, _ = w.Write([]byte(`Unauthorized`))
+		case "/internal-error":
+			w.WriteHeader(http.StatusInternalServerError)
+			_, _ = w.Write([]byte(`Internal Server Error`))
+		case "/bad-response":
+			w.WriteHeader(http.StatusOK)
+			if hijacker, ok := w.(http.Hijacker); ok {
+				conn, _, _ := hijacker.Hijack()
+				conn.Close() // Close the connection to simulate an unreadable body
+			}
+		}
+	}))
+	defer server.Close()
+
+	client := &http.Client{}
+	hc := newHttpClient(server.URL, client, SDKParams{
+		apiKey:     "testApiKey",
+		sdkName:    "testSdkName",
+		sdkVersion: "testSdkVersion",
+	})
+
+	tests := []struct {
+		name           string
+		resource       string
+		expectedError  string
+		expectedResult string
+	}{
+		{
+			name:           "api returns http 200",
+			resource:       "/test",
+			expectedResult: "OK",
+		},
+		{
+			name:          "api returns 401 unauthorized error",
+			resource:      "/unauthorized",
+			expectedError: "unauthorized access",
+		},
+		{
+			name:          "api returns an 500 error",
+			resource:      "/internal-error",
+			expectedError: "server error: 500",
+		},
+		{
+			name:          "api returns unreadable body",
+			resource:      "/bad-response",
+			expectedError: "server error: unreadable body",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := hc.get(tc.resource)
+			if err != nil {
+				if err.Error() != tc.expectedError {
+					t.Errorf("Expected error %v, got %v", tc.expectedError, err)
+				}
+				if result != "" { // Check if result is not an empty string when an error is expected
+					t.Errorf("Expected result to be an empty string when there is an error, got %v", result)
+				}
+			} else {
+				if tc.expectedError != "" {
+					t.Errorf("Expected error %v, got nil", tc.expectedError)
+				}
+				if result != tc.expectedResult {
+					t.Errorf("Expected result %v, got %v", tc.expectedResult, result)
+				}
+			}
+		})
+	}
+}