diff --git a/internal/dev_server/adapters/api.go b/internal/dev_server/adapters/api.go index c968b3ac..e7691830 100644 --- a/internal/dev_server/adapters/api.go +++ b/internal/dev_server/adapters/api.go @@ -47,7 +47,7 @@ func (a apiClientApi) GetSdkKey(ctx context.Context, projectKey, environmentKey func (a apiClientApi) GetAllFlags(ctx context.Context, projectKey string) ([]ldapi.FeatureFlag, error) { log.Printf("Fetching all flags for project '%s'", projectKey) - flags, err := a.getFlags(ctx, projectKey, nil) + flags, err := a.getFlags(ctx, projectKey) if err != nil { err = errors.Wrap(err, "unable to get all flags from LD API") } @@ -63,23 +63,30 @@ func (a apiClientApi) GetProjectEnvironments(ctx context.Context, projectKey str return environments, err } -func (a apiClientApi) getFlags(ctx context.Context, projectKey string, href *string) ([]ldapi.FeatureFlag, error) { - return internal.GetPaginatedItems(ctx, projectKey, href, func(ctx context.Context, projectKey string, limit, offset *int64) (flags *ldapi.FeatureFlags, err error) { - // loop until we do not get rate limited - query := a.apiClient.FeatureFlagsApi.GetFeatureFlags(ctx, projectKey).Limit(100) - query = query.Filter("purpose:all+!(holdout)") - - if limit != nil { - query = query.Limit(*limit) - } +const ( + flagsPageSize = 100 + flagsConcurrency = 6 +) - if offset != nil { - query = query.Offset(*offset) - } - return internal.Retry429s(query.Execute) +// getFlags pages the flags list concurrently (see internal.FetchPagesConcurrently). +func (a apiClientApi) getFlags(ctx context.Context, projectKey string) ([]ldapi.FeatureFlag, error) { + return internal.FetchPagesConcurrently(flagsPageSize, flagsConcurrency, func(offset int64) ([]ldapi.FeatureFlag, error) { + return a.getFlagsPage(ctx, projectKey, offset) }) } +func (a apiClientApi) getFlagsPage(ctx context.Context, projectKey string, offset int64) ([]ldapi.FeatureFlag, error) { + query := a.apiClient.FeatureFlagsApi.GetFeatureFlags(ctx, projectKey). + Filter("purpose:all+!(holdout)"). + Limit(flagsPageSize). + Offset(offset) + flags, err := internal.Retry429s(query.Execute) + if err != nil { + return nil, err + } + return flags.Items, nil +} + func (a apiClientApi) getEnvironments(ctx context.Context, projectKey string, href *string, query string, limit *int) ([]ldapi.Environment, error) { request := a.apiClient.EnvironmentsApi.GetEnvironmentsByProject(ctx, projectKey) diff --git a/internal/dev_server/adapters/internal/api_util.go b/internal/dev_server/adapters/internal/api_util.go index 8a2b3d33..1e7474e0 100644 --- a/internal/dev_server/adapters/internal/api_util.go +++ b/internal/dev_server/adapters/internal/api_util.go @@ -1,72 +1,63 @@ package internal import ( - "context" "log" "net/http" - "net/url" "strconv" + "sync" "time" - "github.com/launchdarkly/api-client-go/v14" "github.com/pkg/errors" ) -func GetPaginatedItems[T any, R interface { - GetItems() []T - GetLinks() map[string]ldapi.Link -}](ctx context.Context, projectKey string, href *string, fetchFunc func(context.Context, string, *int64, *int64) (R, error)) ([]T, error) { - var result R - var err error - - if href == nil { - result, err = fetchFunc(ctx, projectKey, nil, nil) - if err != nil { - return nil, err - } - } else { - limit, offset, err := parseHref(*href) - if err != nil { - return nil, errors.Wrapf(err, "unable to parse href for next link: %s", *href) - } - result, err = fetchFunc(ctx, projectKey, &limit, &offset) - if err != nil { - return nil, err - } +// FetchPagesConcurrently returns every item across an offset-paginated list. +// +// It fetches page 0, and only if that page is full keeps pulling pages in +// bounded concurrent batches (concurrency at a time) until a short page marks +// the end of the list. It deliberately never relies on a reported total count: +// that field is optional and reads back as 0 when absent, which would silently +// truncate a large result set. A short (or empty) page is the only end signal. +// Small lists cost a single request; large ones parallelise instead of paging +// serially. The first page error is returned as-is. +func FetchPagesConcurrently[T any](pageSize, concurrency int, fetch func(offset int64) ([]T, error)) ([]T, error) { + first, err := fetch(0) + if err != nil { + return nil, err + } + all := first + if len(first) < pageSize { + return all, nil } - items := result.GetItems() + for offset := int64(pageSize); ; offset += int64(concurrency) * int64(pageSize) { + batch := make([][]T, concurrency) + errs := make([]error, concurrency) + var wg sync.WaitGroup + for i := 0; i < concurrency; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + batch[i], errs[i] = fetch(offset + int64(i)*int64(pageSize)) + }(i) + } + wg.Wait() - if links := result.GetLinks(); links != nil { - if next, ok := links["next"]; ok && next.Href != nil { - newItems, err := GetPaginatedItems(ctx, projectKey, next.Href, fetchFunc) - if err != nil { - return nil, err + // Pages are contiguous by offset, so the first short/empty page ends the + // list and every page after it in the batch is empty. + done := false + for i := 0; i < concurrency; i++ { + if errs[i] != nil { + return nil, errs[i] + } + all = append(all, batch[i]...) + if len(batch[i]) < pageSize { + done = true } - items = append(items, newItems...) + } + if done { + return all, nil } } - - return items, nil -} - -func parseHref(href string) (limit, offset int64, err error) { - parsedUrl, err := url.Parse(href) - if err != nil { - return - } - l, err := strconv.Atoi(parsedUrl.Query().Get("limit")) - if err != nil { - return - } - o, err := strconv.Atoi(parsedUrl.Query().Get("offset")) - if err != nil { - return - } - - limit = int64(l) - offset = int64(o) - return } //go:generate go run go.uber.org/mock/mockgen -destination mocks.go -package internal . MockableTime @@ -91,6 +82,14 @@ func Retry429s[T any](requester func() (T, *http.Response, error)) (result T, er for { var res *http.Response result, res, err = requester() + // On a transport-level failure (DNS, connection refused, timeout) the + // client returns a nil response, so guard before touching it - otherwise + // the deref panics. A 429 arrives as a non-nil error *with* a non-nil + // response, so only bail on a nil response, never on err alone, or we'd + // skip the rate-limit retry below. + if res == nil { + return + } if res.StatusCode == 429 { resetUnixMillisString := res.Header.Get("X-Ratelimit-Reset") resetUnixMillis, strconvErr := strconv.ParseInt(resetUnixMillisString, 10, 64) diff --git a/internal/dev_server/adapters/internal/api_util_test.go b/internal/dev_server/adapters/internal/api_util_test.go index 28d388d0..5a0b59dd 100644 --- a/internal/dev_server/adapters/internal/api_util_test.go +++ b/internal/dev_server/adapters/internal/api_util_test.go @@ -1,149 +1,77 @@ package internal import ( - "context" "net/http" "testing" "time" - ldapi "github.com/launchdarkly/api-client-go/v14" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" ) -type testItem struct { - ID string -} - -type testResult struct { - items []testItem - links map[string]ldapi.Link -} +func TestFetchPagesConcurrently(t *testing.T) { + const pageSize, concurrency = 100, 6 -func (r testResult) GetItems() []testItem { - return r.items -} - -func (r testResult) GetLinks() map[string]ldapi.Link { - return r.links -} + // pager returns a fetch func that serves `total` sequential ints across + // pages of pageSize, recording every offset requested. + pager := func(total int) (func(offset int64) ([]int, error), *[]int64) { + var requested []int64 + fetch := func(offset int64) ([]int, error) { + requested = append(requested, offset) + var page []int + for i := offset; i < offset+pageSize && i < int64(total); i++ { + page = append(page, int(i)) + } + return page, nil + } + return fetch, &requested + } -func TestGetPaginatedItems(t *testing.T) { - ctx := context.Background() - projectKey := "test-project" + t.Run("single short page costs one request", func(t *testing.T) { + fetch, requested := pager(42) + items, err := FetchPagesConcurrently(pageSize, concurrency, fetch) + require.NoError(t, err) + assert.Len(t, items, 42) + assert.Equal(t, []int64{0}, *requested) + }) - testCases := []struct { - name string - fetchResponses []testResult - expectedItems []testItem - expectedError bool - }{ - { - name: "Single page", - fetchResponses: []testResult{ - { - items: []testItem{{ID: "1"}, {ID: "2"}}, - links: map[string]ldapi.Link{}, - }, - }, - expectedItems: []testItem{{ID: "1"}, {ID: "2"}}, - }, - { - name: "Multiple pages", - fetchResponses: []testResult{ - { - items: []testItem{{ID: "1"}, {ID: "2"}}, - links: map[string]ldapi.Link{ - "next": {Href: strPtr("http://example.com?limit=2&offset=2")}, - }, - }, - { - items: []testItem{{ID: "3"}, {ID: "4"}}, - links: map[string]ldapi.Link{}, - }, - }, - expectedItems: []testItem{{ID: "1"}, {ID: "2"}, {ID: "3"}, {ID: "4"}}, - }, - { - name: "Error on second page", - fetchResponses: []testResult{ - { - items: []testItem{{ID: "1"}, {ID: "2"}}, - links: map[string]ldapi.Link{ - "next": {Href: strPtr("http://example.com?limit=2&offset=2")}, - }, - }, - }, - expectedError: true, - }, - { - name: "Empty response", - fetchResponses: []testResult{ - { - items: []testItem{}, - links: map[string]ldapi.Link{}, - }, - }, - expectedItems: []testItem{}, - }, - { - name: "Multiple pages with varying item counts", - fetchResponses: []testResult{ - { - items: []testItem{{ID: "1"}, {ID: "2"}, {ID: "3"}}, - links: map[string]ldapi.Link{ - "next": {Href: strPtr("http://example.com?limit=3&offset=3")}, - }, - }, - { - items: []testItem{{ID: "4"}, {ID: "5"}}, - links: map[string]ldapi.Link{ - "next": {Href: strPtr("http://example.com?limit=3&offset=5")}, - }, - }, - { - items: []testItem{{ID: "6"}}, - links: map[string]ldapi.Link{}, - }, - }, - expectedItems: []testItem{{ID: "1"}, {ID: "2"}, {ID: "3"}, {ID: "4"}, {ID: "5"}, {ID: "6"}}, - }, - { - name: "Invalid next link", - fetchResponses: []testResult{ - { - items: []testItem{{ID: "1"}, {ID: "2"}}, - links: map[string]ldapi.Link{ - "next": {Href: strPtr("invalid-url")}, - }, - }, - }, - expectedError: true, - }, - } + t.Run("exactly one full page still probes for more, then stops", func(t *testing.T) { + fetch, _ := pager(100) + items, err := FetchPagesConcurrently(pageSize, concurrency, fetch) + require.NoError(t, err) + assert.Len(t, items, 100) + }) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - callCount := 0 - fetchFunc := func(ctx context.Context, projectKey string, limit, offset *int64) (testResult, error) { - if callCount >= len(tc.fetchResponses) { - return testResult{}, assert.AnError - } - result := tc.fetchResponses[callCount] - callCount++ - return result, nil - } + t.Run("many pages are all collected in order", func(t *testing.T) { + fetch, _ := pager(1350) // 13 full pages + a short one + items, err := FetchPagesConcurrently(pageSize, concurrency, fetch) + require.NoError(t, err) + require.Len(t, items, 1350) + for i := range items { + assert.Equal(t, i, items[i]) + } + }) - items, err := GetPaginatedItems(ctx, projectKey, nil, fetchFunc) + t.Run("never trusts a total: keeps paging while pages are full", func(t *testing.T) { + // A source that reports nothing about totals; end is only inferable from + // a short page. 250 items => pages at 0,100,200(short). + fetch, _ := pager(250) + items, err := FetchPagesConcurrently(pageSize, concurrency, fetch) + require.NoError(t, err) + assert.Len(t, items, 250) + }) - if tc.expectedError { - assert.Error(t, err) - } else { - assert.NoError(t, err) - assert.Equal(t, tc.expectedItems, items) + t.Run("a page error aborts and propagates", func(t *testing.T) { + fetch := func(offset int64) ([]int, error) { + if offset == 0 { + return make([]int, pageSize), nil // full, so it fans out } - }) - } + return nil, assert.AnError + } + _, err := FetchPagesConcurrently(pageSize, concurrency, fetch) + assert.ErrorIs(t, err, assert.AnError) + }) } func TestRetry429s(t *testing.T) { @@ -175,15 +103,23 @@ func TestRetry429s(t *testing.T) { } else { header := make(http.Header) header.Set("X-Ratelimit-Reset", "1000") - return "", &http.Response{StatusCode: 429, Header: header}, nil + // The generated client returns a non-nil error alongside the 429 + // response; the retry must not bail on err alone. + return "", &http.Response{StatusCode: 429, Header: header}, assert.AnError } }) assert.Equal(t, "lol", res) assert.NoError(t, err) assert.Equal(t, 2, called) }) -} -func strPtr(s string) *string { - return &s + t.Run("it returns the error without panicking on a nil response", func(t *testing.T) { + called := 0 + _, err := Retry429s(func() (string, *http.Response, error) { + called++ + return "", nil, assert.AnError + }) + assert.ErrorIs(t, err, assert.AnError) + assert.Equal(t, 1, called) + }) }