From 381e717b4cdebb75f90dc4c62120e8d607f2a98c Mon Sep 17 00:00:00 2001
From: huangtianhua <huangtianhua@huawei.com>
Date: Mon, 18 Nov 2013 16:58:45 +0800
Subject: [PATCH] Create snapshot throws 500 Internal Error

The server doesn't check whether the parameter "volume_id" is in request body.
So the 500 error has been thrown.

We should catch the KeyError and transfer the KeyError to 400(HTTPBadRequest)
instead of 500.

Change-Id: I8a1dde1fd6ed820b39995af434efacc2a27c9604
Closes-Bug: #1252179
---
 cinder/api/v1/snapshots.py            |  7 ++++++-
 cinder/api/v2/snapshots.py            |  7 ++++++-
 cinder/tests/api/v1/test_snapshots.py | 14 ++++++++++++++
 cinder/tests/api/v2/test_snapshots.py | 14 ++++++++++++++
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/cinder/api/v1/snapshots.py b/cinder/api/v1/snapshots.py
index d2922dc5abe..08d87afbf6e 100644
--- a/cinder/api/v1/snapshots.py
+++ b/cinder/api/v1/snapshots.py
@@ -167,7 +167,12 @@ def create(self, req, body):
         snapshot = body['snapshot']
         kwargs['metadata'] = snapshot.get('metadata', None)
 
-        volume_id = snapshot['volume_id']
+        try:
+            volume_id = snapshot['volume_id']
+        except KeyError:
+            msg = _("'volume_id' must be specified")
+            raise exc.HTTPBadRequest(explanation=msg)
+
         volume = self.volume_api.get(context, volume_id)
         force = snapshot.get('force', False)
         msg = _("Create snapshot from volume %s")
diff --git a/cinder/api/v2/snapshots.py b/cinder/api/v2/snapshots.py
index f6ac6da76a9..24703317a8c 100644
--- a/cinder/api/v2/snapshots.py
+++ b/cinder/api/v2/snapshots.py
@@ -178,7 +178,12 @@ def create(self, req, body):
         snapshot = body['snapshot']
         kwargs['metadata'] = snapshot.get('metadata', None)
 
-        volume_id = snapshot['volume_id']
+        try:
+            volume_id = snapshot['volume_id']
+        except KeyError:
+            msg = _("'volume_id' must be specified")
+            raise exc.HTTPBadRequest(explanation=msg)
+
         volume = self.volume_api.get(context, volume_id)
         force = snapshot.get('force', False)
         msg = _("Create snapshot from volume %s")
diff --git a/cinder/tests/api/v1/test_snapshots.py b/cinder/tests/api/v1/test_snapshots.py
index 831a0db1ba4..c5b0b8c2871 100644
--- a/cinder/tests/api/v1/test_snapshots.py
+++ b/cinder/tests/api/v1/test_snapshots.py
@@ -130,6 +130,20 @@ def test_snapshot_create_force(self):
                           req,
                           body)
 
+    def test_snapshot_create_without_volume_id(self):
+        snapshot_name = 'Snapshot Test Name'
+        snapshot_description = 'Snapshot Test Desc'
+        body = {
+            "snapshot": {
+                "force": True,
+                "name": snapshot_name,
+                "description": snapshot_description
+            }
+        }
+        req = fakes.HTTPRequest.blank('/v1/snapshots')
+        self.assertRaises(webob.exc.HTTPBadRequest,
+                          self.controller.create, req, body)
+
     def test_snapshot_update(self):
         self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get)
         self.stubs.Set(volume.api.API, "update_snapshot",
diff --git a/cinder/tests/api/v2/test_snapshots.py b/cinder/tests/api/v2/test_snapshots.py
index 0264a313507..43fa3a4e197 100644
--- a/cinder/tests/api/v2/test_snapshots.py
+++ b/cinder/tests/api/v2/test_snapshots.py
@@ -142,6 +142,20 @@ def test_snapshot_create_force(self):
                           req,
                           body)
 
+    def test_snapshot_create_without_volume_id(self):
+        snapshot_name = 'Snapshot Test Name'
+        snapshot_description = 'Snapshot Test Desc'
+        body = {
+            "snapshot": {
+                "force": True,
+                "name": snapshot_name,
+                "description": snapshot_description
+            }
+        }
+        req = fakes.HTTPRequest.blank('/v2/snapshots')
+        self.assertRaises(webob.exc.HTTPBadRequest,
+                          self.controller.create, req, body)
+
     def test_snapshot_update(self):
         self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get)
         self.stubs.Set(volume.api.API, "update_snapshot",