Merge "Fix error status code on update-volume API" into stable/mitaka

3 files changed, 34 insertions, 5 deletions

diff --git a/nova/api/openstack/compute/legacy_v2/contrib/volumes.py b/nova/api/openstack/compute/legacy_v2/contrib/volumes.py
index 54a1847542..70bed0b4bd 100644
--- a/nova/api/openstack/compute/legacy_v2/contrib/volumes.py
+++ b/nova/api/openstack/compute/legacy_v2/contrib/volumes.py
@@ -367,8 +367,19 @@ class VolumeAttachmentController(wsgi.Controller):
         except KeyError:
             msg = _("volumeId must be specified.")
             raise exc.HTTPBadRequest(explanation=msg)
+
         self._validate_volume_id(new_volume_id)
-        new_volume = self.volume_api.get(context, new_volume_id)
+        try:
+            new_volume = self.volume_api.get(context, new_volume_id)
+        except exception.VolumeNotFound as e:
+            # NOTE: This BadRequest is different from the above NotFound even
+            # though the same VolumeNotFound exception. This is intentional
+            # because new_volume_id is specified in a request body and if a
+            # nonexistent resource in the body (not URI) the code should be
+            # 400 Bad Request as API-WG guideline. On the other hand,
+            # old_volume_id is specified with URI. So it is valid to return
+            # NotFound response if that is not existent.
+            raise exc.HTTPBadRequest(explanation=e.format_message())
 
         instance = common.get_instance(self.compute_api, context, server_id)
 
diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py
index 2c07c59094..b585bd6b89 100644
--- a/nova/api/openstack/compute/volumes.py
+++ b/nova/api/openstack/compute/volumes.py
@@ -358,11 +358,21 @@ class VolumeAttachmentController(wsgi.Controller):
         old_volume_id = id
         try:
             old_volume = self.volume_api.get(context, old_volume_id)
+        except exception.VolumeNotFound as e:
+            raise exc.HTTPNotFound(explanation=e.format_message())
 
-            new_volume_id = body['volumeAttachment']['volumeId']
+        new_volume_id = body['volumeAttachment']['volumeId']
+        try:
             new_volume = self.volume_api.get(context, new_volume_id)
         except exception.VolumeNotFound as e:
-            raise exc.HTTPNotFound(explanation=e.format_message())
+            # NOTE: This BadRequest is different from the above NotFound even
+            # though the same VolumeNotFound exception. This is intentional
+            # because new_volume_id is specified in a request body and if a
+            # nonexistent resource in the body (not URI) the code should be
+            # 400 Bad Request as API-WG guideline. On the other hand,
+            # old_volume_id is specified with URI. So it is valid to return
+            # NotFound response if that is not existent.
+            raise exc.HTTPBadRequest(explanation=e.format_message())
 
         instance = common.get_instance(self.compute_api, context, server_id)
 
diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py
index 857da39071..2468a58e6c 100644
--- a/nova/tests/unit/api/openstack/compute/test_volumes.py
+++ b/nova/tests/unit/api/openstack/compute/test_volumes.py
@@ -721,9 +721,17 @@ class VolumeAttachTestsV21(test.NoDBTestCase):
             status_int = result.status_int
         self.assertEqual(202, status_int)
 
-    def test_swap_volume_no_attachment(self):
+    def test_swap_volume_with_nonexistent_uri(self):
         self.assertRaises(exc.HTTPNotFound, self._test_swap,
-                          self.attachments, FAKE_UUID_C)
+                          self.attachments, uuid=FAKE_UUID_C)
+
+    @mock.patch.object(cinder.API, 'get')
+    def test_swap_volume_with_nonexistent_dest_in_body(self, mock_update):
+        mock_update.side_effect = [
+            None, exception.VolumeNotFound(volume_id=FAKE_UUID_C)]
+        body = {'volumeAttachment': {'volumeId': FAKE_UUID_C}}
+        self.assertRaises(exc.HTTPBadRequest, self._test_swap,
+                          self.attachments, body=body)
 
     def test_swap_volume_without_volumeId(self):
         body = {'volumeAttachment': {'device': '/dev/fake'}}