Return EventsAPIV2Error from Events V2 API call. #368

abhishekrb19 · 2021-10-06T05:51:20Z

This patch does the following:

Return EventsAPIV2Error instead of error for the Events V2 API call. This will be consistent with the other REST API calls and callers could leverage the wrapper methods added such as .BadRequest(), .RateLimited(),
and .Temporary(). Add unit tests for the same.
Note that this only works for Events V2 API call. Specifically, Events V1 has a different error structure and slightly different error code. Hence, the naming to avoid disambiguation.
Fix a few typos in the code base in client.go.
Add compile-time type assertions for leveraged built-in interfaces.

theckman

Thank you for raising this PR. If you think there's value in returning a similar error value as the REST API, I'm not against that being added to the repo. That said, the type you're trying to do it with is really designed to work with the REST API only, and so it doesn't feel right to include it on the Events API.

How would you feel about creating a new type, maybe with a name like EventsAPIError?

event_v2.go

theckman · 2021-10-18T02:02:49Z

@abhishekrb19 might you have some time soon to look at the requested changes?

abhishekrb19 · 2021-10-22T04:43:51Z

@theckman, thank you for the review and reminder.
You're right - I missed the documentation of APIError - that it was designed to work only with the REST API. Also, the error response is different for the EventV2 API and returns only a small number of response codes - https://developer.pagerduty.com/api-reference/b3A6Mjc0ODI2Nw-send-an-event-to-pager-duty.

I adapted your recommendation to use EventsAPIV2Error instead of EventsAPIError to avoid ambiguity since there is also an Events V1 API where the structure of JSON response is different.

theckman

I've noticed a few things that should probably be adjusted before merging this PR.

I did also notice that the ManageEvent* method isn't included in this change, but it will need to be. I think to do this we'll need to create separate checkResponse and getErrorFromResponse methods for the Events V2 API calls within the client.go file.

theckman · 2021-11-09T03:21:58Z

event_v2.go

+	// If the API response did not contain an error object, the .Valid field of
+	// EventsAPIV2Error will be false. If .Valid is true, the .ErrorObject field is
+	// valid and should be consulted.
+	EventsAPIV2Error NullEventsAPIV2ErrorObject


Would you be willing to make this struct field APIError. While the names aligned in the other type, it was more to make it clear it wasn't just a standard Go error (by the name).

theckman · 2021-11-09T03:22:15Z

event_v2.go

+}
+
+var _ error = &EventsAPIV2Error{}            // assert that it satisfies the error interface.
+var _ json.Unmarshaler = &EventsAPIV2Error{} // assert that it satisfies the json.Unmarshaler interface.


👍 here too, but also could you change this a little bit:

var _ json.Unmarshaler = (*EventsAPIV2Error)(nil)

theckman · 2021-11-09T03:49:47Z

client.go

@@ -546,7 +550,7 @@ func (c *Client) decodeJSON(resp *http.Response, payload interface{}) error {

 func (c *Client) checkResponse(resp *http.Response, err error) (*http.Response, error) {
 	if err != nil {
-		return resp, fmt.Errorf("Error calling the API endpoint: %v", err)
+		return resp, fmt.Errorf("error calling the API endpoint: %v", err)


Even though I personally agree with this change, could you undo it? I plan to lowercase all of the errors for v2, but the style of this library (within v1) is to capitalize the first word of an error message.

theckman · 2021-11-09T03:53:11Z

event_v2.go

+		e.EventsAPIV2Error.ErrorObject.Message,
+		e.EventsAPIV2Error.ErrorObject.Status,
+		apiErrorsDetailString(e.EventsAPIV2Error.ErrorObject.Errors),
+	)


If the Errors slice is empty, this will print HTTP response failed with status code: CODE, message: MSG, status: STATUS: .

That doesn't seem like a very ideal error message, so could you handle the empty errors case separately to avoid the dangling :?

theckman · 2021-11-09T03:54:30Z

client.go

@@ -165,7 +169,7 @@ func (a APIError) Error() string {
 func apiErrorsDetailString(errs []string) string {
 	switch n := len(errs); n {
 	case 0:
-		panic("errs slice is empty")
+		return ""


I think this should be returned back to a panic(), especially when the the dangling : issue is addressed below and so this should never receive a 0 length slice.

theckman · 2021-11-09T03:55:29Z

client.go

 // thus crashing their whole program.
 type NullAPIErrorObject struct {
 	Valid       bool
 	ErrorObject APIErrorObject
 }

+var _ json.Unmarshaler = &NullAPIErrorObject{} // assert that it satisfies the json.Unmarshaler interface.


👍, but also could you change this a little bit:

var _ json.Unmarshaler = (*NullAPIErrorObject)(nil)

theckman · 2021-11-09T03:57:47Z

event_v2.go

+	message string
+}
+
+var _ error = &EventsAPIV2Error{}            // assert that it satisfies the error interface.


I don't think this assertion is necessary as the compiler will flag it elsewhere. The json.Unmarshaler one is a good check though, because that wouldn't be caught until runtime. Could you remove just this error one?

theckman · 2021-11-09T03:59:11Z

client.go

@@ -135,6 +137,8 @@ type APIError struct {
 	message string
 }

+var _ error = &APIError{} // assert that it implements the error interface.


Like the other I commented on below I'm not sure there's much value in this specific interface check, could you remove it?

theckman · 2021-11-09T04:00:14Z

event_v2.go

+
+// UnmarshalJSON satisfies encoding/json.Unmarshaler.
+func (e *EventsAPIV2Error) UnmarshalJSON(data []byte) error {
+	var eaeo EventsAPIV2ErrorObject


LOL. Say that five times fast...

While you're in there making changes, I'm ok with just eo. But you don't need to change it.

event_v2.go

theckman · 2021-12-09T06:00:11Z

@abhishekrb19 might you have the time to make the requested changes, so that this can make it into v1.5.0?

The original author didn't seem to have the spare cycles to address the feedback on the PR, so I squashed their commits into one and then added this commit to address the feedback.

abhishekrb19 · 2022-04-13T01:19:17Z

@theckman, sorry for not getting back earlier! Thank you for addressing the comments!

abhishekrb19 added 2 commits October 5, 2021 20:08

Fix typos.

d3e5bf6

Return APIError instead of error for V2Event.

1f6f620

theckman requested changes Oct 7, 2021

View reviewed changes

event_v2.go Show resolved Hide resolved

theckman added this to the v1.5.0 milestone Oct 9, 2021

abhishekrb19 added 5 commits October 21, 2021 21:14

Type assertions and better formatting.

b91cc0c

Add EventsAPIV2Error and wrapper types.

064618d

Add unit tests for EventAPIV2Error.

f01b045

Update comment.

f3fe7e5

Merge branch 'master' into api_err_v2event

3ac8942

abhishekrb19 changed the title ~~Return APIError from V2 Event API.~~ Return EventsAPIV2Error from Events V2 API call. Oct 22, 2021

Remove unused import.

e546488

abhishekrb19 requested a review from theckman October 22, 2021 04:43

abhishekrb19 added 5 commits October 21, 2021 21:53

new line.

4270270

error slice can legit be empty - don't panic.

96d05a3

rename and add go doc.

6dd40ed

Change UT names.

850c843

fix typo and few renames.

5adc5b2

theckman requested changes Nov 9, 2021

View reviewed changes

theckman modified the milestones: v1.5.0, v1.6.0 Jan 19, 2022

theckman mentioned this pull request Jan 24, 2022

Return custom error type for Events API (V2) calls #419

Merged

theckman closed this in #419 Feb 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return EventsAPIV2Error from Events V2 API call. #368

Return EventsAPIV2Error from Events V2 API call. #368

abhishekrb19 commented Oct 6, 2021 •

edited

Loading

theckman left a comment

theckman commented Oct 18, 2021

abhishekrb19 commented Oct 22, 2021

theckman left a comment

theckman Nov 9, 2021 •

edited

Loading

theckman Nov 9, 2021

theckman Nov 9, 2021 •

edited

Loading

theckman Nov 9, 2021

theckman Nov 9, 2021

theckman Nov 9, 2021

theckman Nov 9, 2021

theckman Nov 9, 2021

theckman Nov 9, 2021

theckman commented Dec 9, 2021

abhishekrb19 commented Apr 13, 2022

Return EventsAPIV2Error from Events V2 API call. #368

Return EventsAPIV2Error from Events V2 API call. #368

Conversation

abhishekrb19 commented Oct 6, 2021 • edited Loading

theckman left a comment

Choose a reason for hiding this comment

theckman commented Oct 18, 2021

abhishekrb19 commented Oct 22, 2021

theckman left a comment

Choose a reason for hiding this comment

theckman Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman Nov 9, 2021

Choose a reason for hiding this comment

theckman commented Dec 9, 2021

abhishekrb19 commented Apr 13, 2022

abhishekrb19 commented Oct 6, 2021 •

edited

Loading

theckman Nov 9, 2021 •

edited

Loading

theckman Nov 9, 2021 •

edited

Loading