From c4ff3c0bdc60fd5c2776f9a1e2676aaebdc10911 Mon Sep 17 00:00:00 2001 From: arreyder Date: Wed, 2 Sep 2026 21:49:39 -0500 Subject: [PATCH] fix(client): page through SearchEntitlements results SearchEntitlements sent page_size=100 and then made exactly one call, never reading NextPageToken, so every entitlement past the first page was silently dropped. The server has always documented this endpoint as paginated; it just never enforced it, so the bug was invisible. Loop until the token is empty, converting each page before moving on. The per-page conversion is load-bearing: the response's expanded array is indexed per response, so ExpandedMap offsets only resolve against the page they came from. Merging raw pages first would attach the wrong app or resource to each entitlement. Stop on an empty token rather than a short page. The server applies granted-status filtering after it cuts the page, so an intermediate page can be empty while later pages still hold results. A nil list is now an empty page rather than an error. The wire format omits an empty repeated field, so a search matching nothing arrives as a nil list and previously failed with "search-entitlements: list is nil". Co-Authored-By: Claude Opus 5 (1M context) --- pkg/client/entitlement.go | 101 +++++++++---- pkg/client/entitlement_pagination_test.go | 168 ++++++++++++++++++++++ 2 files changed, 242 insertions(+), 27 deletions(-) create mode 100644 pkg/client/entitlement_pagination_test.go diff --git a/pkg/client/entitlement.go b/pkg/client/entitlement.go index a23c63e2..2bf77dae 100644 --- a/pkg/client/entitlement.go +++ b/pkg/client/entitlement.go @@ -3,6 +3,7 @@ package client import ( "context" "errors" + "fmt" "net/http" "github.com/conductorone/conductorone-sdk-go/pkg/models/operations" @@ -97,36 +98,80 @@ func (e *ExpandableEntitlementWithBindings) SetPath(pathname string, value int) e.ExpandedMap[pathname] = value } +// maxRepeatedSearchEntitlementsPageToken bounds how many times the server may +// hand back a page token identical to the one just sent before we treat paging +// as stuck. Without it a server that never advances the token spins forever. +const maxRepeatedSearchEntitlementsPageToken = 8 + func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitlementsFilter) ([]*EntitlementWithBindings, error) { - // TODO(morgabra) Pagination // TODO(morgabra) Should we abstract the OpenAPI objects from the rest of cone? Kinda... no? But they aren't typed... - req := shared.RequestCatalogSearchServiceSearchEntitlementsRequest{ - EntitlementAlias: stringPtr(filter.EntitlementAlias), - GrantedStatus: filter.GrantedStatus.ToPointer(), - PageSize: intPtr(100), - PageToken: nil, - Query: stringPtr(filter.Query), - AppDisplayName: stringPtr(filter.AppDisplayName), - IncludeDeleted: &filter.IncludeDeleted, - ExpandMask: &filter.AppEntitlementExpandMask, - } - resp, err := c.sdk.RequestCatalogSearch.SearchEntitlements(ctx, &req) - if err != nil { - return nil, err - } + rv := make([]*EntitlementWithBindings, 0) + pageToken := "" + repeatedToken := 0 - if err := NewHTTPError(resp.RawResponse); err != nil { - return nil, err + for { + req := shared.RequestCatalogSearchServiceSearchEntitlementsRequest{ + EntitlementAlias: stringPtr(filter.EntitlementAlias), + GrantedStatus: filter.GrantedStatus.ToPointer(), + PageSize: intPtr(100), + PageToken: stringPtr(pageToken), + Query: stringPtr(filter.Query), + AppDisplayName: stringPtr(filter.AppDisplayName), + IncludeDeleted: &filter.IncludeDeleted, + ExpandMask: &filter.AppEntitlementExpandMask, + } + resp, err := c.sdk.RequestCatalogSearch.SearchEntitlements(ctx, &req) + if err != nil { + return nil, err + } + + if err := NewHTTPError(resp.RawResponse); err != nil { + return nil, err + } + + page, err := convertSearchEntitlementsPage(resp.RequestCatalogSearchServiceSearchEntitlementsResponse) + if err != nil { + return nil, err + } + rv = append(rv, page...) + + // Stop on an empty token, never on an empty page: the server filters + // granted status after it cuts the page, so a page can come back short or + // empty while later pages still hold results. + nextPageToken := StringFromPtr(resp.RequestCatalogSearchServiceSearchEntitlementsResponse.NextPageToken) + if nextPageToken == "" { + break + } + if nextPageToken == pageToken { + repeatedToken++ + if repeatedToken >= maxRepeatedSearchEntitlementsPageToken { + return nil, fmt.Errorf("search-entitlements: page token repeated %d times, pagination is not advancing", repeatedToken) + } + } else { + repeatedToken = 0 + } + pageToken = nextPageToken } - list := resp.RequestCatalogSearchServiceSearchEntitlementsResponse.List - if list == nil { - return nil, errors.New("search-entitlements: list is nil") + return rv, nil +} + +// convertSearchEntitlementsPage expands and converts one response page. +// +// This runs per page rather than once over a concatenated list because the +// response's expanded array is indexed per response: ExpandedMap holds offsets +// into THIS page's expanded objects, so merging raw pages first would resolve +// those offsets against the wrong array. +func convertSearchEntitlementsPage( + resp *shared.RequestCatalogSearchServiceSearchEntitlementsResponse, +) ([]*EntitlementWithBindings, error) { + if resp == nil { + return nil, errors.New("search-entitlements: response is nil") } // Unmarshal the expanded fields - expanded := make([]any, 0, len(resp.RequestCatalogSearchServiceSearchEntitlementsResponse.Expanded)) - for _, x := range resp.RequestCatalogSearchServiceSearchEntitlementsResponse.Expanded { + expanded := make([]any, 0, len(resp.Expanded)) + for _, x := range resp.Expanded { x := x converted, err := UnmarshalAnyType[shared.RequestCatalogSearchServiceSearchEntitlementsResponseExpanded](&x) if err != nil { @@ -135,9 +180,11 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme expanded = append(expanded, converted) } - // Convert the list of entitlements to a list of expandable entitlements - expandableList := make([]*ExpandableEntitlementWithBindings, 0, len(list)) - for _, v := range list { + // Convert the list of entitlements to a list of expandable entitlements. A + // nil list is an empty page, not a fault -- the wire format omits an empty + // repeated field entirely. + expandableList := make([]*ExpandableEntitlementWithBindings, 0, len(resp.List)) + for _, v := range resp.List { ent := NewExpandableEntitlementWithBindings(v) if ent == nil { return nil, errors.New("search-entitlements: entitlement is nil") @@ -147,7 +194,7 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme } // Populate the expandable objects with the indexes of related objects - err = ExpandableReponse[*ExpandableEntitlementWithBindings]{ + err := ExpandableReponse[*ExpandableEntitlementWithBindings]{ List: expandableList, }.PopulateExpandedIndexes() @@ -156,7 +203,7 @@ func (c *client) SearchEntitlements(ctx context.Context, filter *SearchEntitleme } // Iterate over the expandable objects and convert them to the final response - rv := make([]*EntitlementWithBindings, 0, len(list)) + rv := make([]*EntitlementWithBindings, 0, len(expandableList)) for _, v := range expandableList { rv = append(rv, &EntitlementWithBindings{ Entitlement: AppEntitlement(*v.Entitlement.AppEntitlement), diff --git a/pkg/client/entitlement_pagination_test.go b/pkg/client/entitlement_pagination_test.go new file mode 100644 index 00000000..5b8f3e42 --- /dev/null +++ b/pkg/client/entitlement_pagination_test.go @@ -0,0 +1,168 @@ +package client + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// entitlementPage is one canned SearchEntitlements response. +type entitlementPage struct { + ids []string + nextPageToken string +} + +// serveEntitlementPages returns a server that hands back pages in order and +// records the page_token it was sent for each request. +func serveEntitlementPages(t *testing.T, pages []entitlementPage) (*httptest.Server, *[]string) { + t.Helper() + seen := make([]string, 0, len(pages)) + call := 0 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req map[string]any + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("decode request: %v", err) + } + token, _ := req["pageToken"].(string) + seen = append(seen, token) + + if call >= len(pages) { + t.Errorf("server called %d times, only %d pages configured", call+1, len(pages)) + w.WriteHeader(http.StatusInternalServerError) + return + } + page := pages[call] + call++ + + entries := make([]string, 0, len(page.ids)) + for _, id := range page.ids { + entries = append(entries, fmt.Sprintf( + `{"entitlement":{"appEntitlement":{"id":%q,"appId":"app1","displayName":%q}}}`, id, id)) + } + body := fmt.Sprintf(`{"list":[%s]`, strings.Join(entries, ",")) + if page.nextPageToken != "" { + body += fmt.Sprintf(`,"nextPageToken":%q`, page.nextPageToken) + } + body += "}" + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(server.Close) + + return server, &seen +} + +func entitlementIDs(got []*EntitlementWithBindings) []string { + ids := make([]string, 0, len(got)) + for _, e := range got { + ids = append(ids, e.Entitlement.GetAppId()+"/"+StringFromPtr(e.Entitlement.ID)) + } + return ids +} + +// TestSearchEntitlementsFollowsNextPageToken is the regression this whole change +// exists for: before it, the client made exactly one call and silently dropped +// every entitlement past the first page. +func TestSearchEntitlementsFollowsNextPageToken(t *testing.T) { + server, seen := serveEntitlementPages(t, []entitlementPage{ + {ids: []string{"ent1", "ent2"}, nextPageToken: "tok1"}, + {ids: []string{"ent3"}, nextPageToken: "tok2"}, + {ids: []string{"ent4"}}, + }) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + + if len(got) != 4 { + t.Fatalf("got %d entitlements (%v), want 4 across 3 pages", len(got), entitlementIDs(got)) + } + want := []string{"app1/ent1", "app1/ent2", "app1/ent3", "app1/ent4"} + for i, w := range want { + if entitlementIDs(got)[i] != w { + t.Errorf("entitlement %d = %q, want %q (page order must be preserved)", i, entitlementIDs(got)[i], w) + } + } + + wantTokens := []string{"", "tok1", "tok2"} + if len(*seen) != len(wantTokens) { + t.Fatalf("server saw %d requests (%v), want %d", len(*seen), *seen, len(wantTokens)) + } + for i, w := range wantTokens { + if (*seen)[i] != w { + t.Errorf("request %d sent pageToken %q, want %q", i, (*seen)[i], w) + } + } +} + +// TestSearchEntitlementsDoesNotStopOnEmptyPage covers the server's post-filter: +// granted_status is applied after the page is cut, so an intermediate page can be +// empty while later pages still hold results. Stopping on a short page would +// silently truncate. +func TestSearchEntitlementsDoesNotStopOnEmptyPage(t *testing.T) { + server, _ := serveEntitlementPages(t, []entitlementPage{ + {ids: []string{"ent1"}, nextPageToken: "tok1"}, + {ids: nil, nextPageToken: "tok2"}, + {ids: []string{"ent2"}}, + }) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + + if len(got) != 2 { + t.Fatalf("got %d entitlements (%v), want 2: an empty middle page must not end pagination", + len(got), entitlementIDs(got)) + } +} + +// TestSearchEntitlementsEmptyResultIsNotAnError pins that a wholly empty result +// returns an empty slice. The wire format omits an empty repeated field, so the +// list arrives nil. +func TestSearchEntitlementsEmptyResultIsNotAnError(t *testing.T) { + server, _ := serveEntitlementPages(t, []entitlementPage{{ids: nil}}) + + c := newPaperSecretTestClient(server.URL, server.Client()) + got, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err != nil { + t.Fatalf("SearchEntitlements: %v", err) + } + if len(got) != 0 { + t.Fatalf("got %d entitlements, want 0", len(got)) + } +} + +// TestSearchEntitlementsStopsOnStuckPageToken keeps a server that never advances +// the token from spinning the client forever. +func TestSearchEntitlementsStopsOnStuckPageToken(t *testing.T) { + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"list":[{"entitlement":{"appEntitlement":{"id":"ent1","appId":"app1"}}}],"nextPageToken":"stuck"}`)) + })) + defer server.Close() + + c := newPaperSecretTestClient(server.URL, server.Client()) + _, err := c.SearchEntitlements(context.Background(), &SearchEntitlementsFilter{}) + if err == nil { + t.Fatal("SearchEntitlements returned nil error on a non-advancing page token") + } + if !strings.Contains(err.Error(), "not advancing") { + t.Errorf("error = %v, want a non-advancing pagination error", err) + } + if calls > maxRepeatedSearchEntitlementsPageToken+2 { + t.Errorf("server called %d times, guard should have stopped it near %d", + calls, maxRepeatedSearchEntitlementsPageToken) + } +}