-
Notifications
You must be signed in to change notification settings - Fork 5
feat(uhttp): add WithPaginationData DoOption #1116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4e371d4
0b732ab
b6d7029
cd84047
1fb1b18
056b443
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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" | ||
| ) | ||
|
|
||
|
|
@@ -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 { | ||
|
Comment on lines
+108
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: the final commit (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: this guard only catches an untyped
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
| } | ||
| 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. | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion (confidence: high): this only exercises |
||
| } | ||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: |
||
| 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) | ||
| } | ||
There was a problem hiding this comment.
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 Respoutside the page loop),json.Unmarshalleaves an absentpaginationkey untouched andxml.Unmarshalappends to slices, soHasPaginationData()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."