From 43c6dc4e72d5eea383ebfa16eda284a09c6363ea Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Tue, 15 Dec 2020 13:43:53 +0300 Subject: [PATCH 1/2] Fix refs pagination for updated-at sort There's buggy behavior: When refs are sorted by committerdate and page token is passed, then weird results are returned. It works fine when refs are sorted by refname though. IsPageToken defined in internal/gitaly/service/ref/refs.go is using bytes.Compare which compares refs alphabethically. It works fine when refs are being sorted by refname, but when we sort by another field, IsPageToken would unexpectedly return the first result that is greater alphabetically. For example, if refs have been created in the following order: a c b d and we pass { SortBy: 'updated_asc', PageToken: 'b', Limit: 1 }, then we get the ref "b", while expected result is "d". We can use bytes.HasPrefix instead to fix this case --- ...-fix-page-token-for-committerdate-sort.yml | 5 + internal/gitaly/service/ref/refs.go | 5 +- internal/gitaly/service/ref/refs_test.go | 116 +++++++++++------- 3 files changed, 83 insertions(+), 43 deletions(-) create mode 100644 changelogs/unreleased/id-fix-page-token-for-committerdate-sort.yml diff --git a/changelogs/unreleased/id-fix-page-token-for-committerdate-sort.yml b/changelogs/unreleased/id-fix-page-token-for-committerdate-sort.yml new file mode 100644 index 0000000000..5cca77ebb7 --- /dev/null +++ b/changelogs/unreleased/id-fix-page-token-for-committerdate-sort.yml @@ -0,0 +1,5 @@ +--- +title: Fix refs pagination for updated-at sort +merge_request: 2932 +author: +type: fixed diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go index 000d738e2d..c4984f6407 100644 --- a/internal/gitaly/service/ref/refs.go +++ b/internal/gitaly/service/ref/refs.go @@ -480,7 +480,10 @@ func paginationParamsToOpts(p *gitalypb.PaginationParameter) *findRefsOpts { } if p.GetPageToken() != "" { - opts.IsPageToken = func(l []byte) bool { return bytes.Compare(l, []byte(p.GetPageToken())) >= 0 } + pageToken := []byte(p.GetPageToken()) + opts.IsPageToken = func(l []byte) bool { + return bytes.HasPrefix(l, pageToken) + } } return opts diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go index b8ab9f8154..e7d3c7132f 100644 --- a/internal/gitaly/service/ref/refs_test.go +++ b/internal/gitaly/service/ref/refs_test.go @@ -898,54 +898,86 @@ func TestFindLocalBranchesPagination(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - limit := 1 - rpcRequest := &gitalypb.FindLocalBranchesRequest{ - Repository: testRepo, - PaginationParams: &gitalypb.PaginationParameter{ - Limit: int32(limit), - PageToken: "refs/heads/gitaly/squash-test", + testCases := []struct { + desc string + sortBy gitalypb.FindLocalBranchesRequest_SortBy + pageToken string + limit int32 + expectedBranch string + }{ + { + desc: "sorted by name", + sortBy: gitalypb.FindLocalBranchesRequest_NAME, + pageToken: "refs/heads/gitaly/squash-test", + limit: 1, + expectedBranch: "refs/heads/improve/awesome", + }, + { + desc: "sorted by committer date asc", + sortBy: gitalypb.FindLocalBranchesRequest_UPDATED_ASC, + pageToken: "refs/heads/feature_conflict", + limit: 1, + expectedBranch: "refs/heads/'test'", + }, + { + desc: "sorted by committer date desc", + sortBy: gitalypb.FindLocalBranchesRequest_UPDATED_DESC, + pageToken: "refs/heads/empty-branch", + limit: 1, + expectedBranch: "refs/heads/'test'", }, - } - c, err := client.FindLocalBranches(ctx, rpcRequest) - if err != nil { - t.Fatal(err) - } - - var branches []*gitalypb.FindLocalBranchResponse - for { - r, err := c.Recv() - if err == io.EOF { - break - } - require.NoError(t, err) - if err != nil { - t.Fatal(err) - } - branches = append(branches, r.GetBranches()...) } - require.Len(t, branches, limit) + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + rpcRequest := &gitalypb.FindLocalBranchesRequest{ + Repository: testRepo, + SortBy: tc.sortBy, + PaginationParams: &gitalypb.PaginationParameter{ + Limit: tc.limit, + PageToken: tc.pageToken, + }, + } + c, err := client.FindLocalBranches(ctx, rpcRequest) + if err != nil { + t.Fatal(err) + } - expectedBranch := "refs/heads/improve/awesome" - target := localBranches[expectedBranch] + var branches []*gitalypb.FindLocalBranchResponse + for { + r, err := c.Recv() + if err == io.EOF { + break + } + require.NoError(t, err) + if err != nil { + t.Fatal(err) + } + branches = append(branches, r.GetBranches()...) + } - branch := &gitalypb.FindLocalBranchResponse{ - Name: []byte(expectedBranch), - CommitId: target.Id, - CommitSubject: target.Subject, - CommitAuthor: &gitalypb.FindLocalBranchCommitAuthor{ - Name: target.Author.Name, - Email: target.Author.Email, - Date: target.Author.Date, - }, - CommitCommitter: &gitalypb.FindLocalBranchCommitAuthor{ - Name: target.Committer.Name, - Email: target.Committer.Email, - Date: target.Committer.Date, - }, - Commit: target, + require.Len(t, branches, int(tc.limit)) + + target := localBranches[tc.expectedBranch] + branch := &gitalypb.FindLocalBranchResponse{ + Name: []byte(tc.expectedBranch), + CommitId: target.Id, + CommitSubject: target.Subject, + CommitAuthor: &gitalypb.FindLocalBranchCommitAuthor{ + Name: target.Author.Name, + Email: target.Author.Email, + Date: target.Author.Date, + }, + CommitCommitter: &gitalypb.FindLocalBranchCommitAuthor{ + Name: target.Committer.Name, + Email: target.Committer.Email, + Date: target.Committer.Date, + }, + Commit: target, + } + assertContainsLocalBranch(t, branches, branch) + }) } - assertContainsLocalBranch(t, branches, branch) } // Test that `s` contains the elements in `relativeOrder` in that order -- GitLab From 7602cd08b7b6f450db376730e11bad1fc281c2a5 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 12 Feb 2021 10:15:13 +0100 Subject: [PATCH 2/2] Add failing testcase --- internal/gitaly/service/ref/refs_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/gitaly/service/ref/refs_test.go b/internal/gitaly/service/ref/refs_test.go index e7d3c7132f..f5b87d2b71 100644 --- a/internal/gitaly/service/ref/refs_test.go +++ b/internal/gitaly/service/ref/refs_test.go @@ -926,6 +926,13 @@ func TestFindLocalBranchesPagination(t *testing.T) { limit: 1, expectedBranch: "refs/heads/'test'", }, + { + desc: "pageToken does not exist", + sortBy: gitalypb.FindLocalBranchesRequest_NAME, + pageToken: "refs/heads/gitaly/squash-test1", // does not exist + limit: 1, + expectedBranch: "refs/heads/improve/awesome", + }, } for _, tc := range testCases { -- GitLab