Merge pull request #15489 from jntullo/bz/inconsistent_response_delet…

…e_snapshot

Return Not Found on Snapshots Delete actions

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
 

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