From 4e371d46b9a81ddcce7132e0d3d4353c8c4aee7b Mon Sep 17 00:00:00 2001 From: subencheng Date: Mon, 31 Aug 2026 13:34:21 -0700 Subject: [PATCH 1/6] feat(uhttp): add WithPaginationData DoOption 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) --- pkg/uhttp/pagination.go | 45 +++++++++++++ pkg/uhttp/pagination_test.go | 122 +++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 397eb4b9f..21d953263 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -1,8 +1,13 @@ package uhttp import ( + "errors" + "fmt" "strings" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/conductorone/baton-sdk/pkg/pagination" ) @@ -97,3 +102,43 @@ func WithNextLinkPagination(bag *pagination.Bag, config *NextLinkConfig) DoOptio return nil } } + +// PaginatedResponse is implemented by a connector type carrying the pagination +// data of an API response. Implementations should report presence, not a next +// page: returning false on an empty cursor fails the last request of every sync. +type PaginatedResponse interface { + HasPaginationData() bool +} + +var ErrMissingPaginationData = errors.New("uhttp: response is missing pagination data") + +// WithPaginationData decodes a JSON response body into response like +// WithJSONResponse does, then fails the request if response reports no +// pagination data, so an API that drops the cursor doesn't silently truncate a +// sync at the first page. Non-2xx responses are skipped: pair with +// WithErrorResponse. +func WithPaginationData(response PaginatedResponse) DoOption { + return func(resp *WrapperResponse) error { + if response == nil { + return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil") + } + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil + } + + if err := WithJSONResponse(response)(resp); err != nil { + return err + } + + if !response.HasPaginationData() { + return WrapErrors( + codes.FailedPrecondition, + fmt.Sprintf("%T reported no pagination data. status code: %d", response, resp.StatusCode), + ErrMissingPaginationData, + ) + } + + return nil + } +} diff --git a/pkg/uhttp/pagination_test.go b/pkg/uhttp/pagination_test.go index bf7e9c9b9..9122ca289 100644 --- a/pkg/uhttp/pagination_test.go +++ b/pkg/uhttp/pagination_test.go @@ -1,9 +1,14 @@ package uhttp import ( + "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. @@ -21,3 +26,120 @@ 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 { + Items []string `json:"items"` + Pagination *pageCursor `json:"pagination"` +} + +type pageCursor struct { + NextCursor string `json:"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) + } +} + +func TestWithPaginationData_NonJSONResponseErrors(t *testing.T) { + var target pagedResponse + resp := newPaginationResponse(http.StatusOK, "text/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)) + })) + 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()) +} From 0b732aba8e254feee0503431bf5d243cdda7a879 Mon Sep 17 00:00:00 2001 From: subencheng Date: Mon, 31 Aug 2026 13:42:52 -0700 Subject: [PATCH 2/6] feat(uhttp): support XML in WithPaginationData 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) --- pkg/uhttp/pagination.go | 11 +++++------ pkg/uhttp/pagination_test.go | 27 +++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 21d953263..e13963bf1 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -112,11 +112,10 @@ type PaginatedResponse interface { var ErrMissingPaginationData = errors.New("uhttp: response is missing pagination data") -// WithPaginationData decodes a JSON response body into response like -// WithJSONResponse does, then fails the request if response reports no -// pagination data, so an API that drops the cursor doesn't silently truncate a -// sync at the first page. Non-2xx responses are skipped: pair with -// WithErrorResponse. +// WithPaginationData decodes a JSON or XML response body into response like +// WithResponse does, then fails the request if response reports no pagination +// data, so an API that drops the cursor doesn't silently truncate a sync at the +// first page. Non-2xx responses are skipped: pair with WithErrorResponse. func WithPaginationData(response PaginatedResponse) DoOption { return func(resp *WrapperResponse) error { if response == nil { @@ -127,7 +126,7 @@ func WithPaginationData(response PaginatedResponse) DoOption { return nil } - if err := WithJSONResponse(response)(resp); err != nil { + if err := WithResponse(response)(resp); err != nil { return err } diff --git a/pkg/uhttp/pagination_test.go b/pkg/uhttp/pagination_test.go index 9122ca289..a02643c29 100644 --- a/pkg/uhttp/pagination_test.go +++ b/pkg/uhttp/pagination_test.go @@ -1,6 +1,7 @@ package uhttp import ( + "encoding/xml" "net/http" "net/http/httptest" "net/url" @@ -29,12 +30,13 @@ func TestParseLinkHeader(t *testing.T) { // The pagination object is a pointer, so it is nil only when the API omitted it. type pagedResponse struct { - Items []string `json:"items"` - Pagination *pageCursor `json:"pagination"` + 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"` + NextCursor string `json:"next_cursor" xml:"next_cursor"` } func (p *pagedResponse) HasPaginationData() bool { @@ -94,7 +96,24 @@ func TestWithPaginationData_SkipsErrorResponses(t *testing.T) { } } -func TestWithPaginationData_NonJSONResponseErrors(t *testing.T) { +func TestWithPaginationData_XML(t *testing.T) { + const withCursor = `aabc` + const withoutPagination = `a` + + 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", ``) From b6d70299129f933615769829ca3ce99983a5035a Mon Sep 17 00:00:00 2001 From: subencheng Date: Mon, 31 Aug 2026 14:10:52 -0700 Subject: [PATCH 3/6] chore(uhttp): drop doc comments on WithPaginationData Co-Authored-By: Claude Opus 5 (1M context) --- pkg/uhttp/pagination.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index e13963bf1..156b40775 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -103,19 +103,12 @@ func WithNextLinkPagination(bag *pagination.Bag, config *NextLinkConfig) DoOptio } } -// PaginatedResponse is implemented by a connector type carrying the pagination -// data of an API response. Implementations should report presence, not a next -// page: returning false on an empty cursor fails the last request of every sync. type PaginatedResponse interface { HasPaginationData() bool } var ErrMissingPaginationData = errors.New("uhttp: response is missing pagination data") -// WithPaginationData decodes a JSON or XML response body into response like -// WithResponse does, then fails the request if response reports no pagination -// data, so an API that drops the cursor doesn't silently truncate a sync at the -// first page. Non-2xx responses are skipped: pair with WithErrorResponse. func WithPaginationData(response PaginatedResponse) DoOption { return func(resp *WrapperResponse) error { if response == nil { From cd8404795d5d2f4b3f9e0bed2793da631eba9b68 Mon Sep 17 00:00:00 2001 From: subencheng Date: Tue, 1 Sep 2026 12:21:16 -0700 Subject: [PATCH 4/6] constant --- pkg/uhttp/pagination.go | 2 +- pkg/uhttp/wrapper.go | 15 +++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 156b40775..6d6297985 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -115,7 +115,7 @@ func WithPaginationData(response PaginatedResponse) DoOption { return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil") } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { + if !isSuccessStatusCode(resp.StatusCode) { return nil } diff --git a/pkg/uhttp/wrapper.go b/pkg/uhttp/wrapper.go index 228061593..a12ff18d5 100644 --- a/pkg/uhttp/wrapper.go +++ b/pkg/uhttp/wrapper.go @@ -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) @@ -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 { @@ -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 } @@ -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 } @@ -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...) } From 1fb1b186228a6a924767064e3640c97c599067b6 Mon Sep 17 00:00:00 2001 From: subencheng Date: Tue, 1 Sep 2026 13:50:32 -0700 Subject: [PATCH 5/6] reflect --- pkg/uhttp/pagination.go | 14 ++++++ pkg/uhttp/pagination_test.go | 85 ++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 6d6297985..8b28ad686 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -3,6 +3,7 @@ package uhttp import ( "errors" "fmt" + "reflect" "strings" "google.golang.org/grpc/codes" @@ -103,18 +104,31 @@ func WithNextLinkPagination(bag *pagination.Bag, config *NextLinkConfig) DoOptio } } +// 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 to a zero value: allocate a fresh one per request rather than declaring one outside the page loop, or the check below reports it as a caller error. func WithPaginationData(response PaginatedResponse) DoOption { return func(resp *WrapperResponse) error { if response == nil { return status.Error(codes.InvalidArgument, "WithPaginationData: response is nil") } + 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 !rv.Elem().IsZero() { + return status.Errorf(codes.InvalidArgument, "WithPaginationData: %T must be zero-valued. allocate a fresh response per request", response) + } + if !isSuccessStatusCode(resp.StatusCode) { return nil } diff --git a/pkg/uhttp/pagination_test.go b/pkg/uhttp/pagination_test.go index a02643c29..8b5995627 100644 --- a/pkg/uhttp/pagination_test.go +++ b/pkg/uhttp/pagination_test.go @@ -162,3 +162,88 @@ func TestWithPaginationData_ThroughDo(t *testing.T) { 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 +} + +// json.Unmarshal leaves a field untouched when its key is absent, so a response declared +// outside the page loop would report the previous page's cursor as its own. The reuse is +// the caller error, not the missing cursor, so it must not be reported as one. +func TestWithPaginationData_ReusedResponseRejected(t *testing.T) { + var target pagedResponse + + first := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":"abc"}}`) + require.NoError(t, WithPaginationData(&target)(first)) + + second := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["b"]}`) + err := WithPaginationData(&target)(second) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.NotErrorIs(t, err, ErrMissingPaginationData) + require.Equal(t, []string{"a"}, target.Items, "the rejected request must not decode into the target") +} + +// xml.Unmarshal appends to existing slices rather than replacing them, so a reused +// response silently accumulates items on top of inheriting pagination data. +func TestWithPaginationData_ReusedResponseRejectedXML(t *testing.T) { + const body = `ababc` + + var target pagedResponse + require.NoError(t, WithPaginationData(&target)(newPaginationResponse(http.StatusOK, applicationXML, body))) + + err := WithPaginationData(&target)(newPaginationResponse(http.StatusOK, applicationXML, body)) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Equal(t, []string{"a", "b"}, target.Items, "the rejected request must not append to the target") +} + +// Reuse is the common case, but any dirty target is refused: the option cannot tell a +// stale cursor apart from one the caller set deliberately. +func TestWithPaginationData_PrePopulatedResponseRejected(t *testing.T) { + target := pagedResponse{Items: []string{"kept"}} + resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":"abc"}}`) + + err := WithPaginationData(&target)(resp) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.Equal(t, []string{"kept"}, target.Items) +} + +// A dirty target is a caller bug regardless of what the server said, so the zero-value +// check runs ahead of the status check and reports it even on an error response. +func TestWithPaginationData_ReusedResponseRejectedOnErrorResponses(t *testing.T) { + target := pagedResponse{Pagination: &pageCursor{NextCursor: "abc"}} + resp := newPaginationResponse(http.StatusInternalServerError, applicationJSON, `{"message":"nope"}`) + + err := WithPaginationData(&target)(resp) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + require.NotErrorIs(t, err, ErrMissingPaginationData) +} + +// 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) +} From 056b443e7b615eb01f8dac780ad8d03e728e0236 Mon Sep 17 00:00:00 2001 From: subencheng Date: Tue, 1 Sep 2026 14:16:43 -0700 Subject: [PATCH 6/6] feat(uhttp): drop zero-value requirement from WithPaginationData 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) --- pkg/uhttp/pagination.go | 6 +--- pkg/uhttp/pagination_test.go | 55 ------------------------------------ 2 files changed, 1 insertion(+), 60 deletions(-) diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 8b28ad686..fdb54e283 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -113,7 +113,7 @@ type PaginatedResponse interface { 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 to a zero value: allocate a fresh one per request rather than declaring one outside the page loop, or the check below reports it as a caller error. +// response must be a non-nil pointer, since the body is decoded into it. func WithPaginationData(response PaginatedResponse) DoOption { return func(resp *WrapperResponse) error { if response == nil { @@ -125,10 +125,6 @@ func WithPaginationData(response PaginatedResponse) DoOption { return status.Errorf(codes.InvalidArgument, "WithPaginationData: response must be a non-nil pointer, got %T", response) } - if !rv.Elem().IsZero() { - return status.Errorf(codes.InvalidArgument, "WithPaginationData: %T must be zero-valued. allocate a fresh response per request", response) - } - if !isSuccessStatusCode(resp.StatusCode) { return nil } diff --git a/pkg/uhttp/pagination_test.go b/pkg/uhttp/pagination_test.go index 8b5995627..72341b02d 100644 --- a/pkg/uhttp/pagination_test.go +++ b/pkg/uhttp/pagination_test.go @@ -172,61 +172,6 @@ func (p valuePagedResponse) HasPaginationData() bool { return p.Pagination != nil } -// json.Unmarshal leaves a field untouched when its key is absent, so a response declared -// outside the page loop would report the previous page's cursor as its own. The reuse is -// the caller error, not the missing cursor, so it must not be reported as one. -func TestWithPaginationData_ReusedResponseRejected(t *testing.T) { - var target pagedResponse - - first := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":"abc"}}`) - require.NoError(t, WithPaginationData(&target)(first)) - - second := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["b"]}`) - err := WithPaginationData(&target)(second) - require.Error(t, err) - require.Equal(t, codes.InvalidArgument, status.Code(err)) - require.NotErrorIs(t, err, ErrMissingPaginationData) - require.Equal(t, []string{"a"}, target.Items, "the rejected request must not decode into the target") -} - -// xml.Unmarshal appends to existing slices rather than replacing them, so a reused -// response silently accumulates items on top of inheriting pagination data. -func TestWithPaginationData_ReusedResponseRejectedXML(t *testing.T) { - const body = `ababc` - - var target pagedResponse - require.NoError(t, WithPaginationData(&target)(newPaginationResponse(http.StatusOK, applicationXML, body))) - - err := WithPaginationData(&target)(newPaginationResponse(http.StatusOK, applicationXML, body)) - require.Error(t, err) - require.Equal(t, codes.InvalidArgument, status.Code(err)) - require.Equal(t, []string{"a", "b"}, target.Items, "the rejected request must not append to the target") -} - -// Reuse is the common case, but any dirty target is refused: the option cannot tell a -// stale cursor apart from one the caller set deliberately. -func TestWithPaginationData_PrePopulatedResponseRejected(t *testing.T) { - target := pagedResponse{Items: []string{"kept"}} - resp := newPaginationResponse(http.StatusOK, applicationJSON, `{"items":["a"],"pagination":{"next_cursor":"abc"}}`) - - err := WithPaginationData(&target)(resp) - require.Error(t, err) - require.Equal(t, codes.InvalidArgument, status.Code(err)) - require.Equal(t, []string{"kept"}, target.Items) -} - -// A dirty target is a caller bug regardless of what the server said, so the zero-value -// check runs ahead of the status check and reports it even on an error response. -func TestWithPaginationData_ReusedResponseRejectedOnErrorResponses(t *testing.T) { - target := pagedResponse{Pagination: &pageCursor{NextCursor: "abc"}} - resp := newPaginationResponse(http.StatusInternalServerError, applicationJSON, `{"message":"nope"}`) - - err := WithPaginationData(&target)(resp) - require.Error(t, err) - require.Equal(t, codes.InvalidArgument, status.Code(err)) - require.NotErrorIs(t, err, ErrMissingPaginationData) -} - // 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, `{}`)