Commit d13451cd authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Returns correct header data for commits endpoint

parent c46f933b
...@@ -499,10 +499,12 @@ class Repository ...@@ -499,10 +499,12 @@ class Repository
cache_method :commit_count, fallback: 0 cache_method :commit_count, fallback: 0
def commit_count_for_ref(ref) def commit_count_for_ref(ref)
return 0 if empty? return 0 unless exists?
cache.fetch(:"commit_count_#{ref}") do begin
raw_repository.commit_count(ref) cache.fetch(:"commit_count_#{ref}") { raw_repository.commit_count(ref) }
rescue Rugged::ReferenceError
0
end end
end end
......
---
title: Fix pagination headers for repository commits api endpoint
merge_request:
author: George Andrinopoulos
---
title: "GET 'projects/:id/repository/commits' endpoint improvements"
merge_request: 9679
author: George Andrinopoulos, Jordan Ryan Reuter
---
title: Add pagination to project commits API endpoint
merge_request: 5298
author: Jordan Ryan Reuter
...@@ -71,3 +71,6 @@ changes are in V4: ...@@ -71,3 +71,6 @@ changes are in V4:
- Simplify project payload exposed on Environment endpoints [!9675](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9675) - Simplify project payload exposed on Environment endpoints [!9675](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9675)
- API uses merge request `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the merge requests, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530) - API uses merge request `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the merge requests, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530)
- API uses issue `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the issues, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530) - API uses issue `IID`s (internal ID, as in the web UI) rather than `ID`s. This affects the issues, award emoji, todos, and time tracking APIs. [!9530](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9530)
- Change initial page from `0` to `1` on `GET projects/:id/repository/commits` (like on the rest of the API) [!9679] (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9679)
- Return correct `Link` header data for `GET projects/:id/repository/commits` [!9679] (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9679)
...@@ -18,22 +18,32 @@ module API ...@@ -18,22 +18,32 @@ module API
optional :ref_name, type: String, desc: 'The name of a repository branch or tag, if not given the default branch is used' optional :ref_name, type: String, desc: 'The name of a repository branch or tag, if not given the default branch is used'
optional :since, type: DateTime, desc: 'Only commits after or on this date will be returned' optional :since, type: DateTime, desc: 'Only commits after or on this date will be returned'
optional :until, type: DateTime, desc: 'Only commits before or on this date will be returned' optional :until, type: DateTime, desc: 'Only commits before or on this date will be returned'
optional :page, type: Integer, default: 0, desc: 'The page for pagination'
optional :per_page, type: Integer, default: 20, desc: 'The number of results per page'
optional :path, type: String, desc: 'The file path' optional :path, type: String, desc: 'The file path'
use :pagination
end end
get ":id/repository/commits" do get ":id/repository/commits" do
ref = params[:ref_name] || user_project.try(:default_branch) || 'master' path = params[:path]
before = params[:until]
after = params[:since]
ref = params[:ref_name] || user_project.try(:default_branch) || 'master'
offset = (params[:page] - 1) * params[:per_page] offset = (params[:page] - 1) * params[:per_page]
commits = user_project.repository.commits(ref, commits = user_project.repository.commits(ref,
path: params[:path], path: path,
limit: params[:per_page], limit: params[:per_page],
offset: offset, offset: offset,
after: params[:since], before: before,
before: params[:until]) after: after)
commit_count =
if path || before || after
user_project.repository.count_commits(ref: ref, path: path, before: before, after: after)
else
# Cacheable commit count.
user_project.repository.commit_count_for_ref(ref)
end
paginated_commits = Kaminari.paginate_array(commits, total_count: commits.size) paginated_commits = Kaminari.paginate_array(commits, total_count: commit_count)
present paginate(paginated_commits), with: Entities::RepoCommit present paginate(paginated_commits), with: Entities::RepoCommit
end end
......
...@@ -292,6 +292,7 @@ module Gitlab ...@@ -292,6 +292,7 @@ module Gitlab
} }
options = default_options.merge(options) options = default_options.merge(options)
options[:limit] ||= 0
options[:offset] ||= 0 options[:offset] ||= 0
actual_ref = options[:ref] || root_ref actual_ref = options[:ref] || root_ref
begin begin
...@@ -354,12 +355,14 @@ module Gitlab ...@@ -354,12 +355,14 @@ module Gitlab
end end
def count_commits(options) def count_commits(options)
cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list) cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list]
cmd += %W(--after=#{options[:after].iso8601}) if options[:after] cmd << "--after=#{options[:after].iso8601}" if options[:after]
cmd += %W(--before=#{options[:before].iso8601}) if options[:before] cmd << "--before=#{options[:before].iso8601}" if options[:before]
cmd += %W(--count #{options[:ref]}) cmd += %W[--count #{options[:ref]}]
cmd += %W(-- #{options[:path]}) if options[:path].present? cmd += %W[-- #{options[:path]}] if options[:path].present?
raw_output = IO.popen(cmd) {|io| io.read }
raw_output = IO.popen(cmd) { |io| io.read }
raw_output.to_i raw_output.to_i
end end
......
...@@ -826,29 +826,26 @@ describe Gitlab::Git::Repository, seed_helper: true do ...@@ -826,29 +826,26 @@ describe Gitlab::Git::Repository, seed_helper: true do
describe '#count_commits' do describe '#count_commits' do
context 'with after timestamp' do context 'with after timestamp' do
options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
it 'returns the number of commits after timestamp' do it 'returns the number of commits after timestamp' do
commits = repository.log(options) options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
expect(repository.count_commits(options)).to eq(commits.size) expect(repository.count_commits(options)).to eq(25)
end end
end end
context 'with before timestamp' do context 'with before timestamp' do
options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
it 'returns the number of commits after timestamp' do it 'returns the number of commits after timestamp' do
commits = repository.log(options) options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
expect(repository.count_commits(options)).to eq(commits.size) expect(repository.count_commits(options)).to eq(9)
end end
end end
context 'with path' do context 'with path' do
options = { ref: 'master', limit: nil, path: "encoding" }
it 'returns the number of commits with path ' do it 'returns the number of commits with path ' do
commits = repository.log(options) options = { ref: 'master', limit: nil, path: "encoding" }
expect(repository.count_commits(options)).to eq(commits.size) expect(repository.count_commits(options)).to eq(2)
end end
end end
end end
......
...@@ -1743,10 +1743,18 @@ describe Repository, models: true do ...@@ -1743,10 +1743,18 @@ describe Repository, models: true do
end end
describe '#commit_count_for_ref' do describe '#commit_count_for_ref' do
let(:project) { create :empty_project }
context 'with a non-existing repository' do context 'with a non-existing repository' do
it 'returns 0' do it 'returns 0' do
expect(repository).to receive(:raw_repository).and_return(nil) expect(project.repository.commit_count_for_ref('master')).to eq(0)
expect(repository.commit_count).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
end end
......
...@@ -19,6 +19,7 @@ describe API::Commits, api: true do ...@@ -19,6 +19,7 @@ describe API::Commits, api: true do
it "returns project commits" do it "returns project commits" do
commit = project.repository.commit commit = project.repository.commit
get api("/projects/#{project.id}/repository/commits", user) get api("/projects/#{project.id}/repository/commits", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
...@@ -27,6 +28,16 @@ describe API::Commits, api: true do ...@@ -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_name']).to eq(commit.committer_name)
expect(json_response.first['committer_email']).to eq(commit.committer_email) expect(json_response.first['committer_email']).to eq(commit.committer_email)
end 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 end
context "unauthorized user" do context "unauthorized user" do
...@@ -39,14 +50,26 @@ describe API::Commits, api: true do ...@@ -39,14 +50,26 @@ describe API::Commits, api: true do
context "since optional parameter" do context "since optional parameter" do
it "returns project commits since provided parameter" do it "returns project commits since provided parameter" do
commits = project.repository.commits("master") 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.size).to eq 2
expect(json_response.first["id"]).to eq(commits.first.id) expect(json_response.first["id"]).to eq(commits.first.id)
expect(json_response.second["id"]).to eq(commits.second.id) expect(json_response.second["id"]).to eq(commits.second.id)
end 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 end
context "until optional parameter" do context "until optional parameter" do
...@@ -65,6 +88,18 @@ describe API::Commits, api: true do ...@@ -65,6 +88,18 @@ describe API::Commits, api: true do
expect(json_response.first["id"]).to eq(commits.second.id) expect(json_response.first["id"]).to eq(commits.second.id)
expect(json_response.second["id"]).to eq(commits.third.id) expect(json_response.second["id"]).to eq(commits.third.id)
end 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 end
context "invalid xmlschema date parameters" do context "invalid xmlschema date parameters" do
...@@ -79,28 +114,44 @@ describe API::Commits, api: true do ...@@ -79,28 +114,44 @@ describe API::Commits, api: true do
context "path optional parameter" do context "path optional parameter" do
it "returns project commits matching provided path parameter" do it "returns project commits matching provided path parameter" do
path = 'files/ruby/popen.rb' 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) get api("/projects/#{project.id}/repository/commits?path=#{path}", user)
expect(json_response.size).to eq(3) expect(json_response.size).to eq(3)
expect(json_response.first["id"]).to eq("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") expect(json_response.first["id"]).to eq("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
expect(response).to include_pagination_headers
expect(response.headers['X-Total']).to eq(commit_count)
end end
end
context 'pagination' do it 'include correct pagination headers' do
it_behaves_like 'a paginated resources' 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)
let(:page) { 0 } 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(:per_page) { 5 }
let(:ref_name) { 'master' } let(:ref_name) { 'master' }
let!(:request) do let!(:request) do
get api("/projects/#{project.id}/repository/commits?page=#{page}&per_page=#{per_page}&ref_name=#{ref_name}", user) get api("/projects/#{project.id}/repository/commits?page=#{page}&per_page=#{per_page}&ref_name=#{ref_name}", user)
end end
it 'returns the commit count in the correct header' do it 'returns correct headers' do
commit_count = project.repository.commit_count_for_ref(ref_name).to_s 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-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 end
context 'viewing the first page' do context 'viewing the first page' do
...@@ -109,17 +160,19 @@ describe API::Commits, api: true do ...@@ -109,17 +160,19 @@ describe API::Commits, api: true do
expect(json_response.size).to eq(per_page) expect(json_response.size).to eq(per_page)
expect(json_response.first['id']).to eq(commit.id) expect(json_response.first['id']).to eq(commit.id)
expect(response.headers['X-Page']).to eq('1')
end end
end end
context 'viewing the second page' do context 'viewing the third page' do
let(:page) { 1 } let(:page) { 3 }
it 'returns the second 5 commits' do it 'returns the third 5 commits' do
commit = project.repository.commits('HEAD', offset: per_page * page).first commit = project.repository.commits('HEAD', offset: (page - 1) * per_page).first
expect(json_response.size).to eq(per_page) expect(json_response.size).to eq(per_page)
expect(json_response.first['id']).to eq(commit.id) expect(json_response.first['id']).to eq(commit.id)
expect(response.headers['X-Page']).to eq('3')
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment