From ead6bf87d9bef041b5ee286928a04f41c948f459 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mislav=20Marohni=C4=87?= <mislav@github.com>
Date: Mon, 6 Dec 2021 19:54:54 +0100
Subject: [PATCH] api: handle HTTP 409 error message from the server

Previously, "errors" field was either an array of strings or an array of
error objects. This covers an additional case when "errors" is a string:

    $ gh api orgs/cli/actions/permissions/repositories
    {
      "message": "Conflict",
      "errors": "Actions are enabled for all repositories",
      "documentation_url": "https://docs.github.com/rest/reference/actions#list-selected-repositories-enabled-for-github-actions-in-an-organization"
    }
---
 pkg/cmd/api/api.go      | 36 ++++++++++++++----
 pkg/cmd/api/api_test.go | 82 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go
index 64d3e07b9..473139faf 100644
--- a/pkg/cmd/api/api.go
+++ b/pkg/cmd/api/api.go
@@ -540,36 +540,58 @@ func parseErrorResponse(r io.Reader, statusCode int) (io.Reader, string, error)
 
 	var parsedBody struct {
 		Message string
-		Errors  []json.RawMessage
+		Errors  json.RawMessage
 	}
 	err = json.Unmarshal(b, &parsedBody)
 	if err != nil {
-		return r, "", err
+		return bodyCopy, "", err
 	}
+
+	if len(parsedBody.Errors) > 0 && parsedBody.Errors[0] == '"' {
+		var stringError string
+		if err := json.Unmarshal(parsedBody.Errors, &stringError); err != nil {
+			return bodyCopy, "", err
+		}
+		if stringError != "" {
+			if parsedBody.Message != "" {
+				return bodyCopy, fmt.Sprintf("%s (%s)", stringError, parsedBody.Message), nil
+			}
+			return bodyCopy, stringError, nil
+		}
+	}
+
 	if parsedBody.Message != "" {
 		return bodyCopy, fmt.Sprintf("%s (HTTP %d)", parsedBody.Message, statusCode), nil
 	}
 
-	type errorMessage struct {
+	if len(parsedBody.Errors) == 0 || parsedBody.Errors[0] != '[' {
+		return bodyCopy, "", nil
+	}
+
+	var errorObjects []json.RawMessage
+	if err := json.Unmarshal(parsedBody.Errors, &errorObjects); err != nil {
+		return bodyCopy, "", err
+	}
+
+	var objectError struct {
 		Message string
 	}
 	var errors []string
-	for _, rawErr := range parsedBody.Errors {
+	for _, rawErr := range errorObjects {
 		if len(rawErr) == 0 {
 			continue
 		}
 		if rawErr[0] == '{' {
-			var objectError errorMessage
 			err := json.Unmarshal(rawErr, &objectError)
 			if err != nil {
-				return r, "", err
+				return bodyCopy, "", err
 			}
 			errors = append(errors, objectError.Message)
 		} else if rawErr[0] == '"' {
 			var stringError string
 			err := json.Unmarshal(rawErr, &stringError)
 			if err != nil {
-				return r, "", err
+				return bodyCopy, "", err
 			}
 			errors = append(errors, stringError)
 		}
diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go
index 408b74674..04da8cc29 100644
--- a/pkg/cmd/api/api_test.go
+++ b/pkg/cmd/api/api_test.go
@@ -1284,3 +1284,85 @@ func Test_processResponse_template(t *testing.T) {
 	`), stdout.String())
 	assert.Equal(t, "", stderr.String())
 }
+
+func Test_parseErrorResponse(t *testing.T) {
+	type args struct {
+		input      string
+		statusCode int
+	}
+	tests := []struct {
+		name       string
+		args       args
+		wantErrMsg string
+		wantErr    bool
+	}{
+		{
+			name: "no error",
+			args: args{
+				input:      `{}`,
+				statusCode: 500,
+			},
+			wantErrMsg: "",
+			wantErr:    false,
+		},
+		{
+			name: "nil errors",
+			args: args{
+				input:      `{"errors":null}`,
+				statusCode: 500,
+			},
+			wantErrMsg: "",
+			wantErr:    false,
+		},
+		{
+			name: "simple error",
+			args: args{
+				input:      `{"message": "OH NOES"}`,
+				statusCode: 500,
+			},
+			wantErrMsg: "OH NOES (HTTP 500)",
+			wantErr:    false,
+		},
+		{
+			name: "errors string",
+			args: args{
+				input:      `{"message": "Conflict", "errors": "Some description"}`,
+				statusCode: 409,
+			},
+			wantErrMsg: "Some description (Conflict)",
+			wantErr:    false,
+		},
+		{
+			name: "errors array of strings",
+			args: args{
+				input:      `{"errors": ["fail1", "asplode2"]}`,
+				statusCode: 500,
+			},
+			wantErrMsg: "fail1\nasplode2",
+			wantErr:    false,
+		},
+		{
+			name: "errors array of objects",
+			args: args{
+				input:      `{"errors": [{"message":"fail1"}, {"message":"asplode2"}]}`,
+				statusCode: 500,
+			},
+			wantErrMsg: "fail1\nasplode2",
+			wantErr:    false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, got1, err := parseErrorResponse(strings.NewReader(tt.args.input), tt.args.statusCode)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parseErrorResponse() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if gotString, _ := ioutil.ReadAll(got); tt.args.input != string(gotString) {
+				t.Errorf("parseErrorResponse() got = %q, want %q", string(gotString), tt.args.input)
+			}
+			if got1 != tt.wantErrMsg {
+				t.Errorf("parseErrorResponse() got1 = %q, want %q", got1, tt.wantErrMsg)
+			}
+		})
+	}
+}