Skip to content

fix: implement cursor-based pagination for ListRepositories endpoint#6342

Open
MayankSharmaCSE wants to merge 3 commits intomindersec:mainfrom
MayankSharmaCSE:fix/list-repositories-pagination
Open

fix: implement cursor-based pagination for ListRepositories endpoint#6342
MayankSharmaCSE wants to merge 3 commits intomindersec:mainfrom
MayankSharmaCSE:fix/list-repositories-pagination

Conversation

@MayankSharmaCSE
Copy link
Copy Markdown

Summary

The ListRepositories gRPC 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) and cursor parameters in the request, and returns a cursor field in the response. When the cursor is non-empty, clients can use it to fetch the next page.

Files changed:

  • internal/repositories/service.go - Added ListRepositoriesPaginated method to interface and implementation
  • internal/controlplane/handlers_repositories.go - Updated handler to use paginated method
  • internal/repositories/mock/service.go - Mock regenerated via go generate
  • internal/repositories/mock/fixtures/service.go - Added paginated mock fixtures
  • internal/controlplane/handlers_repositories_test.go - Updated tests for new pagination behavior

Dependencies: None - uses existing ListEntitiesAfterID DB query that already supports cursor-based pagination.

Fixes #6340

Testing

  • Unit tests pass:
go test ./internal/controlplane/... -run "TestServer_ListRepositories" -v
go test ./internal/repositories/...
  • All controlplane tests pass: go test ./internal/controlplane/...
  • Build succeeds: go build ./...
  • Manual testing:
  • First page: minder repo list --limit 10
  • Next page: minder repo list --limit 10 --cursor <cursor_from_previous_response>

@MayankSharmaCSE MayankSharmaCSE requested a review from a team as a code owner April 11, 2026 13:00
Copilot AI review requested due to automatic review settings April 11, 2026 13:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ListRepositoriesPaginated method to the repositories service interface + implementation.
  • Updated the ListRepositories gRPC handler to accept limit and cursor and 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.

Comment thread internal/repositories/service.go Outdated
Comment on lines +264 to +268
repoEnts, outErr = qtx.ListEntitiesAfterID(ctx, db.ListEntitiesAfterIDParams{
EntityType: db.EntitiesRepository,
ID: cursor,
Limit: limit,
})
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use the same query for both cases, by passing in the null (all-zeros) UUID in the start case. It should look something like:

SELECT * FROM entities WHERE project_id IN $1 AND id > $2 ORDER BY id LIMIT ($3 + 1);

The cursor would then be the second-to-last id returned, so the "greater than" would naturally fetch the next rows. We over-fetch by one row so that we can decide whether there are more pages (because there's at least one extra row) -- the cost for fetching one more row in the batch should be low.

Comment thread internal/repositories/service.go Outdated
Comment on lines +255 to +259
repoEnts, outErr = qtx.GetEntitiesByType(ctx, db.GetEntitiesByTypeParams{
EntityType: db.EntitiesRepository,
ProviderID: providerID,
Projects: []uuid.UUID{projectID},
})
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread internal/repositories/service.go Outdated
Comment on lines +242 to +246
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)
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment thread internal/repositories/service.go Outdated
Comment on lines +233 to +234
if limit > 1000 {
limit = 1000
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
if limit > 1000 {
limit = 1000
if limit > 100 {
limit = 100

Copilot uses AI. Check for mistakes.
Comment on lines +161 to +165
if in.GetCursor() != "" {
cursor, err = uuid.Parse(in.GetCursor())
if err != nil {
return nil, util.UserVisibleError(codes.InvalidArgument, "invalid cursor format")
}
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
resp.Cursor = nextCursor.String()
resp.Cursor = strings.ReplaceAll(nextCursor.String(), "-", "")

Copilot uses AI. Check for mistakes.
Comment on lines +153 to +155
).
Return(repositories, uuid.Nil, nil).AnyTimes()
}
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@MayankSharmaCSE MayankSharmaCSE force-pushed the fix/list-repositories-pagination branch from c9403da to e8b0879 Compare April 11, 2026 13:10
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 12, 2026

Coverage Status

Coverage is 60.232%MayankSharmaCSE:fix/list-repositories-pagination into mindersec:main. No base build found for mindersec:main.

@MayankSharmaCSE
Copy link
Copy Markdown
Author

@evankanderson Thanks for the review! Both comments are valid and we'll address them:

  1. Database limit: You're right that fetching all entities and then truncating in memory defeats pagination. We'll add a new ListEntitiesByTypePaginated query that
    applies LIMIT at the database level for the first page.
  2. Cursor blinding: The hyphenated UUIDs don't match the proto pattern ^[[:word:]=]*$. We'll encode the cursor as base64 to make it compliant and less of an
    explicit contract.

We also identified a related issue we'll fix:

  • Data scoping: ListEntitiesAfterID needs to be scoped by provider_id/project_id to prevent cross-project data access.

We'll proceed with implementing these fixes. Let us know if you have any concerns!

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like any of the SQL query suggestions have been applied. If you update the SQL queries, you'll also need to run make gen to re-generate the query code.

Comment thread internal/repositories/service.go Outdated
Comment on lines +264 to +268
repoEnts, outErr = qtx.ListEntitiesAfterID(ctx, db.ListEntitiesAfterIDParams{
EntityType: db.EntitiesRepository,
ID: cursor,
Limit: limit,
})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use the same query for both cases, by passing in the null (all-zeros) UUID in the start case. It should look something like:

SELECT * FROM entities WHERE project_id IN $1 AND id > $2 ORDER BY id LIMIT ($3 + 1);

The cursor would then be the second-to-last id returned, so the "greater than" would naturally fetch the next rows. We over-fetch by one row so that we can decide whether there are more pages (because there's at least one extra row) -- the cost for fetching one more row in the batch should be low.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking "request changes" on the SQL changes, and the conflict between the UUID-style cursor implemented and the (documented) opaque cursor without "-" characters.

@MayankSharmaCSE MayankSharmaCSE force-pushed the fix/list-repositories-pagination branch from cbf7507 to d7e26e6 Compare April 23, 2026 14:51
Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>
  - Use sqlc.arg for ListEntitiesByTypePaginated so generated params
    are named (Projects, Cursor, PageLimit) and correctly typed.
  - Fix transaction leak in ListRepositoriesPaginated via named
    returns so deferred rollback runs on early errors.
  - Replace stdlib log with zerolog.Ctx for rollback errors.
  - Extract decodeCursor and hydrateEntityProperties helpers to
    reduce cyclomatic complexity.
  - Introduce DefaultListRepositoriesPageLimit constant.

Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>
  - Use sqlc.arg for ListEntitiesByTypePaginated so generated params
    are named (Projects, Cursor, PageLimit) and correctly typed.
  - Fix transaction leak in ListRepositoriesPaginated via named
    returns so deferred rollback runs on early errors.
  - Replace stdlib log with zerolog.Ctx for rollback errors.
  - Extract decodeCursor and hydrateEntityProperties helpers to
    reduce cyclomatic complexity.
  - Introduce DefaultListRepositoriesPageLimit constant.
@MayankSharmaCSE MayankSharmaCSE force-pushed the fix/list-repositories-pagination branch from 3882115 to 673016c Compare April 23, 2026 20:29
Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you needed a newer version of sqlc, you'll need to update the version in tools/go.mod and re-run make bootstrap to make sure you're using the correct version. (Yeah, it sucks that we have a bunch of tools that sort of live globally via bootstrap, rather than in some hermetic location.)

// Code generated by sqlc. DO NOT EDIT.
// versions:
// sqlc v1.30.0
// sqlc v1.31.1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you brought in a new version of sqlc here. Intentional?

(If not, we'll pick it up in an update to tools/go.mod within a few weeks)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it was intentional because i was working on the updated version but now i have fixed it according to the same version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListRepositories returns all repositories at once with no pagination, leading to memory spikes and timeouts on large projects

4 participants