feat(uhttp): add WithPaginationData DoOption by Bencheng21 · Pull Request #1116 · ConductorOne/baton-sdk

Bencheng21 · 2026-08-31T20:34:39Z

What

Adds an opt-in DoOption for endpoints whose pagination a connector depends on.

The connector's response type — or a dedicated pagination struct — implements a new SDK interface:

type PaginatedResponse interface {
	HasPaginationData() bool
}

WithPaginationData decodes the body into that target via WithResponse (JSON or XML, dispatched on content type), then fails the request if HasPaginationData() reports false.

type PaginationData struct {
	Items      []Item  `json:"items" xml:"items"`
	Pagination *Cursor `json:"pagination" xml:"pagination"`
}

func (p *PaginationData) HasPaginationData() bool { return p.Pagination != nil }

var page PaginationData
resp, err := cli.Do(req, uhttp.WithPaginationData(&page))

Why

Today, an API that stops returning its cursor produces a 200 that decodes cleanly into a struct with a zero-valued pagination field. The connector sees no next page, and the sync ends after the first page — no error, just missing data. This makes that failure loud for connectors that opt in.

This also gives XML connectors their first body-level pagination support. WithNextLinkPagination only reads response headers, so XML APIs paginating in the body decoded with WithXMLResponse and drove the bag by hand, with nothing guarding against the same silent truncation.

Scope

Body-sourced pagination only. An API whose cursor lives in a response header is not covered here beyond the existing WithNextLinkPagination (which handles Link with rel="next" and nothing else); that gap is worth a follow-up.

Notes for reviewers

The error matches ErrMissingPaginationData via errors.Is and carries a FailedPrecondition gRPC code (both, via the existing WrapErrors helper).
Non-2xx responses are skipped. An error response has no pagination data by design; flagging it would bury the real HTTP failure. Pair with WithErrorResponse.
The option decodes the body itself, so it is order-independent relative to other DoOptions. Use it alone on the full response struct, or alongside WithJSONResponse/WithXMLResponse with a separate pagination-only target.
Implementations should report presence, not "has a next page." Keying HasPaginationData off a non-empty cursor string fails the last request of every sync. Key it off presence instead — a pointer field that is nil only when the API omitted the object. Covered by a test; works the same for both encodings, since encoding/xml also leaves a pointer field nil when the element is absent.
The XML target should be a typed struct with xml tags. A map[string]any target will not work: WithResponse routes XML to xml.Unmarshal, which cannot decode into a map — only WithAlwaysXMLResponse and WithGenericResponse go through the xmlMap decoder.
No behavior change for existing callers: purely additive, and WithNextLinkPagination is untouched.

Testing

go test ./pkg/uhttp/... passes. New cases cover, for JSON: cursor present, last page (present-but-empty cursor), missing pagination on a 200, error-response skip, nil target, and end-to-end through Do; for XML: cursor present and missing pagination; plus a content type that is neither, asserting it errors without being misreported as missing pagination.

🤖 Generated with Claude Code

Adds an opt-in DoOption for endpoints whose pagination a connector depends on. The connector's response (or a dedicated pagination struct) implements PaginatedResponse; the option decodes the JSON body into it like WithJSONResponse does, then fails the request if HasPaginationData reports false. Without this, an API that stops returning its cursor produces a 200 that decodes cleanly and a sync that silently ends after the first page. The error matches ErrMissingPaginationData via errors.Is and carries a FailedPrecondition code. Non-2xx responses are skipped so the HTTP error isn't buried under a pagination error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

linear-code · 2026-08-31T20:34:43Z

CXE-1073

github-actions · 2026-08-31T20:38:41Z

+			return WrapErrors(
+				codes.FailedPrecondition,
+				fmt.Sprintf("%T reported no pagination data. status code: %d", response, resp.StatusCode),
+				ErrMissingPaginationData,
+			)


🟡 Suggestion: codes.FailedPrecondition makes this failure non-retryable. pkg/retry/retry.go:61 only retries Unavailable and DeadlineExceeded, so a single transient omission of the pagination object aborts the whole sync action (pkg/sync/parallel_syncer.go:288) instead of retrying the page. The failure this option targets — an API that intermittently drops its cursor — is often transient; codes.Unavailable would still be loud but let the existing syncer retry loop recover. Worth stating the intent either way, since the code choice silently picks the retry policy.

yeah, this error needs investigation, probably needs oncall to take a look at.
non-rety is correct

github-actions · 2026-08-31T20:38:56Z

+		if response == nil {
+			return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil")
+		}


🟡 Suggestion: this guard only catches an untyped nil. The likelier misuse — a typed nil pointer (var p *MyResp; WithPaginationData(p)) or a non-pointer value target — produces a non-nil interface, so it falls through to json.Unmarshal, which returns InvalidUnmarshalError wrapped as failed to unmarshal json response. No crash, but the connector gets a decode error instead of the InvalidArgument this guard is meant to give. A reflect.ValueOf(response) pointer/IsNil check (or a doc note that the target must be a non-nil pointer) would close the gap.

This is used to detect missing pagination data. Connectors are responsible for opting in to it.

github-actions · 2026-08-31T20:39:02Z

+	var body string
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set(ContentType, applicationJSON)
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(body))
+	}))


🟡 Suggestion: body is written by the test goroutine (lines 140/143) and read by the httptest handler goroutine with no synchronization between them. Client-side completion of Do establishes no happens-before edge with the server's handler goroutine, so this is an unsynchronized read/write that -race can flag. Guard it with an atomic.Pointer[string]/mutex, or serve the two bodies from distinct paths.

Decode via WithResponse instead of WithJSONResponse, so the option dispatches on content type and covers XML APIs as well as JSON. JSON behavior is unchanged: WithResponse delegates to WithJSONResponse for a JSON content type. Body-level pagination was previously JSON-only -- XML connectors decoded with WithXMLResponse and drove the bag by hand, with no protection against an API silently dropping its cursor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-08-31T20:50:39Z

General PR Review: feat(uhttp): add WithPaginationData DoOption

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 1e33787f0262.
Review mode: incremental since 1fb1b186
Review run: https://github.com/ConductorOne/baton-sdk/actions/runs/33560185878

Review Summary

The new commit removes the !rv.Elem().IsZero() guard from WithPaginationData plus its four reuse/pre-populated tests, and rewrites the godoc line. This directly addresses the prior finding that the zero-value check made the option order-dependent: Do(req, WithJSONResponse(&page), WithPaginationData(&page)) now works, and the misattributed must be zero-valued error is gone. The full PR diff (pkg/uhttp/pagination.go, pkg/uhttp/pagination_test.go, pkg/uhttp/wrapper.go) was scanned for security and correctness; no dependency, proto, or serialized-state surfaces are touched, and the isSuccessStatusCode extraction in wrapper.go is behavior-preserving at all four call sites. One non-blocking doc gap remains from the removal.

Risk triage (per docs/BUG_CATCHING.md section 2): silent — yes, a defeated check looks identical to a passing one; durable — no, nothing outlives the sync beyond re-syncable c1z contents; uncontrolled dimensions — no; consumer distance — downstream connectors, but the whole surface is new and unreleased, so no caller depends on the removed guard. Worst credible remediation is rung 2 (re-sync). Verdict: MEDIUM, no escalation beyond this review requested.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/uhttp/pagination.go:116 — the rewritten godoc drops the fresh-target contract; a response reused across pages retains the pagination data from the prior page (JSON leaves absent keys untouched, XML appends to slices) and silently defeats the check. Now that this is the responsibility of the caller, document it. Confidence: high on the behavior, medium on whether it warrants a change.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/uhttp/pagination.go`:
- Around line 115-116: The doc comment for `WithPaginationData` now only says
  "response must be a non-nil pointer, since the body is decoded into it." The
  previous wording also stated that the target must be freshly allocated per
  request; that contract still matters even though the runtime zero-value check
  was (correctly) removed. Because `json.Unmarshal` leaves a struct field
  untouched when its key is absent, and `xml.Unmarshal` appends to existing
  slices, a target declared once outside the page loop keeps the non-nil
  pagination pointer from the previous page. `HasPaginationData()` then returns
  true and the option silently passes on precisely the dropped-cursor response
  it was added to catch. Add a line to the godoc such as: "Allocate a fresh
  response per request: a target reused across pages retains the pagination data
  from the prior page and defeats this check." Do not reinstate the runtime
  IsZero guard: it made the option order-dependent when the target is shared
  with another decode option.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-08-31T21:05:13Z

+func TestWithPaginationData_SkipsErrorResponses(t *testing.T) {
+	for _, statusCode := range []int{http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusFound} {
+		var target pagedResponse
+		resp := newPaginationResponse(statusCode, applicationJSON, `{"message":"nope"}`)
+		require.NoError(t, WithPaginationData(&target)(resp), "status %d", statusCode)
+	}


🟡 Suggestion (confidence: high): this only exercises pagedResponse, so nothing covers the documented contract that ParsePaginationHeaders "is called only for successful responses" (pagination.go:122). A connector's parser that assumes a 2xx shape — or one that mutates state it expects to be discarded — would run on 429/500 bodies without any test catching the regression. Adding headerPaged to this loop with an assertion that ParsePaginationHeaders was never invoked (e.g. a called bool on the fixture) closes it cheaply.

github-actions

No blocking issues found.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-08-31T21:15:24Z

+			return err
+		}
+
+		if !response.HasPaginationData() {


🟡 Suggestion (confidence: high on the behavior): this check is only sound if the target is zero-valued for every request. json.Unmarshal leaves a field untouched when the key is absent, so a target declared outside the page loop (var page Response before for { ... cli.Do(req, WithPaginationData(&page)) }) keeps the previous page's non-nil Pagination pointer, HasPaginationData() returns true, and the exact silent truncation this option exists to catch goes undetected. The option can't reset the target through the interface, so this needs to be a stated precondition — allocate a fresh target per request.

hm. this seems like a connector bug if this happens. It looks to me the user/connector should be responsible for it. Similar to a global variable and local variable when we write go code. right? @kans

github-actions · 2026-08-31T21:15:34Z

+type PaginatedResponse interface {
+	HasPaginationData() bool
+}
+
+var ErrMissingPaginationData = errors.New("uhttp: response is missing pagination data")
+
+func WithPaginationData(response PaginatedResponse) DoOption {


🟡 Suggestion: the final commit (b6d7029) strips the doc comments off three new exported SDK symbols, so PaginatedResponse, ErrMissingPaginationData, and WithPaginationData ship to connector authors with no godoc at all. The non-obvious contracts — implement HasPaginationData as presence, not "has a next page" (otherwise every sync fails on its last request), non-2xx responses are skipped so pair with WithErrorResponse, and the fresh-target requirement above — currently exist only in the PR description, which is not visible from an IDE or pkg.go.dev. Restoring those comments on the exported surface is the difference between a self-documenting SDK contract and a footgun.

github-actions

No blocking issues found.

btipling · 2026-09-01T19:03:44Z

+			return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil")
+		}
+
+		if resp.StatusCode < 200 || resp.StatusCode >= 300 {


nit: I would use constants

github-actions · 2026-09-01T19:27:09Z

+		if response == nil {
+			return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil")
+		}


🟡 Suggestion: this response == nil guard misses a typed nil — WithPaginationData((*myResponse)(nil)) still carries a type in the interface, so it passes here and instead surfaces as failed to unmarshal json response: json: Unmarshal(nil *myResponse) rather than InvalidArgument. No panic (json/xml always reject a nil pointer, so HasPaginationData() is never reached), so this is error quality only — but unmarshalXMLToMap in the same package added an explicit guard for exactly this class at wrapper.go:283, and matching it here would keep the two consistent. Confidence: medium.

github-actions

No blocking issues found.

github-actions · 2026-09-01T20:54:19Z

+		if !rv.Elem().IsZero() {
+			return status.Errorf(codes.InvalidArgument, "WithPaginationData: %T must be zero-valued. allocate a fresh response per request", response)
+		}


🟡 Suggestion (confidence: high on the behavior): this zero-value check makes the option order-dependent when the target is shared with another decode option, contradicting the PR description's "order-independent relative to other DoOptions". Do(req, WithJSONResponse(&page), WithPaginationData(&page)) fails with must be zero-valued because Do runs options in slice order (wrapper.go:574-580) and the first one already decoded into page; reversing the two works. The message ("allocate a fresh response per request") also misattributes that case — the caller did allocate fresh. Worth extending the doc comment on line 116 to say the target must not be shared with another decode option, since the pre-existing guidance is to pair this with WithJSONResponse/WithXMLResponse.

github-actions

No blocking issues found.

Callers that declare the response outside the page loop are no longer rejected; only nil and non-pointer responses are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-01T21:19:00Z

+var ErrMissingPaginationData = errors.New("uhttp: response is missing pagination data")
+
+// WithPaginationData decodes the body into response and fails the request if its pagination data is absent, so an API that silently drops its cursor errors instead of ending the sync after one page.
+// response must be a non-nil pointer, since the body is decoded into it.


🟡 Suggestion (confidence: high on behavior, medium on whether it needs a change): dropping the guard is the right call — it made the option order-dependent when sharing a target with another decode option — but this commit also strips the only statement of the fresh-target contract. With reuse (var page Resp outside the page loop), json.Unmarshal leaves an absent pagination key untouched and xml.Unmarshal appends to slices, so HasPaginationData() reports the previous page's cursor as present and the option silently passes on exactly the dropped-cursor response it exists to catch. Since the contract is now the caller's responsibility, please keep it in the godoc, e.g. "allocate a fresh response per request: a target reused across pages retains the prior page's pagination data and defeats this check."

github-actions

No blocking issues found.