-
Notifications
You must be signed in to change notification settings - Fork 96
fix: implement cursor-based pagination for ListRepositories endpoint #6342
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -135,9 +135,11 @@ func (s *Server) repoCreateInfoFromUpstreamEntityRef( | |||||
|
|
||||||
| // ListRepositories returns a list of repositories for a given project | ||||||
| // This function will typically be called by the client to get a list of | ||||||
| // repositories that are registered present in the minder database | ||||||
| // repositories that are registered present in the minder database. | ||||||
| // Pagination is cursor-based: pass an empty cursor for the first page, | ||||||
| // then use the returned cursor for subsequent pages. | ||||||
| func (s *Server) ListRepositories(ctx context.Context, | ||||||
| _ *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) { | ||||||
| in *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) { | ||||||
| entityCtx := engcontext.EntityFromContext(ctx) | ||||||
| projectID := entityCtx.Project.ID | ||||||
| providerName := entityCtx.Provider.Name | ||||||
|
|
@@ -154,8 +156,24 @@ func (s *Server) ListRepositories(ctx context.Context, | |||||
| return nil, status.Errorf(codes.Internal, "cannot get provider: %v", err) | ||||||
| } | ||||||
|
|
||||||
| // Fetch repositories using the service | ||||||
| repoEntities, err := s.repos.ListRepositories(ctx, projectID, provider.ID) | ||||||
| // Parse cursor from request (empty string means first page) | ||||||
| var cursor uuid.UUID | ||||||
| if in.GetCursor() != "" { | ||||||
| cursor, err = uuid.Parse(in.GetCursor()) | ||||||
| if err != nil { | ||||||
| return nil, util.UserVisibleError(codes.InvalidArgument, "invalid cursor format") | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Use request limit if provided, otherwise default | ||||||
| limit := in.GetLimit() | ||||||
| if limit <= 0 { | ||||||
| limit = 100 | ||||||
| } | ||||||
|
|
||||||
| // Fetch repositories using the paginated service method | ||||||
| repoEntities, nextCursor, err := s.repos.ListRepositoriesPaginated( | ||||||
| ctx, projectID, provider.ID, cursor, limit) | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
|
|
@@ -191,11 +209,10 @@ func (s *Server) ListRepositories(ctx context.Context, | |||||
| results = append(results, pbRepo) | ||||||
| } | ||||||
|
|
||||||
| // TODO: Implement cursor-based pagination using entity IDs | ||||||
| // For now, return all results without pagination | ||||||
|
|
||||||
| resp.Results = results | ||||||
| resp.Cursor = "" | ||||||
| if nextCursor != uuid.Nil { | ||||||
| resp.Cursor = nextCursor.String() | ||||||
|
||||||
| resp.Cursor = nextCursor.String() | |
| resp.Cursor = strings.ReplaceAll(nextCursor.String(), "-", "") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,3 +138,33 @@ func WithFailedListRepositories(err error) func(RepoServiceMock) { | |
| Return(nil, err).AnyTimes() | ||
| } | ||
| } | ||
|
|
||
| func WithSuccessfulListRepositoriesPaginated( | ||
| repositories []*models.EntityWithProperties, | ||
| ) func(RepoServiceMock) { | ||
| return func(mock RepoServiceMock) { | ||
| mock.EXPECT(). | ||
| ListRepositoriesPaginated( | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| ). | ||
| Return(repositories, uuid.Nil, nil).AnyTimes() | ||
| } | ||
|
Comment on lines
+153
to
+155
|
||
| } | ||
|
|
||
| func WithFailedListRepositoriesPaginated(err error) func(RepoServiceMock) { | ||
| return func(mock RepoServiceMock) { | ||
| mock.EXPECT(). | ||
| ListRepositoriesPaginated( | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| gomock.Any(), | ||
| ). | ||
| Return(nil, uuid.Nil, err).AnyTimes() | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -70,6 +70,16 @@ | |||||||||
| providerID uuid.UUID, | ||||||||||
| ) ([]*models.EntityWithProperties, error) | ||||||||||
|
|
||||||||||
| // ListRepositoriesPaginated retrieves repositories with cursor-based pagination. | ||||||||||
| // If cursor is empty, returns the first page. limit defaults to 100. | ||||||||||
| ListRepositoriesPaginated( | ||||||||||
| ctx context.Context, | ||||||||||
| projectID uuid.UUID, | ||||||||||
| providerID uuid.UUID, | ||||||||||
| cursor uuid.UUID, | ||||||||||
| limit int64, | ||||||||||
| ) ([]*models.EntityWithProperties, uuid.UUID, error) | ||||||||||
|
|
||||||||||
| // GetRepositoryById retrieves a repository by its ID and project. | ||||||||||
| GetRepositoryById(ctx context.Context, repositoryID uuid.UUID, projectID uuid.UUID) (*pb.Repository, error) | ||||||||||
| // GetRepositoryByName retrieves a repository by its name, owner, project and provider (if specified). | ||||||||||
|
|
@@ -210,6 +220,95 @@ | |||||||||
| return ents, nil | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func (r *repositoryService) ListRepositoriesPaginated( | ||||||||||
| ctx context.Context, | ||||||||||
| projectID uuid.UUID, | ||||||||||
| providerID uuid.UUID, | ||||||||||
| cursor uuid.UUID, | ||||||||||
| limit int64, | ||||||||||
| ) ([]*models.EntityWithProperties, uuid.UUID, error) { | ||||||||||
| if limit <= 0 { | ||||||||||
| limit = 100 | ||||||||||
| } | ||||||||||
| if limit > 1000 { | ||||||||||
| limit = 1000 | ||||||||||
|
Comment on lines
+233
to
+234
|
||||||||||
| if limit > 1000 { | |
| limit = 1000 | |
| if limit > 100 { | |
| limit = 100 |
Copilot
AI
Apr 11, 2026
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.
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
AI
Apr 11, 2026
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.
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.
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.
Copilot
AI
Apr 11, 2026
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.
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.
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.
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.
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.