Request response error handling now unwraps both JsonApi & json errors (#1304)

* handle both jsonapi and json error unwrapping

Author: Uk1288

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Unreleased
 
+## Bug Fixes
+* Improve API error handling to decode both JSON:API error objects and regular JSON errors arrays by @uk1288 [#1304](https://github.com/hashicorp/go-tfe/pull/1304)
+
 ## Enhancements
 * Adds the `ProviderType` field to `AdminSAMLSetting` and `AdminSAMLSettingsUpdateOptions` to support the `provider-type` SAML setting.
 

tfe.go

@@ -1099,22 +1099,33 @@ func checkResponseCode(r *http.Response) error {
 func decodeErrorPayload(r *http.Response) ([]string, error) {
 	// Decode the error payload.
 	var errs []string
-	errPayload := &jsonapi.ErrorsPayload{}
-	err := json.NewDecoder(r.Body).Decode(errPayload)
-	if err != nil || len(errPayload.Errors) == 0 {
+	body, err := io.ReadAll(r.Body)
+	if err != nil {
 		return errs, errors.New(r.Status)
 	}
 
-	// Parse and format the errors.
-	for _, e := range errPayload.Errors {
-		if e.Detail == "" {
-			errs = append(errs, e.Title)
-		} else {
-			errs = append(errs, fmt.Sprintf("%s\n\n%s", e.Title, e.Detail))
+	// attempt JSON:API error payloads unwrapping
+	errPayload := &jsonapi.ErrorsPayload{}
+	if err := json.Unmarshal(body, errPayload); err == nil && len(errPayload.Errors) > 0 {
+		for _, e := range errPayload.Errors {
+			if e.Detail == "" {
+				errs = append(errs, e.Title)
+			} else {
+				errs = append(errs, fmt.Sprintf("%s\n\n%s", e.Title, e.Detail))
+			}
 		}
+		return errs, nil
+	}
+
+	// attempt JSON error payloads unwrapping: like {"errors":["..."]}.
+	var rawErrs struct {
+		Errors []string `json:"errors"`
+	}
+	if err := json.Unmarshal(body, &rawErrs); err == nil && len(rawErrs.Errors) > 0 {
+		return rawErrs.Errors, nil
 	}
 
-	return errs, nil
+	return errs, errors.New(r.Status)
 }
 
 func errorPayloadContains(payloadErrors []string, match string) bool {

tfe_test.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -124,6 +125,82 @@ func Test_unmarshalResponse(t *testing.T) {
 	})
 }
 
+func Test_decodeErrorPayload(t *testing.T) {
+	t.Parallel()
+
+	t.Run("with jsonapi errors payload", func(t *testing.T) {
+		resp := &http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: http.StatusBadRequest,
+			Body: io.NopCloser(bytes.NewBufferString(
+				`{"errors":[{"title":"org name invalid","detail":"Org name is invalid"}]}`,
+			)),
+		}
+
+		errs, err := decodeErrorPayload(resp)
+		require.NoError(t, err)
+		require.Len(t, errs, 1)
+		assert.Equal(t, "org name invalid\n\nOrg name is invalid", errs[0])
+	})
+
+	t.Run("with regular json errors payload", func(t *testing.T) {
+		resp := &http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: http.StatusBadRequest,
+			Body: io.NopCloser(bytes.NewBufferString(
+				`{"errors":["Unsupported GPG Key algorithm. Supported key algorithms are [RSA, DSA]"]}`,
+			)),
+		}
+
+		errs, err := decodeErrorPayload(resp)
+		require.NoError(t, err)
+		require.Len(t, errs, 1)
+		assert.Equal(t, "Unsupported GPG Key algorithm. Supported key algorithms are [RSA, DSA]", errs[0])
+	})
+
+	t.Run("with non-json error body", func(t *testing.T) {
+		resp := &http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: http.StatusBadRequest,
+			Body:       io.NopCloser(bytes.NewBufferString("this is not json")),
+		}
+
+		errs, err := decodeErrorPayload(resp)
+		require.EqualError(t, err, "400 Bad Request")
+		assert.Empty(t, errs)
+	})
+}
+
+func Test_checkResponseCode(t *testing.T) {
+	t.Parallel()
+
+	t.Run("returns regular json error message", func(t *testing.T) {
+		resp := &http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: http.StatusBadRequest,
+			Body: io.NopCloser(bytes.NewBufferString(
+				`{"errors":["Unsupported GPG Key algorithm. Supported key algorithms are [RSA, DSA]"]}`,
+			)),
+		}
+
+		err := checkResponseCode(resp)
+		require.EqualError(t, err, "Unsupported GPG Key algorithm. Supported key algorithms are [RSA, DSA]")
+	})
+
+	t.Run("still maps invalid include message", func(t *testing.T) {
+		resp := &http.Response{
+			Status:     "400 Bad Request",
+			StatusCode: http.StatusBadRequest,
+			Body: io.NopCloser(bytes.NewBufferString(
+				`{"errors":["include parameter is invalid"]}`,
+			)),
+		}
+
+		err := checkResponseCode(resp)
+		assert.ErrorIs(t, err, ErrInvalidIncludeValue)
+	})
+}
+
 func Test_BaseURL(t *testing.T) {
 	t.Parallel()
 	client, err := NewClient(&Config{