Backport PR #382 to release-1.4.x branch

The bug this fixes was introduced as part of v1.4.0, so we need to backport it
into the v1.4.x release line. Because the master branch has diverged towards
v1.5.0, we need to manually backport this to the release-1.4.x branch.

Original commit message (0933613):

The PagerDuty REST API documentation[1] indicates that the errors field of an
error response is an array of error strings. However, as shown in #339 the API
sometimes violates that specification and instead returns the errors field as
a string.

There is a PagerDuty customer support ticket open[2] to to address this issue, but
there is currently no ETA on the resolution. As such we are going to create this
workaround to properly parse the invalid responses and return the proper error
type to callers.

Closes #339

[1] https://developer.pagerduty.com/docs/ZG9jOjExMDI5NTYz-errors
[2] https://tickets.pagerduty.com/hc/requests/341221

client.go

@@ -70,14 +70,24 @@ type APIErrorObject struct {
 	Errors  []string `json:"errors,omitempty"`
 }
 
+// fallbackAPIErrorObject is a shim to solve this issue:
+// https://github.com/PagerDuty/go-pagerduty/issues/339
+//
+// TODO: remove when PagerDuty engineering confirms bugfix to the REST API
+type fallbackAPIErrorObject struct {
+	Code    int    `json:"code,omitempty"`
+	Message string `json:"message,omitempty"`
+	Errors  string `json:"errors,omitempty"`
+}
+
 // NullAPIErrorObject is a wrapper around the APIErrorObject type. If the Valid
 // field is true, the API response included a structured error JSON object. This
 // structured object is then set on the ErrorObject field.
 //
 // While the PagerDuty REST API is documented to always return the error object,
 // we assume it's possible in exceptional failure modes for this to be omitted.
 // As such, this wrapper type provides us a way to check if the object was
-// provided while avoiding cosnumers accidentally missing a nil pointer check,
+// provided while avoiding consumer accidentally missing a nil pointer check,
 // thus crashing their whole program.
 type NullAPIErrorObject struct {
 	Valid       bool
@@ -87,8 +97,27 @@ type NullAPIErrorObject struct {
 // UnmarshalJSON satisfies encoding/json.Unmarshaler
 func (n *NullAPIErrorObject) UnmarshalJSON(data []byte) error {
 	var aeo APIErrorObject
-	if err := json.Unmarshal(data, &aeo); err != nil {
-		return err
+
+	err := json.Unmarshal(data, &aeo)
+	if err != nil {
+		terr, ok := err.(*json.UnmarshalTypeError)
+		if !ok {
+			return err
+		}
+
+		//
+		// see https://github.com/PagerDuty/go-pagerduty/issues/339
+		//
+		var faeo fallbackAPIErrorObject
+
+		if err := json.Unmarshal(data, &faeo); err != nil {
+			// still failed, so return the original error
+			return terr
+		}
+
+		aeo.Code = faeo.Code
+		aeo.Message = faeo.Message
+		aeo.Errors = []string{faeo.Errors}
 	}
 
 	n.ErrorObject = aeo

client_test.go

@@ -332,3 +332,60 @@ func TestAPIError_NotFound(t *testing.T) {
 		})
 	}
 }
+
+func TestNullAPIErrorObject_UnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name string
+		json string
+		err  string
+		want *NullAPIErrorObject
+	}{
+		{
+			name: "error_per_api_spec",
+			json: `{"code":42,"message":"test message","errors":["first error","second error"]}`,
+			want: &NullAPIErrorObject{
+				Valid: true,
+				ErrorObject: APIErrorObject{
+					Code:    42,
+					Message: "test message",
+					Errors: []string{
+						"first error",
+						"second error",
+					},
+				},
+			},
+		},
+		{
+			name: "issue_339",
+			json: `{"code":84,"message":"other message","errors":"only error"}`,
+			want: &NullAPIErrorObject{
+				Valid: true,
+				ErrorObject: APIErrorObject{
+					Code:    84,
+					Message: "other message",
+					Errors: []string{
+						"only error",
+					},
+				},
+			},
+		},
+		{
+			name: "returns_errors",
+			json: `{"code":"42","message":"test message","errors":"first error"}`,
+			err:  "json: cannot unmarshal string into Go struct field APIErrorObject.code of type int",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := &NullAPIErrorObject{}
+
+			err := json.Unmarshal([]byte(tt.json), &got)
+			if !testErrCheck(t, "*NullAPIErrorObject.UnmarshalJSON()", tt.err, err) {
+				return
+			}
+
+			testEqual(t, tt.want, got)
+		})
+	}
+}