diff --git a/pkg/uhttp/pagination.go b/pkg/uhttp/pagination.go index 397eb4b9f..fdb54e283 100644 --- a/pkg/uhttp/pagination.go +++ b/pkg/uhttp/pagination.go @@ -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" ) @@ -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. +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 !isSuccessStatusCode(resp.StatusCode) { + return nil + } + + if err := WithResponse(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..72341b02d 100644 --- a/pkg/uhttp/pagination_test.go +++ b/pkg/uhttp/pagination_test.go @@ -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. @@ -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) + } +} + +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", ``) + + 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()) +} + +// 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) +} 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...) }