Merge branch...

Merge branch '118487-no-limits-on-size-of-diffs-returned-by-the-projects-id-repository-compare-api-endpoint' into 'master'

Resolve "No limits on size of diffs returned by the /projects/:id/repository/compare API endpoint"

Closes #118487

See merge request gitlab-org/gitlab!22658

changelogs/unreleased/118487-no-limits-on-size-of-diffs-returned-by-the-projects-id-repository-.yml

+---
+title: Limit size of diffs returned by /projects/:id/repository/compare API endpoint
+merge_request: 22658
+author:
+type: fixed

doc/api/repositories.md

@@ -128,7 +128,7 @@ curl --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.com/api/v4/pro
 ## Compare branches, tags or commits
 
 This endpoint can be accessed without authentication if the repository is
-publicly accessible.
+publicly accessible. Note that diffs could have an empty diff string if [diff limits](../development/diffs.html#diff-limits) are reached.
 
 ```
 GET /projects/:id/repository/compare

lib/api/entities.rb

@@ -1256,20 +1256,20 @@ module API
     end
 
     class Compare < Grape::Entity
-      expose :commit, using: Entities::Commit do |compare, options|
-        ::Commit.decorate(compare.commits, nil).last
+      expose :commit, using: Entities::Commit do |compare, _|
+        compare.commits.last
       end
 
-      expose :commits, using: Entities::Commit do |compare, options|
-        ::Commit.decorate(compare.commits, nil)
+      expose :commits, using: Entities::Commit do |compare, _|
+        compare.commits
       end
 
-      expose :diffs, using: Entities::Diff do |compare, options|
-        compare.diffs(limits: false).to_a
+      expose :diffs, using: Entities::Diff do |compare, _|
+        compare.diffs.diffs.to_a
       end
 
-      expose :compare_timeout do |compare, options|
-        compare.diffs.overflow?
+      expose :compare_timeout do |compare, _|
+        compare.diffs.diffs.overflow?
       end
 
       expose :same, as: :compare_same_ref

lib/api/repositories.rb

@@ -103,8 +103,13 @@ module API
         optional :straight, type: Boolean, desc: 'Comparison method, `true` for direct comparison between `from` and `to` (`from`..`to`), `false` to compare using merge base (`from`...`to`)', default: false
       end
       get ':id/repository/compare' do
-        compare = Gitlab::Git::Compare.new(user_project.repository.raw_repository, params[:from], params[:to], straight: params[:straight])
+        compare = CompareService.new(user_project, params[:to]).execute(user_project, params[:from], straight: params[:straight])
+
+        if compare
           present compare, with: Entities::Compare
+        else
+          not_found!("Ref")
+        end
       end
 
       desc 'Get repository contributors' do

spec/requests/api/repositories_spec.rb

@@ -362,6 +362,29 @@ describe API::Repositories do
         expect(json_response['diffs']).to be_empty
         expect(json_response['compare_same_ref']).to be_truthy
       end
+
+      it "returns an empty string when the diff overflows" do
+        stub_const('Gitlab::Git::DiffCollection::DEFAULT_LIMITS', { max_files: 2, max_lines: 2 })
+
+        get api(route, current_user), params: { from: 'master', to: 'feature' }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['commits']).to be_present
+        expect(json_response['diffs']).to be_present
+        expect(json_response['diffs'].first['diff']).to be_empty
+      end
+
+      it "returns a 404 when from ref is unknown" do
+        get api(route, current_user), params: { from: 'unknown_ref', to: 'master' }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+
+      it "returns a 404 when to ref is unknown" do
+        get api(route, current_user), params: { from: 'master', to: 'unknown_ref' }
+
+        expect(response).to have_gitlab_http_status(404)
+      end
     end
 
     context 'when unauthenticated', 'and project is public' do