-
Notifications
You must be signed in to change notification settings - Fork 5
HTTP wrapper: Add helper for link header pagination #355
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
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 |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package uhttp | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "github.com/conductorone/baton-sdk/pkg/pagination" | ||
| ) | ||
|
|
||
| /* | ||
| * Uhttp pagination handling. | ||
| * There are three common types of pagination: | ||
| * 1. NextLink: http header containing a url to fetch the next page | ||
| * 2. Cursor: http body containing a token to fetch the next page | ||
| * 3. Offset: offset + limit to fetch the next page | ||
| * - Subset of offset: incremental page numbers | ||
| * | ||
| * All of these helper functions take a bag and push the next page state on (if there is a next page). | ||
| */ | ||
|
|
||
| type NextLinkConfig struct { | ||
| Header string `json:"header,omitempty"` // HTTP header containing the next link. Defaults to "link". | ||
| Rel string `json:"rel,omitempty"` // The rel value to look for in the link header. Defaults to "next". | ||
| ResourceTypeID string `json:"resource_type_id,omitempty"` | ||
| ResourceID string `json:"resource_id,omitempty"` | ||
| } | ||
|
|
||
| // Parses the link header and returns a map of rel values to URLs. | ||
| func parseLinkHeader(header string) (map[string]string, error) { | ||
| if header == "" { | ||
| // Empty header is fine, it just means there are no more pages. | ||
| return nil, nil | ||
| } | ||
|
|
||
| links := make(map[string]string) | ||
| headerLinks := strings.Split(header, ",") | ||
| for _, headerLink := range headerLinks { | ||
| linkParts := strings.Split(headerLink, ";") | ||
gontzess marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if len(linkParts) < 2 { | ||
| continue | ||
| } | ||
| linkUrl := strings.TrimSpace(linkParts[0]) | ||
| linkUrl = strings.Trim(linkUrl, "<>") | ||
| var relValue string | ||
| for _, rel := range linkParts[1:] { | ||
| rel = strings.TrimSpace(rel) | ||
| relParts := strings.Split(rel, "=") | ||
| if len(relParts) < 2 { | ||
| continue | ||
| } | ||
| if relParts[0] == "rel" { | ||
| relValue = strings.Trim(relParts[1], "\"") | ||
| break | ||
| } | ||
| } | ||
| if relValue == "" { | ||
| continue | ||
| } | ||
| links[relValue] = linkUrl | ||
| } | ||
|
|
||
| return links, nil | ||
| } | ||
|
|
||
| // WithNextLinkPagination handles nextlink pagination. | ||
| // The config is optional, and if not provided, the default config will be used. | ||
| func WithNextLinkPagination(bag *pagination.Bag, config *NextLinkConfig) DoOption { | ||
| return func(resp *WrapperResponse) error { | ||
| if config == nil { | ||
| config = &NextLinkConfig{ | ||
| Header: "link", | ||
| Rel: "next", | ||
| } | ||
| } | ||
| if config.Header == "" { | ||
| config.Header = "link" | ||
| } | ||
| if config.Rel == "" { | ||
| config.Rel = "next" | ||
| } | ||
| nextLinkVal := resp.Header.Get(config.Header) | ||
| if nextLinkVal == "" { | ||
| return nil | ||
| } | ||
| links, err := parseLinkHeader(nextLinkVal) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| nextLink := links[config.Rel] | ||
| if nextLink == "" { | ||
| return nil | ||
| } | ||
| bag.Push(pagination.PageState{ | ||
| Token: nextLink, | ||
| ResourceTypeID: config.ResourceTypeID, | ||
| ResourceID: config.ResourceID, | ||
| }) | ||
| return nil | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,23 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package uhttp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/stretchr/testify/require" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Parses the link header and returns a map of rel values to URLs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestParseLinkHeader(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //nolint:revive // This is fine | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Example link header value: <https://api.github.com/repositories/1300192/issues?page=2>; rel="prev", <https://api.github.com/repositories/1300192/issues?page=4>; rel="next", <https://api.github.com/repositories/1300192/issues?page=515>; rel="last", <https://api.github.com/repositories/1300192/issues?page=1>; rel="first" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //nolint:revive // This is fine | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| header := `<https://api.github.com/repositories/1300192/issues?page=2>; rel="prev", <https://api.github.com/repositories/1300192/issues?page=4>; rel="next", <https://api.github.com/repositories/1300192/issues?page=515>; rel="last", <https://api.github.com/repositories/1300192/issues?page=1>; rel="first"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| links, err := parseLinkHeader(header) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.Nil(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=2", links["prev"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=4", links["next"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+23
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. 🛠️ Refactor suggestion Enhance test coverage with edge cases. The test effectively verifies the happy path, but would benefit from additional test cases:
Also, consider using func TestParseLinkHeader(t *testing.T) {
//nolint:revive // This is fine
// Example link header value: <https://api.github.com/repositories/1300192/issues?page=2>; rel="prev", <https://api.github.com/repositories/1300192/issues?page=4>; rel="next", <https://api.github.com/repositories/1300192/issues?page=515>; rel="last", <https://api.github.com/repositories/1300192/issues?page=1>; rel="first"
//nolint:revive // This is fine
header := `<https://api.github.com/repositories/1300192/issues?page=2>; rel="prev", <https://api.github.com/repositories/1300192/issues?page=4>; rel="next", <https://api.github.com/repositories/1300192/issues?page=515>; rel="last", <https://api.github.com/repositories/1300192/issues?page=1>; rel="first"`
links, err := parseLinkHeader(header)
- require.Nil(t, err)
+ require.NoError(t, err)
require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=2", links["prev"])
require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=4", links["next"])
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"])
+
+ // Test empty header
+ links, err = parseLinkHeader("")
+ require.NoError(t, err)
+ require.Nil(t, links)
+
+ // Test malformed header
+ malformedHeader := `<https://api.github.com/repositories/1300192/issues?page=2>; rel="prev" <https://api.github.com/repositories/1300192/issues?page=4>`
+ links, err = parseLinkHeader(malformedHeader)
+ require.NoError(t, err)
+ require.Len(t, links, 1)
+
+ // Test duplicate rel values
+ duplicateRelHeader := `<https://api.github.com/repositories/1300192/issues?page=2>; rel="next", <https://api.github.com/repositories/1300192/issues?page=4>; rel="next"`
+ links, err = parseLinkHeader(duplicateRelHeader)
+ require.NoError(t, err)
+ require.Equal(t, "https://api.github.com/repositories/1300192/issues?page=4", links["next"], "Should use the last occurrence for duplicate rel values")
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
i'm not sure how common it is (maybe its not very), but for the Rootly API which followed the JSONAPI spec,
linksare included as part of the body, separate fromdata: https://github.com/ConductorOne/baton-rootly/blob/main/pkg/connector/client/models.go#L36thought i'd mention for awareness