Skip to content

Commit

Permalink
Update APIError struct to use new NullAPIErrorObject type for safety
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@theckman
theckman committed Feb 12, 2021
1 parent bd61b57 commit e430f7b
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 36 deletions.
34 changes: 30 additions & 4 deletions client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,32 @@ 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
}

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.
Expand All @@ -85,7 +111,7 @@ type APIError struct {
//
// This includes messages that should hopefully provide useful context to
// the end user.
APIError *APIErrorObject `json:"error"`
APIError NullAPIErrorObject `json:"error"`

message string
}
Expand All @@ -97,13 +123,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,
)
}

Expand All @@ -124,7 +150,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 {
Expand Down
87 changes: 55 additions & 32 deletions client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down

0 comments on commit e430f7b

Please sign in to comment.