From 1a7d9346682af5c496d0a974180eb4054ccae8af Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Fri, 30 Mar 2018 16:50:23 +0900
Subject: [PATCH] Fix wrong error handling in update page service

---
 app/services/projects/update_pages_service.rb | 16 +++++----
 .../projects/update_pages_service_spec.rb     | 35 +++++++++++++++++--
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb
index 5bf8208e035..9f7243b9ae1 100644
--- a/app/services/projects/update_pages_service.rb
+++ b/app/services/projects/update_pages_service.rb
@@ -37,9 +37,10 @@ module Projects
         deploy_page!(archive_public_path)
         success
       end
-    rescue InvaildStateError, FailedToExtractError => e
-      register_failure
+    rescue InvaildStateError => e
       error(e.message)
+    rescue => e
+      error(e.message, false)
     end
 
     private
@@ -50,12 +51,13 @@ module Projects
       super
     end
 
-    def error(message, http_status = nil)
+    def error(message, delete_artifact = true)
+      register_failure
       log_error("Projects::UpdatePagesService: #{message}")
       @status.allow_failure = !latest?
       @status.description = message
       @status.drop(:script_failure)
-      delete_artifact!
+      delete_artifact! if delete_artifact
       super
     end
 
@@ -76,7 +78,7 @@ module Projects
       elsif artifacts.ends_with?('.zip')
         extract_zip_archive!(temp_path)
       else
-        raise FailedToExtractError, 'unsupported artifacts format'
+        raise InvaildStateError, 'unsupported artifacts format'
       end
     end
 
@@ -91,13 +93,13 @@ module Projects
     end
 
     def extract_zip_archive!(temp_path)
-      raise FailedToExtractError, 'missing artifacts metadata' unless build.artifacts_metadata?
+      raise InvaildStateError, 'missing artifacts metadata' unless build.artifacts_metadata?
 
       # Calculate page size after extract
       public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
 
       if public_entry.total_size > max_size
-        raise FailedToExtractError, "artifacts for pages are too large: #{public_entry.total_size}"
+        raise InvaildStateError, "artifacts for pages are too large: #{public_entry.total_size}"
       end
 
       # Requires UnZip at least 6.00 Info-ZIP.
diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb
index 934106627a9..0c8c05168ce 100644
--- a/spec/services/projects/update_pages_service_spec.rb
+++ b/spec/services/projects/update_pages_service_spec.rb
@@ -169,10 +169,41 @@ describe Projects::UpdatePagesService do
         end
 
         it 'raises an error' do
-          expect { execute }.to raise_error(SocketError)
+          execute
 
           build.reload
-          expect(build.artifacts?).to eq(true)
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_truthy
+        end
+      end
+
+      context 'when failed to extract zip artifacts' do
+        before do
+          allow_any_instance_of(described_class)
+            .to receive(:extract_zip_archive!)
+            .and_raise(Projects::UpdatePagesService::FailedToExtractError)
+        end
+
+        it 'raises an error' do
+          execute
+
+          build.reload
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_truthy
+        end
+      end
+
+      context 'when missing artifacts metadata' do
+        before do
+          allow(build).to receive(:artifacts_metadata?).and_return(false)
+        end
+
+        it 'raises an error' do
+          execute
+
+          build.reload
+          expect(deploy_status).to be_failed
+          expect(build.artifacts?).to be_falsey
         end
       end
     end