feat: fail user list pages that omit pagination data - #39
Conversation
HubSpot's user list endpoint can return 200 with results and no `paging` object, which silently truncates the list. GetUsers treated that as "last page" and finished the sync short. Use the SDK's new uhttp.WithPaginationData on the list call so a success response without pagination data becomes an error instead. UsersResponse now implements PaginatedResponse with Paging as a pointer, so a missing object is distinguishable from an empty one (the real last page). Pulls in baton-sdk from ben/CXE-1073, where WithPaginationData lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| require ( | ||
| github.com/conductorone/baton-sdk v0.26.0 | ||
| github.com/conductorone/baton-sdk v0.26.1-0.20260831211052-b6d70299129f |
There was a problem hiding this comment.
🟠 Bug: This pseudo-version pins an unmerged commit. I checked the SDK repo: b6d70299129f is the head of ben/CXE-1073 only, 3 commits ahead of main and not merged, so WithPaginationData does not exist in any released baton-sdk. If that branch is rebased or deleted the pseudo-version becomes unresolvable and go mod download breaks for anyone not building from vendor/. Hold this PR until the SDK branch merges and is tagged, then repin to the release version.
| } | ||
|
|
||
| return userResponse.Results, "", annos, nil | ||
| return userResponse.Results, userResponse.Paging.Next.After, annos, nil |
There was a problem hiding this comment.
🟠 Bug: This makes a load-bearing assumption that 2026-09-beta emits a paging object on the final page. HubSpot's v3 envelope convention is the opposite — paging is present only when a next cursor exists and is omitted on the last page — and this repo's own pre-existing code assumed exactly that (GetDeletedUsers line 275, and the if (userResponse.Paging != PaginationData{}) check this PR deletes). Nothing in the repo, the endpoint comment at line 31-32, or the PR description verifies the empty-paging-on-last-page shape that TestGetUsersLastPage encodes.
If the assumption is wrong, this doesn't fail loudly on truncation — it fails every sync on the last page with ErrMissingPaginationData, across user.go:147, role.go:110, and account.go:87. Please confirm the last-page response against the live endpoint and record it (a fixture or a doc link in the comment). If paging really is omitted on the last page, absence alone can't distinguish "done" from "truncated" — you'd need a signal like len(results) < limit ⇒ legitimate final page, len(results) == limit with no paging ⇒ truncation error.
| // HasPaginationData reports whether HubSpot returned the paging object. The | ||
| // 2026-03 endpoint omits it and silently truncates, so uhttp.WithPaginationData | ||
| // turns that into an error instead of a short sync. | ||
| func (u *UsersResponse) HasPaginationData() bool { | ||
| return u.Paging != nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The comment says "the 2026-03 endpoint omits it", but UsersResponse is only used by GetUsers, which hits listUsersURL() → settings/users/2026-09-beta (line 34). As written it reads as if the guard protects a call that isn't made here. Reword to describe what's being asserted about 2026-09-beta.
Connector PR Review: feat: fail user list pages that omit pagination dataBlocking Issues: 1 | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff (connector source, go.mod/go.sum, and the vendored baton-sdk and dpop updates) for security and correctness. The earlier first-page-only getUsersVars.After gate is gone, so WithPaginationData now runs on every page and that prior finding is addressed. This revision also reverts the user list endpoint from 2026-09-beta back to 2026-03 and deletes the comment documenting that 2026-03 omits paging, which contradicts the new assertion; that is the blocking issue below. Prior notes about the duplicate decode on the first page, GetDeletedUsers keeping the old value check, and the missing non-2xx test case still stand and are not re-flagged here. Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
WithPaginationData only needs the paging envelope, not the payload around it. Pull it into PagingResponse so UsersResponse goes back to being a plain data model and every list endpoint can reuse one PaginatedResponse type instead of growing its own method and pointer field. Both targets decode from the same buffered body, so passing the payload to WithJSONResponse and the envelope to WithPaginationData composes without touching the response stream twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // PagingResponse decodes just the paging envelope every HubSpot list endpoint | ||
| // returns, so uhttp.WithPaginationData can assert the API actually reported a | ||
| // page without caring about the payload alongside it. Paging is a pointer so a | ||
| // missing object (the 2026-03 endpoint omits it and silently truncates) stays | ||
| // distinguishable from an empty one, which is a legitimate last page. |
There was a problem hiding this comment.
🟡 Suggestion: The first sentence ("the paging envelope every HubSpot list endpoint returns") contradicts the parenthetical three lines down ("the 2026-03 endpoint omits it"), and it isn't true of this codebase either — GetTeams/GetRoles go through plain c.get with no paging at all, and GetDeletedUsers decodes its own Paging. Scoping the claim to the endpoint this struct is actually wired to would make the invariant the code enforces easier to trust.
| // PagingResponse decodes just the paging envelope every HubSpot list endpoint | |
| // returns, so uhttp.WithPaginationData can assert the API actually reported a | |
| // page without caring about the payload alongside it. Paging is a pointer so a | |
| // missing object (the 2026-03 endpoint omits it and silently truncates) stays | |
| // distinguishable from an empty one, which is a legitimate last page. | |
| // PagingResponse decodes just the paging envelope returned by the paginated | |
| // user list endpoint, so uhttp.WithPaginationData can assert the API actually | |
| // reported a page without caring about the payload alongside it. Paging is a | |
| // pointer so a missing object (the 2026-03 endpoint omits it and silently | |
| // truncates) stays distinguishable from an empty one, which is a legitimate | |
| // last page. |
Move the paging field back onto UsersResponse and pass the response
itself to WithPaginationData. The option unmarshals the whole body into
its argument, so the receiver has to mirror the top level of the
response -- handing it the inner paging field decodes against the wrong
level and reports "no pagination data" even for a page with a cursor.
getPaginated is gone; get takes DoOptions instead, so the pagination
assertion is just another option on the normal call. The caller's paging
guard comes back as a nil check.
Paging stays a pointer: encoding/json leaves a value field zero whether
the key was absent or empty, and with `!= PaginationData{}` every shape
of last page (absent, {}, and an empty cursor) fails the assertion,
which would error out the final page of every sync.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| c.listUsersURL(), | ||
| &userResponse, | ||
| queryParams, | ||
| uhttp.WithPaginationData(&userResponse), |
There was a problem hiding this comment.
🟡 Suggestion: the body is now unmarshaled into userResponse twice. WithPaginationData(&userResponse) calls WithResponse → WithJSONResponse(&userResponse) internally, and then doRequest appends its own WithJSONResponse(resourceResponse) for the same pointer (client.go:399-401). It is harmless today (identical JSON, same target), but it doubles the decode cost of the largest response in the sync and is easy to misread as two independent decodes. Passing nil as resourceResponse here, or a short comment noting the option already populates the struct, would make the intent explicit.
| } | ||
|
|
||
| if (userResponse.Paging != PaginationData{}) { | ||
| if userResponse.Paging != nil { |
There was a problem hiding this comment.
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.
| func WithPaginationData(response PaginatedResponse) DoOption { | ||
| return func(resp *WrapperResponse) error { | ||
| if response == nil { | ||
| return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil") |
There was a problem hiding this comment.
Oh that's what this does! I like it. If we know we can expect pagination as a guarantee in the API, erroring like this makes sense. We should only use this in connectors we know always will have pagination data!
| var options []uhttp.DoOption | ||
| if getUsersVars.After == "" { | ||
| options = append(options, uhttp.WithPaginationData(&userResponse)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): gating the assertion to the first page is a defensible tradeoff — the reported truncation (2026-03 returning 50 results with no paging) shows up on page 1, and skipping pages 2+ sidesteps the unverified risk that the final page omits paging — but nothing in the code says so. Every reader (and the PR title, "fail user list pages that omit pagination data") will read this as an oversight. Please add a short comment on the if explaining that only the first response is asserted and that a mid-sync page dropping paging will still end the sync silently, so the tradeoff is a recorded decision rather than an accident.
|
|
||
| // 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) { |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): all three tests call GetUsers with GetUsersVars{Limit: 50}, i.e. After == "", so the new first-page-only conditional is entirely unexercised — deleting the if would leave this suite green. Add a case that passes After: "50" against a body with no paging and asserts it returns no error and an empty next page, which pins the "later pages are deliberately not asserted" behavior in place.
|
I may close this one. It looks like the That makes it difficult to distinguish between an API response that is missing the this new WithPaginatedData() doesn't work really well for the hubspot api. |
7047bff to
cc878b7
Compare
| annos, err := c.get( | ||
| ctx, | ||
| c.listUsersURL(), | ||
| c.usersURL(), |
There was a problem hiding this comment.
🟠 Bug: this switches the user list back to settings/users/2026-03 (base used listUsersURL() → 2026-09-beta), and the comment deleted in this same PR states that 2026-03 "omits paging and silently truncates". Pairing that endpoint with the new hard WithPaginationData assertion means every list call — user sync, account grants (pkg/connector/account.go:87), role grants (pkg/connector/role.go:110) — fails with ErrMissingPaginationData, or, if paging is sometimes present, re-introduces the truncation this PR is meant to close. The endpoint change isn't mentioned in the PR description; if it wasn't intentional, restore c.listUsersURL(). (high confidence)
| t.Helper() | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") |
There was a problem hiding this comment.
🟡 Suggestion: the handler replies with the same body for any path, so nothing in the suite pins which endpoint GetUsers calls — that's why the 2026-09-beta → 2026-03 switch on client.go:153 passes green. Capture r.URL.Path in the handler and assert it in at least one case so the list endpoint is covered by tests.
|
|
||
| require ( | ||
| github.com/conductorone/baton-sdk v0.26.0 | ||
| github.com/conductorone/baton-sdk v0.28.0 |
There was a problem hiding this comment.
🟡 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 confirming go mod vendor was re-run after the retarget: the vendored pkg/sdk/version.go reports v0.27.0 under a v0.28.0 module, and go build -mod=vendor won't catch vendored content that doesn't match the released module. (low confidence — the const may simply lag upstream releases.)
2 participants
What
HubSpot's user list endpoint can return
200withresultsand nopagingobject, which silently truncates the list.GetUsersread that as "no next page" and the sync finished short without any signal.This wires up the SDK's new
uhttp.WithPaginationDataon the user list call so a success response missing pagination data fails loudly instead.Changes
go.mod: bumpbaton-sdktov0.26.1-0.20260831211052-b6d70299129f(theben/CXE-1073branch, whereWithPaginationDatalives) + revendor.UsersResponsekeeps itspagingfield and implementsuhttp.PaginatedResponse.Client.gettakes variadicDoOptions, so the pagination assertion is just another option on the normal call — no dedicated paginated-get helper.GetUserspassesuhttp.WithPaginationData(&userResponse)and keeps its paging guard.Two decisions worth reviewing
The option's argument must mirror the top level of the response body.
WithPaginationDatacallsWithResponse(response), which unmarshals the entire body into whatever it is handed. Passing the inner&userResponse.Pagingmakes it look for a top-levelnext, find nothing, and report no pagination data — on a response that has a perfectly good cursor:Pagingis a pointer, not a value.encoding/jsonleaves a value field zero whether the key was absent or present-and-empty, and here those mean opposite things: absent is the truncation bug, empty is a normal last page. WithPaging PaginationDataand!= PaginationData{}, every shape of last page fails the assertion:"paging":{"next":{"after":"50"}}"paging":{"next":{"after":""}}"paging":{}pagingabsentThe value variant errors out the final page of every sync, so the pointer is load-bearing rather than stylistic.
Tests
New
pkg/hubspot/client_test.gocovers three cases againsthttptestservers:paging.next.afterpresentpagingpresent but emptypagingabsentuhttp.ErrMissingPaginationDatago build ./...,go vet ./...,go test ./... -count=1, andgolangci-lint run ./pkg/...are all clean.Notes for reviewers
go.modpoints at an unmerged SDK commit. This shouldn't land until conductorone/baton-sdkben/CXE-1073is merged and released — then the pseudo-version here should be swapped for the tagged version.2026-09-betareturns thepagingobject on the final page (with an empty cursor) rather than omitting it. If it omitspagingwhen there is no next page, the last page of every sync will now fail. I could not verify this without live credentials.GetDeletedUsersandGetUserLastLoginstill do the old empty-Pagingcheck. Left alone since they weren't part of the ask.🤖 Generated with Claude Code