From 3ec4914056ad4149a855a3f51f5f4a8df3a754b0 Mon Sep 17 00:00:00 2001
From: Jillian Tullo <jtullo@redhat.com>
Date: Fri, 30 Jun 2017 14:50:05 -0400
Subject: [PATCH] move rescue block to raise NotFound error during resource
 search

---
 .../api/subcollections/snapshots.rb           | 16 ++---
 spec/requests/api/snapshots_spec.rb           | 59 ++++++++++++++++++-
 2 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/app/controllers/api/subcollections/snapshots.rb b/app/controllers/api/subcollections/snapshots.rb
index 8fde24994bc..91a406b72ab 100644
--- a/app/controllers/api/subcollections/snapshots.rb
+++ b/app/controllers/api/subcollections/snapshots.rb
@@ -23,14 +23,16 @@ def snapshots_create_resource(parent, _type, _id, data)
       end
 
       def delete_resource_snapshots(parent, type, id, _data)
-        raise parent.unsupported_reason(:remove_snapshot) unless parent.supports_remove_snapshot?
         snapshot = resource_search(id, type, collection_class(type))
-
-        message = "Deleting snapshot #{snapshot.name} for #{snapshot_ident(parent)}"
-        task_id = queue_object_action(parent, message, :method_name => "remove_snapshot", :args => [id])
-        action_result(true, message, :task_id => task_id)
-      rescue => e
-        action_result(false, e.to_s)
+        begin
+          raise parent.unsupported_reason(:remove_snapshot) unless parent.supports_remove_snapshot?
+
+          message = "Deleting snapshot #{snapshot.name} for #{snapshot_ident(parent)}"
+          task_id = queue_object_action(parent, message, :method_name => "remove_snapshot", :args => [id])
+          action_result(true, message, :task_id => task_id)
+        rescue => e
+          action_result(false, e.to_s)
+        end
       end
       alias snapshots_delete_resource delete_resource_snapshots
 
diff --git a/spec/requests/api/snapshots_spec.rb b/spec/requests/api/snapshots_spec.rb
index 1854baa0bd9..d253824b23a 100644
--- a/spec/requests/api/snapshots_spec.rb
+++ b/spec/requests/api/snapshots_spec.rb
@@ -223,11 +223,28 @@
 
         expect(response).to have_http_status(:forbidden)
       end
+
+      it "raises a 404 with proper message if the resource isn't found" do
+        api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subresource_actions, :post))
+        vm = FactoryGirl.create(:vm_vmware)
+
+        run_post("#{vms_url(vm.id)}/snapshots/0", :action => "delete")
+
+        expected = {
+          "error" => a_hash_including(
+            "kind"    => "not_found",
+            "message" => "Couldn't find Snapshot with 'id'=0",
+            "klass"   => "ActiveRecord::RecordNotFound"
+          )
+        }
+        expect(response.parsed_body).to include(expected)
+        expect(response).to have_http_status(:not_found)
+      end
     end
 
     describe "POST /api/vms/:c_id/snapshots with delete action" do
       it "can queue multiple snapshots for deletion" do
-        api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subresource_actions, :delete))
+        api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subcollection_actions, :post))
         ems = FactoryGirl.create(:ext_management_system)
         host = FactoryGirl.create(:host, :ext_management_system => ems)
         vm = FactoryGirl.create(:vm_vmware, :name => "Alice and Bob's VM", :host => host, :ext_management_system => ems)
@@ -262,6 +279,29 @@
         expect(response.parsed_body).to include(expected)
         expect(response).to have_http_status(:ok)
       end
+
+      it "raises a 404 with proper message if a resource isn't found" do
+        api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subcollection_actions, :post))
+        vm = FactoryGirl.create(:vm_vmware)
+
+        run_post(
+          "#{vms_url(vm.id)}/snapshots",
+          :action    => "delete",
+          :resources => [
+            {:href => "#{vms_url(vm.id)}/snapshots/0"}
+          ]
+        )
+
+        expected = {
+          "error" => a_hash_including(
+            "kind"    => "not_found",
+            "message" => "Couldn't find Snapshot with 'id'=0",
+            "klass"   => "ActiveRecord::RecordNotFound"
+          )
+        }
+        expect(response.parsed_body).to include(expected)
+        expect(response).to have_http_status(:not_found)
+      end
     end
 
     describe "DELETE /api/vms/:c_id/snapshots/:s_id" do
@@ -284,6 +324,23 @@
 
         expect(response).to have_http_status(:forbidden)
       end
+
+      it "raises a 404 with proper message if the resource isn't found" do
+        api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subresource_actions, :delete))
+        vm = FactoryGirl.create(:vm_vmware)
+
+        run_delete("#{vms_url(vm.id)}/snapshots/0", :action => "delete")
+
+        expected = {
+          "error" => a_hash_including(
+            "kind"    => "not_found",
+            "message" => "Couldn't find Snapshot with 'id'=0",
+            "klass"   => "ActiveRecord::RecordNotFound"
+          )
+        }
+        expect(response.parsed_body).to include(expected)
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end