fix: implement cursor-based pagination for ListRepositories endpoint#6342
fix: implement cursor-based pagination for ListRepositories endpoint#6342MayankSharmaCSE wants to merge 2 commits intomindersec:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements cursor-based pagination for the ListRepositories controlplane endpoint to avoid unbounded responses when projects contain large numbers of registered repositories.
Changes:
- Added a new
ListRepositoriesPaginatedmethod to the repositories service interface + implementation. - Updated the
ListRepositoriesgRPC handler to acceptlimitandcursorand return a response cursor. - Regenerated/extended repository service mocks and updated handler tests to use the new method.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/repositories/service.go | Adds paginated listing method and pagination logic in repository service. |
| internal/controlplane/handlers_repositories.go | Switches handler to parse cursor/limit and call paginated service; returns response cursor. |
| internal/repositories/mock/service.go | Regenerated mock to include ListRepositoriesPaginated. |
| internal/repositories/mock/fixtures/service.go | Adds fixtures for the new paginated mock method. |
| internal/controlplane/handlers_repositories_test.go | Updates ListRepositories handler tests to use the new paginated mock fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| repoEnts, outErr = qtx.ListEntitiesAfterID(ctx, db.ListEntitiesAfterIDParams{ | ||
| EntityType: db.EntitiesRepository, | ||
| ID: cursor, | ||
| Limit: limit, | ||
| }) |
There was a problem hiding this comment.
The cursor branch calls ListEntitiesAfterID with only entity_type + id + limit. That query is not scoped to provider_id/project_id, so this endpoint can return repositories from other projects/providers once a non-empty cursor is used (and potentially leak data). Add a scoped pagination query (entity_type + provider_id + project_id + id > cursor ORDER BY id LIMIT ...) and use it here.
| repoEnts, outErr = qtx.GetEntitiesByType(ctx, db.GetEntitiesByTypeParams{ | ||
| EntityType: db.EntitiesRepository, | ||
| ProviderID: providerID, | ||
| Projects: []uuid.UUID{projectID}, | ||
| }) |
There was a problem hiding this comment.
When cursor == uuid.Nil this calls GetEntitiesByType, which has no LIMIT and (in SQL) no ORDER BY. That means the first page still loads all repositories into memory (defeating the pagination goal) and the resulting nextCursor based on the last element is not stable (can skip/duplicate results). Use a DB-level ordered + limited query for the first page too.
There was a problem hiding this comment.
I think this is the critical comment -- you should set a database limit to avoid doing a lot of extra backend work. Given the cursor-based query, you should be able to fetch using the natural order of the table (ORDER BY id) and then a WHERE clause on id > cursor.
| outErr := errors.New("outer error placeholder") | ||
| defer func() { | ||
| if outErr != nil { | ||
| if err := tx.Rollback(); err != nil { | ||
| log.Printf("error rolling back transaction: %v", err) |
There was a problem hiding this comment.
Rollback is gated on outErr, but outErr is only set around the initial query. If an error occurs later (e.g., in the properties loop), outErr can be nil and the deferred rollback won't run, leaving the transaction open. Use a named return error (like ListRepositories) or defer an unconditional rollback (ignoring ErrTxDone after Commit).
| if limit > 1000 { | ||
| limit = 1000 |
There was a problem hiding this comment.
This method clamps limit to 1000, but the protobuf contract for ListRepositoriesRequest.limit is validated to lte: 100 via protovalidate. Consider aligning the server-side clamp/docs with the API contract (or update the proto + any clients in the same PR).
| if limit > 1000 { | |
| limit = 1000 | |
| if limit > 100 { | |
| limit = 100 |
| if in.GetCursor() != "" { | ||
| cursor, err = uuid.Parse(in.GetCursor()) | ||
| if err != nil { | ||
| return nil, util.UserVisibleError(codes.InvalidArgument, "invalid cursor format") | ||
| } |
There was a problem hiding this comment.
ListRepositoriesRequest.cursor is protovalidate-constrained to the pattern ^[[:word:]=]*$, which is not a UUID shape (notably excludes '-'). Parsing the cursor strictly as uuid.UUID can reject otherwise-valid client cursors, and will also fail round-tripping if the server returns a hyphenated UUID. Consider using an opaque cursor encoding that matches the proto constraints (e.g., base64 or hex).
There was a problem hiding this comment.
You may need to update the annotations in minder.proto, or consider blinding the cursor (e.g. base64 the contents) to make it less of an explicit contract.
| resp.Results = results | ||
| resp.Cursor = "" | ||
| if nextCursor != uuid.Nil { | ||
| resp.Cursor = nextCursor.String() |
There was a problem hiding this comment.
resp.Cursor is set using nextCursor.String(), which produces a hyphenated UUID. Because request validation restricts cursor to ^[[:word:]=]*$, clients may not be able to send this cursor back on the next request. Return an encoding that matches the proto constraints (e.g., UUID hex without hyphens or base64).
| resp.Cursor = nextCursor.String() | |
| resp.Cursor = strings.ReplaceAll(nextCursor.String(), "-", "") |
| ). | ||
| Return(repositories, uuid.Nil, nil).AnyTimes() | ||
| } |
There was a problem hiding this comment.
This fixture always returns uuid.Nil as nextCursor, so tests can’t cover the new behavior of returning a non-empty cursor and fetching subsequent pages. Consider accepting nextCursor as an argument and adding a test that asserts ListRepositoriesResponse.Cursor is set.
Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>
c9403da to
e8b0879
Compare
|
@evankanderson Thanks for the review! Both comments are valid and we'll address them:
We also identified a related issue we'll fix:
We'll proceed with implementing these fixes. Let us know if you have any concerns! |
Summary
The
ListRepositoriesgRPC endpoint was returning all registered repositories in a single unbounded response, with no pagination support. A TODO comment in the code explicitly acknowledged this: "TODO: Implement cursor-based pagination using entity IDs. For now, return all results without pagination."This caused memory exhaustion and timeouts on projects with large numbers of repositories. The fix implements cursor-based pagination using entity IDs as cursors. The handler now accepts optional
limit(default 100, max 1000) andcursorparameters in the request, and returns acursorfield in the response. When the cursor is non-empty, clients can use it to fetch the next page.Files changed:
internal/repositories/service.go- AddedListRepositoriesPaginatedmethod to interface and implementationinternal/controlplane/handlers_repositories.go- Updated handler to use paginated methodinternal/repositories/mock/service.go- Mock regenerated viago generateinternal/repositories/mock/fixtures/service.go- Added paginated mock fixturesinternal/controlplane/handlers_repositories_test.go- Updated tests for new pagination behaviorDependencies: None - uses existing
ListEntitiesAfterIDDB query that already supports cursor-based pagination.Fixes #6340
Testing
go test ./internal/controlplane/...go build ./...minder repo list --limit 10minder repo list --limit 10 --cursor <cursor_from_previous_response>