-
Notifications
You must be signed in to change notification settings - Fork 3
feat: fail user list pages that omit pagination data #39
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
base: main
Are you sure you want to change the base?
Changes from all commits
6832cf7
7611e27
b3dba57
f069158
cc878b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,12 +28,6 @@ func (c *Client) usersURL() string { | |
| return c.baseURL.JoinPath("settings/users/2026-03").String() | ||
| } | ||
|
|
||
| // listUsersURL is the paginated list endpoint. 2026-09-beta returns | ||
| // paging.next.after with limit=50; 2026-03 omits paging and silently truncates. | ||
| func (c *Client) listUsersURL() string { | ||
| return c.baseURL.JoinPath("settings/users/2026-09-beta").String() | ||
| } | ||
|
|
||
| func (c *Client) userURL(userID string) string { | ||
| return c.baseURL.JoinPath("settings/users/2026-03", userID).String() | ||
| } | ||
|
|
@@ -59,8 +53,20 @@ func (c *Client) accountLastLoginURL() string { | |
| } | ||
|
|
||
| type UsersResponse struct { | ||
| Results []User `json:"results"` | ||
| Paging PaginationData `json:"paging"` | ||
| Results []User `json:"results"` | ||
| Paging *PaginationData `json:"paging"` | ||
| } | ||
|
|
||
| // HasPaginationData makes UsersResponse a uhttp.PaginatedResponse. | ||
| // WithPaginationData unmarshals the whole body into whatever it is given, so the | ||
| // receiver has to mirror the top level of the response; an inner field would | ||
| // decode against the wrong level and always report nothing. | ||
| // | ||
| // Paging is a pointer because encoding/json leaves a value field zero whether | ||
| // the key was absent or empty, and those mean opposite things here: absent is | ||
| // the endpoint truncating silently, empty is a legitimate last page. | ||
| func (u *UsersResponse) HasPaginationData() bool { | ||
| return u.Paging != nil | ||
| } | ||
|
|
||
| type AccountLoginResponse struct { | ||
|
|
@@ -144,16 +150,17 @@ func (c *Client) GetUsers(ctx context.Context, getUsersVars GetUsersVars) ([]Use | |
|
|
||
| annos, err := c.get( | ||
| ctx, | ||
| c.listUsersURL(), | ||
| c.usersURL(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Bug: this switches the user list back to |
||
| &userResponse, | ||
| queryParams, | ||
| uhttp.WithPaginationData(&userResponse), | ||
| ) | ||
|
|
||
| if err != nil { | ||
| return nil, "", nil, err | ||
| } | ||
|
|
||
| if (userResponse.Paging != PaginationData{}) { | ||
| if userResponse.Paging != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If pagination is nil we should error the connector in a way that alerts us. Maybe we need a new well known error that pages. We expect this to be here. I think it is unlikely this will ever happen again for this service. This was a massive bug in their API, that feels like they will have resolved and will not flap back and forth. We generally should be able to expect that the documented API shape can be relied upon and when it doesn't in any way, not just missing pagination that it blows up in a way we or a self healing agent is notified to go fix right away. |
||
| return userResponse.Results, userResponse.Paging.Next.After, annos, nil | ||
| } | ||
|
|
||
|
|
@@ -330,8 +337,14 @@ func (c *Client) GetUserLastLogin(ctx context.Context, userId string) (*time.Tim | |
| return nil, annos, nil | ||
| } | ||
|
|
||
| func (c *Client) get(ctx context.Context, url string, resourceResponse interface{}, queryParams url.Values) (annotations.Annotations, error) { | ||
| return c.doRequest(ctx, url, http.MethodGet, nil, resourceResponse, queryParams) | ||
| func (c *Client) get( | ||
| ctx context.Context, | ||
| url string, | ||
| resourceResponse interface{}, | ||
| queryParams url.Values, | ||
| doOptions ...uhttp.DoOption, | ||
| ) (annotations.Annotations, error) { | ||
| return c.doRequest(ctx, url, http.MethodGet, nil, resourceResponse, queryParams, doOptions...) | ||
| } | ||
|
|
||
| func (c *Client) put(ctx context.Context, url string, data interface{}, resourceResponse interface{}) (annotations.Annotations, error) { | ||
|
|
@@ -353,6 +366,7 @@ func (c *Client) doRequest( | |
| data interface{}, | ||
| resourceResponse interface{}, | ||
| queryParams url.Values, | ||
| doOptions ...uhttp.DoOption, | ||
| ) (annotations.Annotations, error) { | ||
| parsedURL, err := url.Parse(urlAddress) | ||
| if err != nil { | ||
|
|
@@ -376,7 +390,6 @@ func (c *Client) doRequest( | |
| return nil, err | ||
| } | ||
|
|
||
| var doOptions []uhttp.DoOption | ||
| if resourceResponse != nil { | ||
| doOptions = append(doOptions, uhttp.WithJSONResponse(resourceResponse)) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package hubspot | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "testing" | ||
|
|
||
| "github.com/conductorone/baton-sdk/pkg/uhttp" | ||
| ) | ||
|
|
||
| // newTestClient spins up a server that always replies with body and returns a | ||
| // client pointed at it. Each case gets its own server so uhttp's response cache | ||
| // never serves one case's body to another. | ||
| func newTestClient(t *testing.T, body string) *Client { | ||
| t.Helper() | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: the handler replies with the same body for any path, so nothing in the suite pins which endpoint |
||
| _, _ = w.Write([]byte(body)) | ||
| })) | ||
| t.Cleanup(server.Close) | ||
|
|
||
| client, err := NewClient("token", server.Client(), server.URL+"/") | ||
| if err != nil { | ||
| t.Fatalf("NewClient: %v", err) | ||
| } | ||
| return client | ||
| } | ||
|
|
||
| func TestGetUsersReturnsNextPage(t *testing.T) { | ||
| client := newTestClient(t, `{"results":[{"id":"1","email":"a@example.com"}],"paging":{"next":{"after":"50"}}}`) | ||
|
|
||
| users, nextPage, _, err := client.GetUsers(context.Background(), GetUsersVars{Limit: 50}) | ||
| if err != nil { | ||
| t.Fatalf("GetUsers: %v", err) | ||
| } | ||
| if len(users) != 1 || users[0].Id != "1" { | ||
| t.Errorf("got users %+v, want a single user with id 1", users) | ||
| } | ||
| if nextPage != "50" { | ||
| t.Errorf("got next page %q, want %q", nextPage, "50") | ||
| } | ||
| } | ||
|
|
||
| // The last page still carries a paging object, just without a cursor. | ||
| func TestGetUsersLastPage(t *testing.T) { | ||
| client := newTestClient(t, `{"results":[{"id":"1","email":"a@example.com"}],"paging":{}}`) | ||
|
|
||
| users, nextPage, _, err := client.GetUsers(context.Background(), GetUsersVars{Limit: 50}) | ||
| if err != nil { | ||
| t.Fatalf("GetUsers: %v", err) | ||
| } | ||
| if len(users) != 1 { | ||
| t.Errorf("got %d users, want 1", len(users)) | ||
| } | ||
| if nextPage != "" { | ||
| t.Errorf("got next page %q, want empty", nextPage) | ||
| } | ||
| } | ||
|
|
||
| // A 200 with results but no paging object means the API truncated the list | ||
| // without telling us. That must be an error, not a short sync. | ||
| func TestGetUsersMissingPagingErrors(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion (high confidence): all three tests call |
||
| client := newTestClient(t, `{"results":[{"id":"1","email":"a@example.com"}]}`) | ||
|
|
||
| _, _, _, err := client.GetUsers(context.Background(), GetUsersVars{Limit: 50}) | ||
| if err == nil { | ||
| t.Fatal("GetUsers: expected an error when the response omits paging") | ||
| } | ||
| if !errors.Is(err, uhttp.ErrMissingPaginationData) { | ||
| t.Errorf("got error %v, want it to wrap ErrMissingPaginationData", err) | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
🟡 Suggestion: the PR description says this pins an unmerged SDK branch pseudo-version, but the diff now pins released
v0.28.0— worth updating the description. Also worth confirminggo mod vendorwas re-run after the retarget: the vendoredpkg/sdk/version.goreportsv0.27.0under av0.28.0module, andgo build -mod=vendorwon't catch vendored content that doesn't match the released module. (low confidence — the const may simply lag upstream releases.)