Merge branch 'check-if-fetched-data-does-is-complete' into 'master'

Validate chunk size when persist

See merge request gitlab-org/gitlab-ce!23341

app/models/ci/build_trace_chunk.rb

@@ -15,6 +15,8 @@ module Ci
     WRITE_LOCK_SLEEP = 0.01.seconds
     WRITE_LOCK_TTL = 1.minute
 
+    FailedToPersistDataError = Class.new(StandardError)
+
     # Note: The ordering of this enum is related to the precedence of persist store.
     # The bottom item takes the higest precedence, and the top item takes the lowest precedence.
     enum data_store: {
@@ -109,16 +111,19 @@ module Ci
 
     def unsafe_persist_to!(new_store)
       return if data_store == new_store.to_s
-      raise ArgumentError, 'Can not persist empty data' unless size > 0
 
-      old_store_class = self.class.get_store_class(data_store)
+      current_data = get_data
 
-      get_data.tap do |the_data|
-        self.raw_data = nil
-        self.data_store = new_store
-        unsafe_set_data!(the_data)
+      unless current_data&.bytesize.to_i == CHUNK_SIZE
+        raise FailedToPersistDataError, 'Data is not fullfilled in a bucket'
       end
 
+      old_store_class = self.class.get_store_class(data_store)
+
+      self.raw_data = nil
+      self.data_store = new_store
+      unsafe_set_data!(current_data)
+
       old_store_class.delete_data(self)
     end
 

changelogs/unreleased/check-if-fetched-data-does-is-complete.yml

+---
+title: Validate chunk size when persist
+merge_request: 23341
+author:
+type: fixed

spec/models/ci/build_trace_chunk_spec.rb

@@ -436,32 +436,47 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       let(:data_store) { :redis }
 
       context 'when data exists' do
-        let(:data) { 'Sample data in redis' }
-
         before do
           build_trace_chunk.send(:unsafe_set_data!, data)
         end
 
-        it 'persists the data' do
-          expect(build_trace_chunk.redis?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to eq(data)
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
-          expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
+        context 'when data size reached CHUNK_SIZE' do
+          let(:data) { 'a' * described_class::CHUNK_SIZE }
 
-          subject
+          it 'persists the data' do
+            expect(build_trace_chunk.redis?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to eq(data)
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
+
+            subject
 
-          expect(build_trace_chunk.fog?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+            expect(build_trace_chunk.fog?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+          end
+
+          it_behaves_like 'Atomic operation'
         end
 
-        it_behaves_like 'Atomic operation'
+        context 'when data size has not reached CHUNK_SIZE' do
+          let(:data) { 'Sample data in redis' }
+
+          it 'does not persist the data and the orignal data is intact' do
+            expect { subject }.to raise_error(described_class::FailedToPersistDataError)
+
+            expect(build_trace_chunk.redis?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to eq(data)
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
+          end
+        end
       end
 
       context 'when data does not exist' do
         it 'does not persist' do
-          expect { subject }.to raise_error('Can not persist empty data')
+          expect { subject }.to raise_error(described_class::FailedToPersistDataError)
         end
       end
     end
@@ -470,32 +485,47 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       let(:data_store) { :database }
 
       context 'when data exists' do
-        let(:data) { 'Sample data in database' }
-
         before do
           build_trace_chunk.send(:unsafe_set_data!, data)
         end
 
-        it 'persists the data' do
-          expect(build_trace_chunk.database?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to eq(data)
-          expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
+        context 'when data size reached CHUNK_SIZE' do
+          let(:data) { 'a' * described_class::CHUNK_SIZE }
 
-          subject
+          it 'persists the data' do
+            expect(build_trace_chunk.database?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to eq(data)
+            expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
 
-          expect(build_trace_chunk.fog?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+            subject
+
+            expect(build_trace_chunk.fog?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+          end
+
+          it_behaves_like 'Atomic operation'
         end
 
-        it_behaves_like 'Atomic operation'
+        context 'when data size has not reached CHUNK_SIZE' do
+          let(:data) { 'Sample data in database' }
+
+          it 'does not persist the data and the orignal data is intact' do
+            expect { subject }.to raise_error(described_class::FailedToPersistDataError)
+
+            expect(build_trace_chunk.database?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to eq(data)
+            expect { Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk) }.to raise_error(Excon::Error::NotFound)
+          end
+        end
       end
 
       context 'when data does not exist' do
         it 'does not persist' do
-          expect { subject }.to raise_error('Can not persist empty data')
+          expect { subject }.to raise_error(described_class::FailedToPersistDataError)
         end
       end
     end
@@ -504,27 +534,37 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
       let(:data_store) { :fog }
 
       context 'when data exists' do
-        let(:data) { 'Sample data in fog' }
-
         before do
           build_trace_chunk.send(:unsafe_set_data!, data)
         end
 
-        it 'does not change data store' do
-          expect(build_trace_chunk.fog?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+        context 'when data size reached CHUNK_SIZE' do
+          let(:data) { 'a' * described_class::CHUNK_SIZE }
 
-          subject
+          it 'does not change data store' do
+            expect(build_trace_chunk.fog?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
 
-          expect(build_trace_chunk.fog?).to be_truthy
-          expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
-          expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+            subject
+
+            expect(build_trace_chunk.fog?).to be_truthy
+            expect(Ci::BuildTraceChunks::Redis.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Database.new.data(build_trace_chunk)).to be_nil
+            expect(Ci::BuildTraceChunks::Fog.new.data(build_trace_chunk)).to eq(data)
+          end
+
+          it_behaves_like 'Atomic operation'
         end
 
-        it_behaves_like 'Atomic operation'
+        context 'when data size has not reached CHUNK_SIZE' do
+          let(:data) { 'Sample data in fog' }
+
+          it 'does not raise error' do
+            expect { subject }.not_to raise_error
+          end
+        end
       end
     end
   end