Repository: PagerDuty/go-pagerduty
Pull request #272 (2021-02-12 10:34:38)

Update APIError struct to use new NullAPIErrorObject type for safety
I discovered that my initial implementation of the APIError type's NotFound()
method had the potential for triggering a nil pointer dereference panic, which
wouldn't be a great experience for someone dealing with PagerDuty code.

I provided a fix in #271, but in doing so I remembered this pattern from the
`database/sql` package and decided that, while not as nice to use, it was a much
safer API to reduce the chances of the consumer accidentally triggering a panic
in their program.

Since we've yet to release v1.4.0 (i.e., the new APIError type still hasn't been
released) we can still change this without breaking API compatibility
guarantees.

theckman: @echlebek I could use your eyes on this one too, since it was your issue that spawned this API.

Patch:
diff --git a/client.go b/client.go
index 0eed9b4..5023e1d 100644
--- a/client.go
+++ b/client.go
@@ -67,6 +67,33 @@ type APIErrorObject struct {
 	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,
+// thus crashing their whole program.
+type NullAPIErrorObject struct {
+	Valid       bool
+	ErrorObject APIErrorObject
+}
+
+// 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
+	}
+
+	n.ErrorObject = aeo
+	n.Valid = true
+
+	return nil
+}
+
 // APIError represents the error response received when an API call fails. The
 // HTTP response code is set inside of the StatusCode field, with the APIError
 // field being the structured JSON error object returned from the API.
@@ -80,12 +107,14 @@ type APIError struct {
 	// StatusCode is the HTTP response status code
 	StatusCode int `json:"-"`
 
-	// APIError represents the object returned by the API when an error occurs.
-	// If the response has no error object present, this will be nil.
+	// APIError represents the object returned by the API when an error occurs,
+	// which includes messages that should hopefully provide useful context
+	// to the end user.
 	//
-	// This includes messages that should hopefully provide useful context to
-	// the end user.
-	APIError *APIErrorObject `json:"error"`
+	// If the API response did not contain an error object, the .Valid field of
+	// APIError will be false. If .Valid is true, the .ErrorObject field is
+	// valid and should be consulted.
+	APIError NullAPIErrorObject `json:"error"`
 
 	message string
 }
@@ -97,13 +126,13 @@ func (a APIError) Error() string {
 		return a.message
 	}
 
-	if a.APIError == nil {
+	if !a.APIError.Valid {
 		return fmt.Sprintf("HTTP response failed with status code %d and no JSON error object was present", a.StatusCode)
 	}
 
 	return fmt.Sprintf(
 		"HTTP response failed with status code %d, message: %s (code: %d)",
-		a.StatusCode, a.APIError.Message, a.APIError.Code,
+		a.StatusCode, a.APIError.ErrorObject.Message, a.APIError.ErrorObject.Code,
 	)
 }
 
@@ -124,7 +153,7 @@ func (a APIError) Temporary() bool {
 // NotFound returns whether this was an error where it seems like the resource
 // was not found.
 func (a APIError) NotFound() bool {
-	return a.StatusCode == http.StatusNotFound || a.APIError.Code == 2100
+	return a.StatusCode == http.StatusNotFound || (a.APIError.Valid && a.APIError.ErrorObject.Code == 2100)
 }
 
 func newDefaultHTTPClient() *http.Client {


Test patch:
diff --git a/client_test.go b/client_test.go
index c1f01a9..9c5d8cb 100644
--- a/client_test.go
+++ b/client_test.go
@@ -104,10 +104,13 @@ func TestAPIError_RateLimited(t *testing.T) {
 			name: "rate_limited",
 			a: APIError{
 				StatusCode: http.StatusTooManyRequests,
-				APIError: &APIErrorObject{
-					Code:    420,
-					Message: "Enhance Your Calm",
-					Errors:  []string{"Enhance Your Calm"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    420,
+						Message: "Enhance Your Calm",
+						Errors:  []string{"Enhance Your Calm"},
+					},
 				},
 			},
 			want: true,
@@ -116,10 +119,13 @@ func TestAPIError_RateLimited(t *testing.T) {
 			name: "not_found",
 			a: APIError{
 				StatusCode: http.StatusNotFound,
-				APIError: &APIErrorObject{
-					Code:    2100,
-					Message: "Not Found",
-					Errors:  []string{"Not Found"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    2100,
+						Message: "Not Found",
+						Errors:  []string{"Not Found"},
+					},
 				},
 			},
 			want: false,
@@ -147,10 +153,13 @@ func TestAPIError_Temporary(t *testing.T) {
 			name: "rate_limited",
 			a: APIError{
 				StatusCode: http.StatusTooManyRequests,
-				APIError: &APIErrorObject{
-					Code:    420,
-					Message: "Enhance Your Calm",
-					Errors:  []string{"Enhance Your Calm"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    420,
+						Message: "Enhance Your Calm",
+						Errors:  []string{"Enhance Your Calm"},
+					},
 				},
 			},
 			want: true,
@@ -159,10 +168,13 @@ func TestAPIError_Temporary(t *testing.T) {
 			name: "not_found",
 			a: APIError{
 				StatusCode: http.StatusNotFound,
-				APIError: &APIErrorObject{
-					Code:    2100,
-					Message: "Not Found",
-					Errors:  []string{"Not Found"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    2100,
+						Message: "Not Found",
+						Errors:  []string{"Not Found"},
+					},
 				},
 			},
 			want: false,
@@ -204,10 +216,13 @@ func TestAPIError_NotFound(t *testing.T) {
 			name: "rate_limited",
 			a: APIError{
 				StatusCode: http.StatusTooManyRequests,
-				APIError: &APIErrorObject{
-					Code:    420,
-					Message: "Enhance Your Calm",
-					Errors:  []string{"Enhance Your Calm"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    420,
+						Message: "Enhance Your Calm",
+						Errors:  []string{"Enhance Your Calm"},
+					},
 				},
 			},
 			want: false,
@@ -216,10 +231,13 @@ func TestAPIError_NotFound(t *testing.T) {
 			name: "not_found",
 			a: APIError{
 				StatusCode: http.StatusNotFound,
-				APIError: &APIErrorObject{
-					Code:    2100,
-					Message: "Not Found",
-					Errors:  []string{"Not Found"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    2100,
+						Message: "Not Found",
+						Errors:  []string{"Not Found"},
+					},
 				},
 			},
 			want: true,
@@ -228,10 +246,13 @@ func TestAPIError_NotFound(t *testing.T) {
 			name: "not_found_weird_status",
 			a: APIError{
 				StatusCode: http.StatusBadRequest,
-				APIError: &APIErrorObject{
-					Code:    2100,
-					Message: "Not Found",
-					Errors:  []string{"Not Found"},
+				APIError: NullAPIErrorObject{
+					Valid: true,
+					ErrorObject: APIErrorObject{
+						Code:    2100,
+						Message: "Not Found",
+						Errors:  []string{"Not Found"},
+					},
 				},
 			},
 			want: true,
@@ -240,10 +261,12 @@ func TestAPIError_NotFound(t *testing.T) {
 			name: "not_found_weird_error_code",
 			a: APIError{
 				StatusCode: http.StatusNotFound,
-				APIError: &APIErrorObject{
-					Code:    2101,
-					Message: "Not Found",
-					Errors:  []string{"Not Found"},
+				APIError: NullAPIErrorObject{
+					ErrorObject: APIErrorObject{
+						Code:    2101,
+						Message: "Not Found",
+						Errors:  []string{"Not Found"},
+					},
 				},
 			},
 			want: true,