-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update pagination query to conform to API spec #405
Conversation
While looking for API shape mismatches for #389, I stumbled upon a major issue with our pagination parameters across the entire package. This fixes that discrepency, but is done via a pretty intrusive breaking change by removing the embedded `APIListObject` from all `List*Options` types, replacing them with three distinct fields: `Limit`, `Offset`, and `Total`. For the traditional API pagination, there is a `total` URL query parameter that tells the API to include the total count of records in the response. For the query parameter it's a bool flag, with a value of `true` telling the API to include the count. When the response comes back from the API, there is a `total` field in the JSON that is the total count of items in the collection. This value is only set if that `total` query parameter is set to `true` because it's an expensive operation that PagerDuty recommends you don't enable. The problem is we tried to reuse the `APIListObject` struct type for both the pagination URL query parameters, and the pagination fields in the JSON response body. The issue with doing that is for requests the `Total` field needs to be a bool, but within the `APIListObject` it's a `uint` because it was really only designed to be used for response body parsing. So since we're intending to remove the embedded struct types in v2 (#318), it feels like the best course of action here is to move forward with that plan for these paginated query parameter struct types so that we can conform to the PagerDuty API spec. Related to #318 Related to #389
2c184b1
to
2714206
Compare
type Priorities struct { | ||
// ListPrioritiesResponse repreents the API response from PagerDuty when listing | ||
// the configured priorities. | ||
type ListPrioritiesResponse struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced to not make this change, but it feels like a pretty low cost to help make this area of the code more consistent when we're already introducing breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'm a big fan of consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #416 as I remembered a way to do this in a non-breaking way. 👍
@@ -295,7 +309,6 @@ func (c *Client) DeleteOverrideWithContext(ctx context.Context, scheduleID, over | |||
|
|||
// ListOnCallUsersOptions is the data structure used when calling the ListOnCallUsers API endpoint. | |||
type ListOnCallUsersOptions struct { | |||
APIListObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all of these APIListObject
deletes in this file, it looks like they were added to these types by mistake. They are not part of the API specification.
@@ -33,8 +33,24 @@ type ListVendorResponse struct { | |||
|
|||
// ListVendorOptions is the data structure used when calling the ListVendors API endpoint. | |||
type ListVendorOptions struct { | |||
APIListObject | |||
Query string `url:"query,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query
does not appear in the API documentation for this call. So I removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. Thanks for making these changes! 👍
type Priorities struct { | ||
// ListPrioritiesResponse repreents the API response from PagerDuty when listing | ||
// the configured priorities. | ||
type ListPrioritiesResponse struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'm a big fan of consistency.
In #405 I opted to rename one of the types to be more consistent, and in the moment totally forgot about type aliases. A type alias allows you to rename a type in a non-breaking way, by converting the old name to be an alias to the new one. This change does that, and effectively removes one breaking change from v1.5.0.
In #405 I opted to rename one of the types to be more consistent, and in the moment totally forgot about type aliases. A type alias allows you to rename a type in a non-breaking way, by converting the old name to be an alias to the new one. This change does that, and effectively removes one breaking change from v1.5.0. I also do the sane thing to unify the Priority type into one.
Find a way to gracefully avoid one breaking change in #405
While looking for API shape mismatches for #389, I stumbled upon a major issue
with our pagination parameters across the entire package. This fixes that
discrepency, but is done via a pretty intrusive breaking change by removing the
embedded
APIListObject
from allList*Options
types, replacing them withthree distinct fields:
Limit
,Offset
, andTotal
.For the traditional API pagination, there is a
total
URL query parameter thattells the API to include the total count of records in the response. For the
query parameter it's a bool flag, with a value of
true
telling the API toinclude the count.
When the response comes back from the API, there is a
total
field in the JSONthat is the total count of items in the collection. This value is only set if
that
total
query parameter is set totrue
because it's an expensiveoperation that PagerDuty recommends you don't enable.
The problem is we tried to reuse the
APIListObject
struct type for both thepagination URL query parameters, and the pagination fields in the JSON response
body. The issue with doing that is for requests the
Total
field needs to be abool, but within the
APIListObject
it's auint
because it was really only designedto be used for response body parsing.
So since we're intending to remove the embedded struct types in v2 (#318), it
feels like the best course of action here is to move forward with that plan for
these paginated query parameter struct types so that we can conform to the
PagerDuty API spec.
Related to #318
Related to #389