Merge branch '1381-present-commits-pagination-headers-correctly' into 'master'

GET "projects/:id/repository/commits" endpoint improvements

Closes #1381 and #20207

See merge request !9679

3 files changed, 144 insertions, 5 deletions

diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 3f11f0a4516..bc139d5ef28 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -824,6 +824,32 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it { is_expected.to eq(17) }
   end
 
+  describe '#count_commits' do
+    context 'with after timestamp' do
+      it 'returns the number of commits after timestamp' do
+        options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
+
+        expect(repository.count_commits(options)).to eq(25)
+      end
+    end
+
+    context 'with before timestamp' do
+      it 'returns the number of commits after timestamp' do
+        options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
+
+        expect(repository.count_commits(options)).to eq(9)
+      end
+    end
+
+    context 'with path' do
+      it 'returns the number of commits with path ' do
+        options = { ref: 'master', limit: nil, path: "encoding" }
+
+        expect(repository.count_commits(options)).to eq(2)
+      end
+    end
+  end
+
   describe "branch_names_contains" do
     subject { repository.branch_names_contains(SeedRepo::LastCommit::ID) }
 
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index eb992e1354e..274e4f00a0a 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1042,7 +1042,7 @@ describe Repository, models: true do
 
     it 'expires the cache for all branches' do
       expect(cache).to receive(:expire).
-        at_least(repository.branches.length).
+        at_least(repository.branches.length * 2).
         times
 
       repository.expire_branch_cache
@@ -1050,14 +1050,14 @@ describe Repository, models: true do
 
     it 'expires the cache for all branches when the root branch is given' do
       expect(cache).to receive(:expire).
-        at_least(repository.branches.length).
+        at_least(repository.branches.length * 2).
         times
 
       repository.expire_branch_cache(repository.root_ref)
     end
 
     it 'expires the cache for a specific branch' do
-      expect(cache).to receive(:expire).once
+      expect(cache).to receive(:expire).twice
 
       repository.expire_branch_cache('foo')
     end
@@ -1742,6 +1742,29 @@ describe Repository, models: true do
     end
   end
 
+  describe '#commit_count_for_ref' do
+    let(:project) { create :empty_project }
+
+    context 'with a non-existing repository' do
+      it 'returns 0' do
+        expect(project.repository.commit_count_for_ref('master')).to eq(0)
+      end
+    end
+
+    context 'with empty repository' do
+      it 'returns 0' do
+        project.create_repository
+        expect(project.repository.commit_count_for_ref('master')).to eq(0)
+      end
+    end
+
+    context 'when searching for the root ref' do
+      it 'returns the same count as #commit_count' do
+        expect(repository.commit_count_for_ref(repository.root_ref)).to eq(repository.commit_count)
+      end
+    end
+  end
+
   describe '#cache_method_output', caching: true do
     context 'with a non-existing repository' do
       let(:value) do
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 5190fcca2d1..585449e62b6 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -19,6 +19,7 @@ describe API::Commits, api: true  do
 
       it "returns project commits" do
         commit = project.repository.commit
+
         get api("/projects/#{project.id}/repository/commits", user)
 
         expect(response).to have_http_status(200)
@@ -27,6 +28,16 @@ describe API::Commits, api: true  do
         expect(json_response.first['committer_name']).to eq(commit.committer_name)
         expect(json_response.first['committer_email']).to eq(commit.committer_email)
       end
+
+      it 'include correct pagination headers' do
+        commit_count = project.repository.count_commits(ref: 'master').to_s
+
+        get api("/projects/#{project.id}/repository/commits", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "unauthorized user" do
@@ -39,14 +50,26 @@ describe API::Commits, api: true  do
     context "since optional parameter" do
       it "returns project commits since provided parameter" do
         commits = project.repository.commits("master")
-        since = commits.second.created_at
+        after = commits.second.created_at
 
-        get api("/projects/#{project.id}/repository/commits?since=#{since.utc.iso8601}", user)
+        get api("/projects/#{project.id}/repository/commits?since=#{after.utc.iso8601}", user)
 
         expect(json_response.size).to eq 2
         expect(json_response.first["id"]).to eq(commits.first.id)
         expect(json_response.second["id"]).to eq(commits.second.id)
       end
+
+      it 'include correct pagination headers' do
+        commits = project.repository.commits("master")
+        after = commits.second.created_at
+        commit_count = project.repository.count_commits(ref: 'master', after: after).to_s
+
+        get api("/projects/#{project.id}/repository/commits?since=#{after.utc.iso8601}", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "until optional parameter" do
@@ -65,6 +88,18 @@ describe API::Commits, api: true  do
         expect(json_response.first["id"]).to eq(commits.second.id)
         expect(json_response.second["id"]).to eq(commits.third.id)
       end
+
+      it 'include correct pagination headers' do
+        commits = project.repository.commits("master")
+        before = commits.second.created_at
+        commit_count = project.repository.count_commits(ref: 'master', before: before).to_s
+
+        get api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
     end
 
     context "invalid xmlschema date parameters" do
@@ -79,11 +114,66 @@ describe API::Commits, api: true  do
     context "path optional parameter" do
       it "returns project commits matching provided path parameter" do
         path = 'files/ruby/popen.rb'
+        commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
 
         get api("/projects/#{project.id}/repository/commits?path=#{path}", user)
 
         expect(json_response.size).to eq(3)
         expect(json_response.first["id"]).to eq("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+      end
+
+      it 'include correct pagination headers' do
+        path = 'files/ruby/popen.rb'
+        commit_count = project.repository.count_commits(ref: 'master', path: path).to_s
+
+        get api("/projects/#{project.id}/repository/commits?path=#{path}", user)
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eql('1')
+      end
+    end
+
+    context 'with pagination params' do
+      let(:page) { 1 }
+      let(:per_page) { 5 }
+      let(:ref_name) { 'master' }
+      let!(:request) do
+        get api("/projects/#{project.id}/repository/commits?page=#{page}&per_page=#{per_page}&ref_name=#{ref_name}", user)
+      end
+
+      it 'returns correct headers' do
+        commit_count = project.repository.count_commits(ref: ref_name).to_s
+
+        expect(response).to include_pagination_headers
+        expect(response.headers['X-Total']).to eq(commit_count)
+        expect(response.headers['X-Page']).to eq('1')
+        expect(response.headers['Link']).to match(/page=1&per_page=5/)
+        expect(response.headers['Link']).to match(/page=2&per_page=5/)
+      end
+
+      context 'viewing the first page' do
+        it 'returns the first 5 commits' do
+          commit = project.repository.commit
+
+          expect(json_response.size).to eq(per_page)
+          expect(json_response.first['id']).to eq(commit.id)
+          expect(response.headers['X-Page']).to eq('1')
+        end
+      end
+
+      context 'viewing the third page' do
+        let(:page) { 3 }
+
+        it 'returns the third 5 commits' do
+          commit = project.repository.commits('HEAD', offset: (page - 1) * per_page).first
+
+          expect(json_response.size).to eq(per_page)
+          expect(json_response.first['id']).to eq(commit.id)
+          expect(response.headers['X-Page']).to eq('3')
+        end
       end
     end
   end