Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions pkg/uhttp/pagination.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
package uhttp

import (
"errors"
"fmt"
"reflect"
"strings"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/conductorone/baton-sdk/pkg/pagination"
)

Expand Down Expand Up @@ -97,3 +103,44 @@ func WithNextLinkPagination(bag *pagination.Bag, config *NextLinkConfig) DoOptio
return nil
}
}

// PaginatedResponse is implemented by response types that can report whether the API returned the pagination data the caller needs to fetch the next page.
type PaginatedResponse interface {
HasPaginationData() bool
}

// ErrMissingPaginationData is the sentinel returned when a successful response decoded fine but carried no pagination data; match it with errors.Is.
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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."

func WithPaginationData(response PaginatedResponse) DoOption {
Comment on lines +108 to +117

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved

return func(resp *WrapperResponse) error {
if response == nil {
return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil")
}
Comment on lines +119 to +121

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +119 to +121

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed.


rv := reflect.ValueOf(response)
if rv.Kind() != reflect.Pointer || rv.IsNil() {
return status.Errorf(codes.InvalidArgument, "WithPaginationData: response must be a non-nil pointer, got %T", response)
}

if !isSuccessStatusCode(resp.StatusCode) {
return nil
}

if err := WithResponse(response)(resp); err != nil {
return err
}

if !response.HasPaginationData() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@Bencheng21 Bencheng21 Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return nil
}
}
171 changes: 171 additions & 0 deletions pkg/uhttp/pagination_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
package uhttp

import (
"encoding/xml"
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

// Parses the link header and returns a map of rel values to URLs.
Expand All @@ -21,3 +27,168 @@ func TestParseLinkHeader(t *testing.T) {
require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=515", links["last"])
require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=1", links["first"])
}

// The pagination object is a pointer, so it is nil only when the API omitted it.
type pagedResponse struct {
XMLName xml.Name `json:"-" xml:"response"`
Items []string `json:"items" xml:"items"`
Pagination *pageCursor `json:"pagination" xml:"pagination"`
}

type pageCursor struct {
NextCursor string `json:"next_cursor" xml:"next_cursor"`
}

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

func newPaginationResponse(statusCode int, contentType string, body string) *WrapperResponse {
header := http.Header{}
if contentType != "" {
header.Set(ContentType, contentType)
}
return &WrapperResponse{
Header: header,
Status: http.StatusText(statusCode),
StatusCode: statusCode,
Body: []byte(body),
}
}

func TestWithPaginationData_DecodesWhenPresent(t *testing.T) {
var target pagedResponse
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":"abc"}}`)

require.NoError(t, WithPaginationData(&target)(resp))
require.Equal(t, []string{"a"}, target.Items)
require.Equal(t, "abc", target.Pagination.NextCursor)
}

// The last page is not an error: the object is there, its cursor is just empty.
func TestWithPaginationData_LastPageIsNotAnError(t *testing.T) {
var target pagedResponse
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":""}}`)

require.NoError(t, WithPaginationData(&target)(resp))
require.Equal(t, "", target.Pagination.NextCursor)
}

// The case this option exists for: still 200 with items, but no pagination data.
func TestWithPaginationData_MissingPaginationErrors(t *testing.T) {
var target pagedResponse
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"]}`)

err := WithPaginationData(&target)(resp)
require.Error(t, err)
require.ErrorIs(t, err, ErrMissingPaginationData)
require.Equal(t, codes.FailedPrecondition, status.Code(err))
require.Equal(t, []string{"a"}, target.Items, "the body should still be decoded")
}

// Error responses carry no pagination data by design; the HTTP error is the real
// failure and must not be buried under a pagination error.
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)
}
Comment on lines +91 to +96

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

}

func TestWithPaginationData_XML(t *testing.T) {
const withCursor = `<response><items>a</items><pagination><next_cursor>abc</next_cursor></pagination></response>`
const withoutPagination = `<response><items>a</items></response>`

var target pagedResponse
require.NoError(t, WithPaginationData(&target)(newPaginationResponse(http.StatusOK, applicationXML, withCursor)))
require.Equal(t, []string{"a"}, target.Items)
require.Equal(t, "abc", target.Pagination.NextCursor)

var missing pagedResponse
err := WithPaginationData(&missing)(newPaginationResponse(http.StatusOK, applicationXML, withoutPagination))
require.ErrorIs(t, err, ErrMissingPaginationData)
require.Equal(t, codes.FailedPrecondition, status.Code(err))
require.Equal(t, []string{"a"}, missing.Items, "the body should still be decoded")
}

// Neither JSON nor XML: still an error, but not reported as missing pagination.
func TestWithPaginationData_UnsupportedContentTypeErrors(t *testing.T) {
var target pagedResponse
resp := newPaginationResponse(http.StatusOK, "text/html", `<html></html>`)

err := WithPaginationData(&target)(resp)
require.Error(t, err)
require.NotErrorIs(t, err, ErrMissingPaginationData, "a content-type change should not be reported as missing pagination")
}

func TestWithPaginationData_NilResponse(t *testing.T) {
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{}`)

err := WithPaginationData(nil)(resp)
require.Error(t, err)
require.Equal(t, codes.InvalidArgument, status.Code(err))
}

// End to end through Do.
func TestWithPaginationData_ThroughDo(t *testing.T) {
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))
}))
Comment on lines +135 to +140

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

defer ts.Close()

client, err := NewBaseHttpClientWithContext(ctx, http.DefaultClient)
require.NoError(t, err)
u, err := url.Parse(ts.URL)
require.NoError(t, err)

do := func() error {
req, err := client.NewRequest(ctx, http.MethodPost, u, WithAcceptJSONHeader())
require.NoError(t, err)
var target pagedResponse
resp, err := client.Do(req, WithPaginationData(&target))
if resp != nil {
defer resp.Body.Close()
}
return err
}

body = `{"items":["a"]}`
require.ErrorIs(t, do(), ErrMissingPaginationData)

body = `{"items":["a"],"pagination":{"next_cursor":"abc"}}`
require.NoError(t, do())
}

// A value receiver, so a non-pointer of this type still satisfies PaginatedResponse.
type valuePagedResponse struct {
Pagination *pageCursor `json:"pagination"`
}

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

// A typed nil is not caught by an `any == nil` check, since the interface still carries a type.
func TestWithPaginationData_TypedNilResponse(t *testing.T) {
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{}`)

err := WithPaginationData((*pagedResponse)(nil))(resp)
require.Error(t, err)
require.Equal(t, codes.InvalidArgument, status.Code(err))
require.NotErrorIs(t, err, ErrMissingPaginationData)
}

// Nothing can be decoded into a non-pointer, so reject it rather than reporting the
// zero value's missing pagination.
func TestWithPaginationData_NonPointerResponse(t *testing.T) {
resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"pagination":{"next_cursor":"abc"}}`)

err := WithPaginationData(valuePagedResponse{})(resp)
require.Error(t, err)
require.Equal(t, codes.InvalidArgument, status.Code(err))
require.NotErrorIs(t, err, ErrMissingPaginationData)
}
15 changes: 11 additions & 4 deletions pkg/uhttp/wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func WithAlwaysXMLResponse(response any) DoOption {
if resp.StatusCode == http.StatusNoContent {
return nil
}
if resp.StatusCode >= 200 && resp.StatusCode < 300 && len(resp.Body) == 0 {
if isSuccessStatusCode(resp.StatusCode) && len(resp.Body) == 0 {
return nil
}
return unmarshalXMLToMap(genericResponse, resp)
Expand Down Expand Up @@ -317,6 +317,13 @@ type ErrorResponse interface {
Message() string
}

// isSuccessStatusCode reports whether code is in the 2xx success class.
// http.StatusOK (200) is the inclusive lower bound and
// http.StatusMultipleChoices (300) the exclusive upper bound.
func isSuccessStatusCode(code int) bool {
return code >= http.StatusOK && code < http.StatusMultipleChoices
}

// GrpcCodeFromHTTPStatus maps an HTTP status code to the appropriate gRPC status code.
func GrpcCodeFromHTTPStatus(httpStatus int) codes.Code {
switch httpStatus {
Expand Down Expand Up @@ -349,7 +356,7 @@ func GrpcCodeFromHTTPStatus(httpStatus int) codes.Code {

func WithErrorResponse(resource ErrorResponse) DoOption {
return func(resp *WrapperResponse) error {
if resp.StatusCode < 300 {
if resp.StatusCode < http.StatusMultipleChoices {
return nil
}

Expand Down Expand Up @@ -425,7 +432,7 @@ func WithGenericResponse(response *map[string]any) DoOption {
return nil
}

if resp.StatusCode >= 200 && resp.StatusCode < 300 && len(resp.Body) == 0 {
if isSuccessStatusCode(resp.StatusCode) && len(resp.Body) == 0 {
return nil
}

Expand Down Expand Up @@ -587,7 +594,7 @@ func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Respo
}
}

if resp.StatusCode < 200 || resp.StatusCode >= 300 {
if !isSuccessStatusCode(resp.StatusCode) {
grpcCode := GrpcCodeFromHTTPStatus(resp.StatusCode)
return resp, WrapErrorsWithRateLimitInfo(grpcCode, resp, optErrs...)
}
Expand Down
Loading