Merge branch 'refactor-repository-size-error' into 'master'

Refactor repository size error message

See merge request gitlab-org/gitlab!27419

app/models/concerns/has_repository.rb

@@ -111,4 +111,8 @@ module HasRepository
   def web_url(only_path: nil)
     raise NotImplementedError
   end
+
+  def repository_size_checker
+    raise NotImplementedError
+  end
 end

ee/app/controllers/concerns/ee/lfs_request.rb

@@ -15,13 +15,15 @@ module EE
 
     override :limit_exceeded?
     def limit_exceeded?
-      project.above_size_limit? || objects_exceed_repo_limit?
+      strong_memoize(:limit_exceeded) do
+        size_checker.changes_will_exceed_size_limit?(lfs_push_size)
+      end
     end
 
     def render_size_error
       render(
         json: {
-          message: ::Gitlab::RepositorySizeError.new(project).push_error(@exceeded_limit), # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          message: size_checker.error_message.push_error(lfs_push_size),
           documentation_url: help_url
         },
         content_type: ::LfsRequest::CONTENT_TYPE,
@@ -29,18 +31,14 @@ module EE
       )
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def objects_exceed_repo_limit?
-      return false unless project.size_limit_enabled?
-
-      strong_memoize(:limit_exceeded) do
-        lfs_push_size = objects.sum { |o| o[:size] }
-        size_with_lfs_push = project.repository_and_lfs_size + lfs_push_size
+    def size_checker
+      project.repository_size_checker
+    end
 
-        @exceeded_limit = size_with_lfs_push - project.actual_size_limit # rubocop:disable Gitlab/ModuleWithInstanceVariables
-        @exceeded_limit > 0 # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    def lfs_push_size
+      strong_memoize(:lfs_push_size) do
+        objects.sum { |o| o[:size] } # rubocop: disable CodeReuse/ActiveRecord
       end
     end
-    # rubocop: enable CodeReuse/ActiveRecord
   end
 end

ee/app/helpers/ee/projects_helper.rb

@@ -180,10 +180,6 @@ module EE
       "The total size of this project's repository #{show_lfs} will be limited to this size. 0 for unlimited. Leave empty to inherit the group/global value."
     end
 
-    def project_above_size_limit_message
-      ::Gitlab::RepositorySizeError.new(@project).above_size_limit_message
-    end
-
     override :membership_locked?
     def membership_locked?
       group = @project.group

ee/app/models/ee/project.rb

@@ -496,6 +496,16 @@ module EE
       ::Gitlab::UrlSanitizer.new(bare_url, credentials: { user: import_data&.user }).full_url
     end
 
+    def repository_size_checker
+      strong_memoize(:repository_size_checker) do
+        ::Gitlab::RepositorySizeChecker.new(
+          current_size_proc: -> { statistics.total_repository_size },
+          limit: (repository_size_limit || namespace.actual_size_limit),
+          enabled: License.feature_available?(:repository_size_limit)
+        )
+      end
+    end
+
     def username_only_import_url=(value)
       unless ::Gitlab::UrlSanitizer.valid?(value)
         self.import_url = value
@@ -512,38 +522,6 @@ module EE
       username_only_import_url
     end
 
-    def repository_and_lfs_size
-      statistics.total_repository_size
-    end
-
-    def above_size_limit?
-      return false unless size_limit_enabled?
-
-      repository_and_lfs_size > actual_size_limit
-    end
-
-    def size_to_remove
-      repository_and_lfs_size - actual_size_limit
-    end
-
-    def actual_size_limit
-      return namespace.actual_size_limit if repository_size_limit.nil?
-
-      repository_size_limit
-    end
-
-    def size_limit_enabled?
-      return false unless License.feature_available?(:repository_size_limit)
-
-      actual_size_limit != 0
-    end
-
-    def changes_will_exceed_size_limit?(size_in_bytes)
-      size_limit_enabled? &&
-        (size_in_bytes > actual_size_limit ||
-         size_in_bytes + repository_and_lfs_size > actual_size_limit)
-    end
-
     def remove_import_data
       super unless mirror?
     end

ee/app/services/ee/commits/create_service.rb

@@ -15,8 +15,10 @@ module EE
       end
 
       def validate_repository_size!
-        if project.above_size_limit?
-          raise_error(::Gitlab::RepositorySizeError.new(project).commit_error)
+        size_checker = project.repository_size_checker
+
+        if size_checker.above_size_limit?
+          raise_error(size_checker.error_message.commit_error)
         end
       end
     end

ee/app/services/ee/merge_requests/merge_base_service.rb

@@ -49,10 +49,10 @@ module EE
       private
 
       def check_size_limit
-        if merge_request.target_project.above_size_limit?
-          message = ::Gitlab::RepositorySizeError.new(merge_request.target_project).merge_error
+        size_checker = merge_request.target_project.repository_size_checker
 
-          raise ::MergeRequests::MergeService::MergeError, message
+        if size_checker.above_size_limit?
+          raise ::MergeRequests::MergeService::MergeError, size_checker.error_message.merge_error
         end
       end
 

ee/app/views/projects/_above_size_limit_warning.html.haml

-- if project.above_size_limit?
+- size_checker = project.repository_size_checker
+- if size_checker.above_size_limit?
   .alert.alert-warning.d-none.d-sm-block
-    = project_above_size_limit_message
+    = size_checker.error_message.above_size_limit_message

ee/app/views/projects/merge_requests/widget/open/_size_limit_reached.html.haml

-- error_messages = Gitlab::RepositorySizeError.new(@project)
+- error_messages = @project.repository_size_checker.error_message
 
 %h4.size-limit-reached
   = icon("exclamation-triangle")

ee/lib/ee/gitlab/git_access.rb

@@ -97,8 +97,8 @@ module EE
       end
 
       def check_size_before_push!
-        if check_size_limit? && project.above_size_limit?
-          raise ::Gitlab::GitAccess::ForbiddenError, ::Gitlab::RepositorySizeError.new(project).push_error
+        if check_size_limit? && size_checker.above_size_limit?
+          raise ::Gitlab::GitAccess::ForbiddenError, size_checker.error_message.push_error
         end
       end
 
@@ -153,17 +153,21 @@ module EE
       end
 
       def check_size_against_limit(size)
-        if project.changes_will_exceed_size_limit?(size)
-          raise ::Gitlab::GitAccess::ForbiddenError, ::Gitlab::RepositorySizeError.new(project).new_changes_error
+        if size_checker.changes_will_exceed_size_limit?(size)
+          raise ::Gitlab::GitAccess::ForbiddenError, size_checker.error_message.new_changes_error
         end
       end
 
       def check_size_limit?
         strong_memoize(:check_size_limit) do
-          project.size_limit_enabled? &&
+          size_checker.enabled? &&
             changes_list.any? { |change| !::Gitlab::Git.blank_ref?(change[:newrev]) }
         end
       end
+
+      def size_checker
+        project.repository_size_checker
+      end
     end
   end
 end

ee/spec/controllers/projects_controller_spec.rb

@@ -17,7 +17,9 @@ describe ProjectsController do
     render_views
 
     it 'shows the over size limit warning message if above_size_limit' do
-      allow_any_instance_of(EE::Project).to receive(:above_size_limit?).and_return(true)
+      allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+        expect(checker).to receive(:above_size_limit?).and_return(true)
+      end
       allow(controller).to receive(:current_user).and_return(user)
 
       get :show, params: { namespace_id: public_project.namespace.path, id: public_project.path }

ee/spec/lib/gitlab/git_access_spec.rb

@@ -186,8 +186,8 @@ describe Gitlab::GitAccess do
       repository.delete_branch('2-mb-file')
       repository.delete_branch('wip')
 
-      allow(project).to receive(:repository_and_lfs_size).and_return(repository_size)
       project.update_attribute(:repository_size_limit, repository_size_limit)
+      allow(project.repository_size_checker).to receive_messages(current_size: repository_size)
     end
 
     shared_examples_for 'a push to repository over the limit' do

ee/spec/models/project_spec.rb

@@ -969,48 +969,6 @@ describe Project do
     end
   end
 
-  describe '#size_limit_enabled?' do
-    let(:project) { create(:project) }
-
-    context 'when repository_size_limit is not configured' do
-      it 'is disabled' do
-        expect(project.size_limit_enabled?).to be_falsey
-      end
-    end
-
-    context 'when repository_size_limit is configured' do
-      before do
-        project.update(repository_size_limit: 1024)
-      end
-
-      context 'with an EES license' do
-        let!(:license) { create(:license, plan: License::STARTER_PLAN) }
-
-        it 'is enabled' do
-          expect(project.size_limit_enabled?).to be_truthy
-        end
-      end
-
-      context 'with an EEP license' do
-        let!(:license) { create(:license, plan: License::PREMIUM_PLAN) }
-
-        it 'is enabled' do
-          expect(project.size_limit_enabled?).to be_truthy
-        end
-      end
-
-      context 'without a License' do
-        before do
-          License.destroy_all # rubocop: disable DestroyAll
-        end
-
-        it 'is disabled' do
-          expect(project.size_limit_enabled?).to be_falsey
-        end
-      end
-    end
-  end
-
   describe '#service_desk_enabled?' do
     let!(:license) { create(:license, plan: License::PREMIUM_PLAN) }
     let(:namespace) { create(:namespace) }
@@ -2121,77 +2079,71 @@ describe Project do
     end
   end
 
-  describe 'repository size restrictions' do
+  describe '#repository_size_checker' do
     let(:project) { build(:project) }
+    let(:checker) { project.repository_size_checker }
 
-    before do
-      allow_any_instance_of(ApplicationSetting).to receive(:repository_size_limit).and_return(50)
-    end
+    describe '#current_size' do
+      let(:project) { create(:project) }
 
-    describe '#changes_will_exceed_size_limit?' do
-      before do
-        allow(project).to receive(:repository_and_lfs_size).and_return(49)
-      end
-      it 'returns true when changes go over' do
-        expect(project.changes_will_exceed_size_limit?(5)).to be_truthy
-      end
-    end
+      it 'returns the total repository and lfs size' do
+        allow(project.statistics).to receive(:total_repository_size).and_return(80)
 
-    describe '#actual_size_limit' do
-      it 'returns the limit set in the application settings' do
-        expect(project.actual_size_limit).to eq(50)
+        expect(checker.current_size).to eq(80)
+      end
     end
 
-      it 'returns the value set in the group' do
-        group = create(:group, repository_size_limit: 100)
-        project.update_attribute(:namespace_id, group.id)
+    describe '#limit' do
+      it 'returns the value set in the namespace when available' do
+        allow(project.namespace).to receive(:actual_size_limit).and_return(100)
 
-        expect(project.actual_size_limit).to eq(100)
+        expect(checker.limit).to eq(100)
       end
 
-      it 'returns the value set locally' do
-        project.update_attribute(:repository_size_limit, 75)
+      it 'returns the value set locally when available' do
+        project.repository_size_limit = 200
 
-        expect(project.actual_size_limit).to eq(75)
+        expect(checker.limit).to eq(200)
       end
     end
 
-    describe '#size_limit_enabled?' do
-      it 'returns false when disabled' do
-        project.update_attribute(:repository_size_limit, 0)
+    describe '#enabled?' do
+      it 'returns true when not equal to zero' do
+        project.repository_size_limit = 1
 
-        expect(project.size_limit_enabled?).to be_falsey
+        expect(checker.enabled?).to be_truthy
       end
 
-      it 'returns true when a limit is set' do
-        project.update_attribute(:repository_size_limit, 75)
+      it 'returns false when equals to zero' do
+        project.repository_size_limit = 0
 
-        expect(project.size_limit_enabled?).to be_truthy
-      end
+        expect(checker.enabled?).to be_falsey
       end
 
-    describe '#above_size_limit?' do
-      let(:project) do
-        create(:project,
-               statistics: build(:project_statistics))
+      context 'when repository_size_limit is configured' do
+        before do
+          project.repository_size_limit = 1
         end
 
-      it 'returns true when above the limit' do
-        allow(project).to receive(:repository_and_lfs_size).and_return(100)
-
-        expect(project.above_size_limit?).to be_truthy
+        context 'when license feature enabled' do
+          before do
+            stub_licensed_features(repository_size_limit: true)
           end
 
-      it 'returns false when not over the limit' do
-        expect(project.above_size_limit?).to be_falsey
+          it 'is enabled' do
+            expect(checker.enabled?).to be_truthy
           end
         end
 
-    describe '#size_to_remove' do
-      it 'returns the correct value' do
-        allow(project).to receive(:repository_and_lfs_size).and_return(100)
+        context 'when license feature disabled' do
+          before do
+            stub_licensed_features(repository_size_limit: false)
+          end
 
-        expect(project.size_to_remove).to eq(50)
+          it 'is disabled' do
+            expect(checker.enabled?).to be_falsey
+          end
+        end
       end
     end
   end
@@ -2282,19 +2234,6 @@ describe Project do
     end
   end
 
-  describe '#repository_and_lfs_size' do
-    let(:project) { create(:project, :repository) }
-    let(:size) { 50 }
-
-    before do
-      allow(project.statistics).to receive(:total_repository_size).and_return(size)
-    end
-
-    it 'returns the total repository and lfs size' do
-      expect(project.repository_and_lfs_size).to eq(size)
-    end
-  end
-
   describe '#approver_group_ids=' do
     let(:project) { create(:project) }
 

ee/spec/requests/lfs_http_spec.rb

@@ -49,22 +49,30 @@ describe 'Git LFS API and storage' do
 
         context 'and project is above the limit' do
           let(:update_lfs_permissions) do
-            allow_any_instance_of(EE::Project).to receive_messages(
-              repository_and_lfs_size: 100.megabytes,
-              actual_size_limit: 99.megabytes)
+            allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+              allow(checker).to receive_messages(
+                enabled?: true,
+                current_size: 110.megabytes,
+                limit: 100.megabytes
+              )
+            end
           end
 
           it 'responds with status 406' do
             expect(response).to have_gitlab_http_status(:not_acceptable)
-            expect(json_response['message']).to eql('Your push has been rejected, because this repository has exceeded its size limit of 99 MB by 1 MB. Please contact your GitLab administrator for more information.')
+            expect(json_response['message']).to eql('Your push has been rejected, because this repository has exceeded its size limit of 100 MB by 160 MB. Please contact your GitLab administrator for more information.')
           end
         end
 
         context 'and project will go over the limit' do
           let(:update_lfs_permissions) do
-            allow_any_instance_of(EE::Project).to receive_messages(
-              repository_and_lfs_size: 200.megabytes,
-              actual_size_limit: 300.megabytes)
+            allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+              allow(checker).to receive_messages(
+                enabled?: true,
+                current_size: 200.megabytes,
+                limit: 300.megabytes
+              )
+            end
           end
 
           it 'responds with status 406' do
@@ -117,9 +125,9 @@ describe 'Git LFS API and storage' do
 
           context 'and project has limit enabled but will stay under the limit' do
             before do
-              allow_any_instance_of(EE::Project).to receive_messages(
-                actual_size_limit: 200,
-                size_limit_enabled?: true)
+              allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+                allow(checker).to receive_messages(limit: 200, enabled?: true)
+              end
 
               put_finalize
             end

ee/spec/services/ee/commits/create_service_spec.rb

@@ -18,7 +18,7 @@ describe Commits::CreateService do
     before do
       stub_licensed_features(repository_size_limit: true)
       project.update!(repository_size_limit: 1)
-      allow(project).to receive(:repository_and_lfs_size).and_return(2)
+      allow(project.repository_size_checker).to receive(:current_size).and_return(2)
     end
 
     subject(:result) { service.execute }

ee/spec/services/merge_requests/merge_service_spec.rb

@@ -15,7 +15,7 @@ describe MergeRequests::MergeService do
   describe '#execute' do
     context 'project has exceeded size limit' do
       before do
-        allow(project).to receive(:above_size_limit?).and_return(true)
+        allow(project.repository_size_checker).to receive(:above_size_limit?).and_return(true)
       end
 
       it 'persists the correct error message' do

lib/gitlab/repository_size_checker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  # Centralized class for repository size related calculations.
+  class RepositorySizeChecker
+    attr_reader :limit
+
+    def initialize(current_size_proc:, limit:, enabled: true)
+      @current_size_proc = current_size_proc
+      @limit = limit
+      @enabled = enabled && limit != 0
+    end
+
+    def current_size
+      @current_size ||= @current_size_proc.call
+    end
+
+    def enabled?
+      @enabled
+    end
+
+    def above_size_limit?
+      return false unless enabled?
+
+      current_size > limit
+    end
+
+    # @param change_size [int] in bytes
+    def changes_will_exceed_size_limit?(change_size)
+      return false unless enabled?
+
+      change_size > limit || exceeded_size(change_size) > 0
+    end
+
+    # @param change_size [int] in bytes
+    def exceeded_size(change_size = 0)
+      current_size + change_size - limit
+    end
+
+    def error_message
+      @error_message_object ||= Gitlab::RepositorySizeErrorMessage.new(self)
+    end
+  end
+end

ee/lib/gitlab/repository_size_error.rb → lib/gitlab/repository_size_error_message.rb

 # frozen_string_literal: true
 
 module Gitlab
-  class RepositorySizeError
+  class RepositorySizeErrorMessage
     include ActiveSupport::NumberHelper
 
-    attr_reader :project
+    delegate :current_size, :limit, :exceeded_size, to: :@checker
 
-    def initialize(project)
-      @project = project
-    end
-
-    def to_s
-      "The size of this repository (#{current_size}) exceeds the limit of #{limit} by #{size_to_remove}."
+    # @param checher [RepositorySizeChecker]
+    def initialize(checker)
+      @checker = checker
     end
 
     def commit_error
@@ -22,12 +19,12 @@ module Gitlab
       "This merge request cannot be merged, #{base_message}"
     end
 
-    def push_error(exceeded_limit = nil)
-      "Your push has been rejected, #{base_message(exceeded_limit)}. #{more_info_message}"
+    def push_error(change_size = 0)
+      "Your push has been rejected, #{base_message(change_size)}. #{more_info_message}"
     end
 
     def new_changes_error
-      "Your push to this repository would cause it to exceed the size limit of #{limit} so it has been rejected. #{more_info_message}"
+      "Your push to this repository would cause it to exceed the size limit of #{formatted(limit)} so it has been rejected. #{more_info_message}"
     end
 
     def more_info_message
@@ -35,28 +32,16 @@ module Gitlab
     end
 
     def above_size_limit_message
-      "#{self} You won't be able to push new code to this project. #{more_info_message}"
+      "The size of this repository (#{formatted(current_size)}) exceeds the limit of #{formatted(limit)} by #{formatted(exceeded_size)}. You won't be able to push new code to this project. #{more_info_message}"
     end
 
     private
 
-    def base_message(exceeded_limit = nil)
-      "because this repository has exceeded its size limit of #{limit} by #{size_to_remove(exceeded_limit)}"
-    end
-
-    def current_size
-      format_number(project.repository_and_lfs_size)
-    end
-
-    def limit
-      format_number(project.actual_size_limit)
-    end
-
-    def size_to_remove(exceeded_limit = nil)
-      format_number(exceeded_limit || project.size_to_remove)
+    def base_message(change_size = 0)
+      "because this repository has exceeded its size limit of #{formatted(limit)} by #{formatted(exceeded_size(change_size))}"
     end
 
-    def format_number(number)
+    def formatted(number)
       number_to_human_size(number, delimiter: ',', precision: 2)
     end
   end

spec/lib/gitlab/repository_size_checker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::RepositorySizeChecker do
+  let(:current_size) { 0 }
+  let(:limit) { 50 }
+  let(:enabled) { true }
+
+  subject do
+    described_class.new(
+      current_size_proc: -> { current_size },
+      limit: limit,
+      enabled: enabled
+    )
+  end
+
+  describe '#enabled?' do
+    context 'when enabled' do
+      it 'returns true' do
+        expect(subject.enabled?).to be_truthy
+      end
+    end
+
+    context 'when limit is zero' do
+      let(:limit) { 0 }
+
+      it 'returns false' do
+        expect(subject.enabled?).to be_falsey
+      end
+    end
+  end
+
+  describe '#changes_will_exceed_size_limit?' do
+    let(:current_size) { 49 }
+
+    it 'returns true when changes go over' do
+      expect(subject.changes_will_exceed_size_limit?(2)).to be_truthy
+    end
+
+    it 'returns false when changes do not go over' do
+      expect(subject.changes_will_exceed_size_limit?(1)).to be_falsey
+    end
+  end
+
+  describe '#above_size_limit?' do
+    context 'when size is above the limit' do
+      let(:current_size) { 100 }
+
+      it 'returns true' do
+        expect(subject.above_size_limit?).to be_truthy
+      end
+    end
+
+    it 'returns false when not over the limit' do
+      expect(subject.above_size_limit?).to be_falsey
+    end
+  end
+
+  describe '#exceeded_size' do
+    context 'when current size is below or equal to the limit' do
+      let(:current_size) { 50 }
+
+      it 'returns zero' do
+        expect(subject.exceeded_size).to eq(0)
+      end
+    end
+
+    context 'when current size is over the limit' do
+      let(:current_size) { 51 }
+
+      it 'returns zero' do
+        expect(subject.exceeded_size).to eq(1)
+      end
+    end
+
+    context 'when change size will be over the limit' do
+      let(:current_size) { 50 }
+
+      it 'returns zero' do
+        expect(subject.exceeded_size(1)).to eq(1)
+      end
+    end
+
+    context 'when change size will not be over the limit' do
+      let(:current_size) { 49 }
+
+      it 'returns zero' do
+        expect(subject.exceeded_size(1)).to eq(0)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/repository_size_error_spec.rb → spec/lib/gitlab/repository_size_error_message_spec.rb

@@ -2,25 +2,18 @@
 
 require 'spec_helper'
 
-describe Gitlab::RepositorySizeError do
-  let(:project) do
-    create(:project, statistics: build(:project_statistics, repository_size: 15.megabytes))
+describe Gitlab::RepositorySizeErrorMessage do
+  let(:checker) do
+    Gitlab::RepositorySizeChecker.new(
+      current_size_proc: -> { 15.megabytes },
+      limit: 10.megabytes
+    )
   end
 
-  let(:message) { described_class.new(project) }
+  let(:message) { checker.error_message }
   let(:base_message) { 'because this repository has exceeded its size limit of 10 MB by 5 MB' }
 
-  before do
-    allow(project).to receive(:actual_size_limit).and_return(10.megabytes)
-  end
-
   describe 'error messages' do
-    describe '#to_s' do
-      it 'returns the correct message' do
-        expect(message.to_s).to eq('The size of this repository (15 MB) exceeds the limit of 10 MB by 5 MB.')
-      end
-    end
-
     describe '#commit_error' do
       it 'returns the correct message' do
         expect(message.commit_error).to eq("Your changes could not be committed, #{base_message}")
@@ -40,7 +33,7 @@ describe Gitlab::RepositorySizeError do
         end
 
         it 'returns the correct message' do
-          expect(message.push_error(15.megabytes))
+          expect(message.push_error(10.megabytes))
             .to eq("Your push has been rejected, #{rejection_message}. #{message.more_info_message}")
         end
       end