Skip to content

Pass on change size, not total size

What does this MR do and why?

This MR aims to fix the logic that calculates the size of incoming LFS objects to calculate what's incoming, not the total existing objects.

LFS pushes include existing LFS objects, so the size calculation was incorrectly reporting users to be over the storage limits.

This MR adjusts the logic to calculating the actual push size based on existing LFS objects.

See this thread for more detail

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

This is quite a convoluted process, documented in this comment.

Otherwise, a quicker check would be to apply the newly added tests to the master branch and see that the test fails, but when on this branch, they pass.

diff for tests
diff --git a/ee/spec/requests/lfs_http_spec.rb b/ee/spec/requests/lfs_http_spec.rb
index 95475e5dafc5..2095ec5fa5fd 100644
--- a/ee/spec/requests/lfs_http_spec.rb
+++ b/ee/spec/requests/lfs_http_spec.rb
@@ -180,6 +180,82 @@
             end
           end
 
+          context 'when push includes an lfs object that already exists' do
+            let(:existing_object) { create(:lfs_object, :with_file, size: 150.megabytes) }
+
+            let(:body) do
+              {
+                'operation' => 'upload',
+                'objects' => [
+                  {
+                    'oid' => sample_oid,
+                    'size' => sample_size
+                  },
+                  {
+                    'oid' => existing_object.oid,
+                    'size' => existing_object.size
+                  }
+                ]
+              }
+            end
+
+            before do
+              create(
+                :lfs_objects_project,
+                project: project,
+                lfs_object: existing_object
+              )
+            end
+
+            it 'uses new objects for change size' do
+              expect_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+                expect(checker).to receive(:changes_will_exceed_size_limit?).with(sample_size, project)
+              end
+
+              batch_request
+            end
+
+            context 'when the push will not go over the repository size limit' do
+              let(:sample_size) { 75.megabytes }
+
+              before do
+                allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+                  allow(checker).to receive_messages(
+                    enabled?: true,
+                    current_size: 150.megabytes,
+                    limit: 300.megabytes
+                  )
+                end
+              end
+
+              it 'responds with status 200' do
+                batch_request
+
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+            end
+
+            context 'when the push will go over the repository size limit' do
+              let(:sample_size) { 275.megabytes }
+
+              before do
+                allow_next_instance_of(Gitlab::RepositorySizeChecker) do |checker|
+                  allow(checker).to receive_messages(
+                    enabled?: true,
+                    current_size: 150.megabytes,
+                    limit: 300.megabytes
+                  )
+                end
+              end
+
+              it 'responds with status 406' do
+                batch_request
+
+                expect(response).to have_gitlab_http_status(:not_acceptable)
+              end
+            end
+          end
+
           context 'when pushing to a subgroup project' do
             let(:sample_size) { 150.megabytes }
             let(:sample_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
@@ -274,7 +350,7 @@
     end
   end
 
-  describe 'when pushing a lfs object' do
+  describe 'when pushing an lfs object' do
     before do
       enable_lfs
     end
_Numbered steps to set up and validate the change are strongly suggested._

Related to customers-gitlab-com#3808 (closed)

Edited by Vijay Hawoldar

Merge request reports

Loading