API support for the 'since' and 'until' operators on commit requests

- Parameter validation as ISO8601 format

11 files changed, 71 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e52e52691c2..c9215954378 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,7 @@ v 8.8.0 (unreleased)
   - Replace Devise Async with Devise ActiveJob integration. !3902 (Connor Shea)
   - Allow "NEWS" and "CHANGES" as alternative names for CHANGELOG. !3768 (Connor Shea)
   - Added button to toggle whitespaces changes on diff view
+  - API support for the 'since' and 'until' operators on commit requests (Paco Guzman)
 
 v 8.7.1 (unreleased)
   - Throttle the update of `project.last_activity_at` to 1 minute. !3848
diff --git a/Gemfile.lock b/Gemfile.lock
index 91d89b4875a..02ccbdd8fc3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -351,7 +351,7 @@ GEM
       posix-spawn (~> 0.3)
     gitlab_emoji (0.3.1)
       gemojione (~> 2.2, >= 2.2.1)
-    gitlab_git (10.0.0)
+    gitlab_git (10.0.2)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.7.3)
       github-linguist (~> 4.7.0)
diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb
index 1420b96840c..a52c614b259 100644
--- a/app/controllers/projects/commits_controller.rb
+++ b/app/controllers/projects/commits_controller.rb
@@ -15,7 +15,7 @@ class Projects::CommitsController < Projects::ApplicationController
       if search.present?
         @repository.find_commits_by_message(search, @ref, @path, @limit, @offset).compact
       else
-        @repository.commits(@ref, @path, @limit, @offset)
+        @repository.commits(@ref, path: @path, limit: @limit, offset: @offset)
       end
 
     @note_counts = project.notes.where(commit_id: @commits.map(&:id)).
diff --git a/app/controllers/projects/graphs_controller.rb b/app/controllers/projects/graphs_controller.rb
index d13ea9f34b6..092ef32e6e3 100644
--- a/app/controllers/projects/graphs_controller.rb
+++ b/app/controllers/projects/graphs_controller.rb
@@ -17,7 +17,7 @@ class Projects::GraphsController < Projects::ApplicationController
   end
 
   def commits
-    @commits = @project.repository.commits(@ref, nil, 2000, 0, true)
+    @commits = @project.repository.commits(@ref, limit: 2000, skip_merges: true)
     @commits_graph = Gitlab::Graphs::Commits.new(@commits)
     @commits_per_week_days = @commits_graph.commits_per_week_days
     @commits_per_time = @commits_graph.commits_per_time
@@ -55,7 +55,7 @@ class Projects::GraphsController < Projects::ApplicationController
   private
 
   def fetch_graph
-    @commits = @project.repository.commits(@ref, nil, 6000, 0, true)
+    @commits = @project.repository.commits(@ref, limit: 6000, skip_merges: true)
     @log = []
 
     @commits.each do |commit|
diff --git a/app/models/repository.rb b/app/models/repository.rb
index d495c8d18f5..292acf9044f 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -87,13 +87,15 @@ class Repository
     nil
   end
 
-  def commits(ref, path = nil, limit = nil, offset = nil, skip_merges = false)
+  def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil)
     options = {
       repo: raw_repository,
       ref: ref,
       path: path,
       limit: limit,
       offset: offset,
+      after: after,
+      before: before,
       # --follow doesn't play well with --skip. See:
       # https://gitlab.com/gitlab-org/gitlab-ce/issues/3574#note_3040520
       follow: false,
@@ -575,7 +577,7 @@ class Repository
   end
 
   def contributors
-    commits = self.commits(nil, nil, 2000, 0, true)
+    commits = self.commits(nil, limit: 2000, offset: 0, skip_merges: true)
 
     commits.group_by(&:author_email).map do |email, commits|
       contributor = Gitlab::Contributor.new
diff --git a/doc/api/commits.md b/doc/api/commits.md
index 6341440c58b..57c2e1d9b87 100644
--- a/doc/api/commits.md
+++ b/doc/api/commits.md
@@ -12,6 +12,8 @@ GET /projects/:id/repository/commits
 | --------- | ---- | -------- | ----------- |
 | `id` | integer | yes | The ID of a project |
 | `ref_name` | string | no | The name of a repository branch or tag or if not given the default branch |
+| `since` | string | no | Only commits after or in this date will be returned in ISO 8601 format YYYY-MM-DDTHH:MM:SSZ |
+| `until` | string | no | Only commits before or in this date will be returned in ISO 8601 format YYYY-MM-DDTHH:MM:SSZ |
 
 ```bash
 curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/5/repository/commits"
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index 4544a41b1e3..93a3a5ce089 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -12,14 +12,20 @@ module API
       # Parameters:
       #   id (required) - The ID of a project
       #   ref_name (optional) - The name of a repository branch or tag, if not given the default branch is used
+      #   since (optional) - Only commits after or in this date will be returned
+      #   until (optional) - Only commits before or in this date will be returned
       # Example Request:
       #   GET /projects/:id/repository/commits
       get ":id/repository/commits" do
+        datetime_attributes! :since, :until
+
         page = (params[:page] || 0).to_i
         per_page = (params[:per_page] || 20).to_i
         ref = params[:ref_name] || user_project.try(:default_branch) || 'master'
+        after = params[:since]
+        before = params[:until]
 
-        commits = user_project.repository.commits(ref, nil, per_page, page * per_page)
+        commits = user_project.repository.commits(ref, limit: per_page, offset: page * per_page, after: after, before: before)
         present commits, with: Entities::RepoCommit
       end
 
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 5bbf721321d..40c967453fb 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -183,6 +183,22 @@ module API
       Gitlab::Access.options_with_owner.values.include? level.to_i
     end
 
+    # Checks the occurrences of datetime attributes, each attribute if present in the params hash must be in ISO 8601
+    # format (YYYY-MM-DDTHH:MM:SSZ) or a Bad Request error is invoked.
+    #
+    # Parameters:
+    #   keys (required) - An array consisting of elements that must be parseable as dates from the params hash
+    def datetime_attributes!(*keys)
+      keys.each do |key|
+        begin
+          params[key] = Time.xmlschema(params[key]) if params[key].present?
+        rescue ArgumentError
+          message = "\"" + key.to_s + "\" must be a timestamp in ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ"
+          render_api_error!(message, 400)
+        end
+      end
+    end
+
     def issuable_order_by
       if params["order_by"] == 'updated_at'
         'updated_at'
diff --git a/lib/gitlab/push_data_builder.rb b/lib/gitlab/push_data_builder.rb
index 67622f321a6..c8f12577112 100644
--- a/lib/gitlab/push_data_builder.rb
+++ b/lib/gitlab/push_data_builder.rb
@@ -66,7 +66,7 @@ module Gitlab
       # This method provide a sample data generated with
       # existing project and commits to test webhooks
       def build_sample(project, user)
-        commits = project.repository.commits(project.default_branch, nil, 3)
+        commits = project.repository.commits(project.default_branch, limit: 3)
         ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{project.default_branch}"
         build(project, user, commits.last.id, commits.first.id, ref, commits)
       end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index a306cc4aef8..802b4e579e1 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -561,7 +561,7 @@ describe Repository, models: true do
   end
 
   describe :skip_merged_commit do
-    subject { repository.commits(Gitlab::Git::BRANCH_REF_PREFIX + "'test'", nil, 100, 0, true).map{ |k| k.id } }
+    subject { repository.commits(Gitlab::Git::BRANCH_REF_PREFIX + "'test'", limit: 100, skip_merges: true).map{ |k| k.id } }
 
     it { is_expected.not_to include('e56497bb5f03a90a51293fc6d516788730953899') }
   end
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index e28998d51b5..cb82ca7802d 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -32,6 +32,41 @@ describe API::API, api: true  do
         expect(response.status).to eq(401)
       end
     end
+
+    context "since optional parameter" do
+      it "should return project commits since provided parameter" do
+        commits = project.repository.commits("master")
+        since = commits.second.created_at
+
+        get api("/projects/#{project.id}/repository/commits?since=#{since.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
+    end
+
+    context "until optional parameter" do
+      it "should return project commits until provided parameter" do
+        commits = project.repository.commits("master")
+        before = commits.second.created_at
+
+        get api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user)
+
+        expect(json_response.size).to eq(commits.size - 1)
+        expect(json_response.first["id"]).to eq(commits.second.id)
+        expect(json_response.second["id"]).to eq(commits.third.id)
+      end
+    end
+
+    context "invalid xmlschema date parameters" do
+      it "should return an invalid parameter error message" do
+        get api("/projects/#{project.id}/repository/commits?since=invalid-date", user)
+
+        expect(response.status).to eq(400)
+        expect(json_response['message']).to include "\"since\" must be a timestamp in ISO 8601 format"
+      end
+    end
   end
 
   describe "GET /projects:id/repository/commits/:sha" do