From 06b2a5647b6f980b843cf0a14c0d30573ad83d30 Mon Sep 17 00:00:00 2001
From: Kristin Laemmert <mildwonkey@users.noreply.github.com>
Date: Wed, 8 Jan 2025 11:00:53 -0500
Subject: [PATCH] (fix): improve status handling in API Domain

Properly handle cases where a non-200 status code is returned and the status text is in the response body - otherwise basically any error would end up in the body as a string (thanks, http/2) and result in an unmarshalling error.

Changes:
- added a "status" field, which is populated with the http.Status if included in the response, or the text from the http.StatusCode if not.
- don't error on a 404 (I swear I did that already, so I guess I did it for reals this time)
- don't try to unmarshal the body into a json object if the return isn't a 2XX

I'm sure I'm missing more edge cases, and welcome any suggestions, but hopefully this is better!
---
 src/pkg/domains/api/api.go          | 17 ++++++++++-------
 src/pkg/domains/api/http_request.go | 23 +++++++++++++++--------
 src/pkg/domains/api/types_test.go   | 19 +++++++++++--------
 3 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/src/pkg/domains/api/api.go b/src/pkg/domains/api/api.go
index a8591f1e..a03c28b5 100644
--- a/src/pkg/domains/api/api.go
+++ b/src/pkg/domains/api/api.go
@@ -13,9 +13,10 @@ import (
 )
 
 type APIResponse struct {
-	Status   int
-	Raw      json.RawMessage
-	Response any
+	StatusCode int
+	Status     string
+	Raw        json.RawMessage
+	Response   any
 }
 
 func (a ApiDomain) makeRequests(ctx context.Context) (types.DomainResources, error) {
@@ -63,11 +64,13 @@ func (a ApiDomain) makeRequests(ctx context.Context) (types.DomainResources, err
 				errs = errors.Join(errs, err)
 			}
 			if response != nil {
-				collection[request.Name] = types.DomainResources{
-					"status":   response.Status,
-					"raw":      response.Raw,
-					"response": response.Response,
+				dr := types.DomainResources{
+					"status":     response.Status,
+					"statuscode": response.StatusCode,
+					"raw":        response.Raw,
+					"response":   response.Response,
 				}
+				collection[request.Name] = dr
 			} else {
 				// If the entire response is empty, return a validly empty resource
 				collection[request.Name] = types.DomainResources{"status": 0}
diff --git a/src/pkg/domains/api/http_request.go b/src/pkg/domains/api/http_request.go
index 654f05a6..d2eaeabc 100644
--- a/src/pkg/domains/api/http_request.go
+++ b/src/pkg/domains/api/http_request.go
@@ -46,21 +46,28 @@ func doHTTPReq(ctx context.Context, client http.Client, method string, url url.U
 		return nil, fmt.Errorf("error: %s returned empty response", url.Redacted())
 	}
 	defer res.Body.Close()
-
+	var respObj APIResponse
+	respObj.StatusCode = res.StatusCode
+	if res.Status == "" {
+		respObj.Status = http.StatusText(res.StatusCode)
+	} else {
+		respObj.Status = res.Status
+	}
 	responseData, err := io.ReadAll(res.Body)
 	if err != nil {
 		message.Debugf("error reading response body: %s", err)
 		return nil, err
 	}
 
-	var respObj APIResponse
-	respObj.Raw = responseData
-	respObj.Status = res.StatusCode
-	err = json.Unmarshal(responseData, &respObj.Response)
-	if err != nil {
-		message.Debugf("error unmarshalling response: %s", err)
+	if respObj.StatusCode >= http.StatusOK && respObj.StatusCode < http.StatusMultiStatus {
+		respObj.Raw = responseData
+		err = json.Unmarshal(responseData, &respObj.Response)
+		if err != nil {
+			message.Debugf("error unmarshalling response: %s", err)
+			return nil, err
+		}
 	}
-	return &respObj, err
+	return &respObj, nil
 }
 
 func clientFromOpts(opts *ApiOpts) http.Client {
diff --git a/src/pkg/domains/api/types_test.go b/src/pkg/domains/api/types_test.go
index fa2d3243..3f85e21f 100644
--- a/src/pkg/domains/api/types_test.go
+++ b/src/pkg/domains/api/types_test.go
@@ -118,7 +118,8 @@ func TestGetResources(t *testing.T) {
 				"response": map[string]interface{}{
 					"healthcheck": "ok",
 				},
-				"status": 200,
+				"status":     "200 OK",
+				"statuscode": 200,
 			}}
 		require.Equal(t, want, drs)
 	})
@@ -135,13 +136,15 @@ func TestGetResources(t *testing.T) {
 
 		require.NoError(t, err) // the spec is correct
 		drs, err := api.GetResources(context.Background())
-		require.Error(t, err)
-		require.Equal(t, drs, types.DomainResources{
+		require.NoError(t, err)
+		require.Equal(t, types.DomainResources{
 			apiReqName: types.DomainResources{
-				"status":   400,
-				"raw":      json.RawMessage{},
-				"response": nil,
-			},
-		})
+				"statuscode": 400,
+				"status":     "400 Bad Request",
+				"response":   nil,
+				"raw":        json.RawMessage(nil),
+			}},
+			drs,
+		)
 	})
 }