From 12548c9cea3c81ddd68e75c9433e8d6c735ce9b9 Mon Sep 17 00:00:00 2001
From: nidhimittalhada <nidhimittal19@gmail.com>
Date: Thu, 24 Dec 2015 17:31:28 +0530
Subject: [PATCH] Hide snapshots with no instances from listing

When a snapshot is being destroyed; for a small window of time
between the deletion of all of its instances and the snapshot ref
itself, the 'status' of the snapshot becomes 'None'. This provides
a bad user experience if a list/show query is made within that
window.

Fix this problem by filtering out snapshots that have this 'invalid'
status.

Corrected a few unit tests, to use 'status' as a manadatory
field in snapshot record.

Change-Id: I2ec83eecc9788278459846502625df7fde1fccbd
Closes-Bug: #1503390
---
 manila/api/v1/share_snapshots.py            |  9 +++
 manila/tests/api/v1/test_share_snapshots.py | 63 +++++++++++++++++++--
 2 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/manila/api/v1/share_snapshots.py b/manila/api/v1/share_snapshots.py
index a071282c2b..3433c780d8 100644
--- a/manila/api/v1/share_snapshots.py
+++ b/manila/api/v1/share_snapshots.py
@@ -76,6 +76,10 @@ class ShareSnapshotsController(wsgi.Controller, wsgi.AdminActionsMixin):
 
         try:
             snapshot = self.share_api.get_snapshot(context, id)
+
+            # Snapshot with no instances is filtered out.
+            if(snapshot.get('status') is None):
+                raise exc.HTTPNotFound()
         except exception.NotFound:
             raise exc.HTTPNotFound()
 
@@ -130,6 +134,11 @@ class ShareSnapshotsController(wsgi.Controller, wsgi.AdminActionsMixin):
             sort_key=sort_key,
             sort_dir=sort_dir,
         )
+
+        # Snapshots with no instances are filtered out.
+        snapshots = list(filter(lambda x: x.get('status') is not None,
+                                snapshots))
+
         limited_list = common.limited(snapshots, req)
         if is_detail:
             snapshots = self._view_builder.detail_list(req, limited_list)
diff --git a/manila/tests/api/v1/test_share_snapshots.py b/manila/tests/api/v1/test_share_snapshots.py
index 5cdd0d0d62..4fac14416d 100644
--- a/manila/tests/api/v1/test_share_snapshots.py
+++ b/manila/tests/api/v1/test_share_snapshots.py
@@ -57,6 +57,20 @@ class ShareSnapshotAPITest(test.TestCase):
         }
         self.maxDiff = None
 
+    def test_snapshot_show_status_none(self):
+        return_snapshot = {
+            'share_id': 100,
+            'name': 'fake_share_name',
+            'description': 'fake_share_description',
+            'status': None,
+        }
+        self.mock_object(share_api.API, 'get_snapshot',
+                         mock.Mock(return_value=return_snapshot))
+        req = fakes.HTTPRequest.blank('/snapshots/200')
+        self.assertRaises(webob.exc.HTTPNotFound,
+                          self.controller.show,
+                          req, '200')
+
     @ddt.data('true', 'True', '<is> True', '1')
     def test_snapshot_create(self, snapshot_support):
         self.mock_object(share_api.API, 'create_snapshot',
@@ -224,9 +238,12 @@ class ShareSnapshotAPITest(test.TestCase):
         req = fakes.HTTPRequest.blank(url, use_admin_context=use_admin_context)
 
         snapshots = [
-            {'id': 'id1', 'display_name': 'n1'},
-            {'id': 'id2', 'display_name': 'n2'},
-            {'id': 'id3', 'display_name': 'n3'},
+            {'id': 'id1', 'display_name': 'n1',
+             'status': 'fake_status', 'share_id': 'fake_share_id'},
+            {'id': 'id2', 'display_name': 'n2',
+             'status': 'fake_status', 'share_id': 'fake_share_id'},
+            {'id': 'id3', 'display_name': 'n3',
+             'status': 'fake_status', 'share_id': 'fake_share_id'},
         ]
         self.mock_object(share_api.API, 'get_all_snapshots',
                          mock.Mock(return_value=snapshots))
@@ -274,14 +291,24 @@ class ShareSnapshotAPITest(test.TestCase):
         req = fakes.HTTPRequest.blank(url, use_admin_context=use_admin_context)
 
         snapshots = [
-            {'id': 'id1', 'display_name': 'n1'},
+            {
+                'id': 'id1',
+                'display_name': 'n1',
+                'status': 'fake_status',
+                'share_id': 'fake_share_id',
+            },
             {
                 'id': 'id2',
                 'display_name': 'n2',
                 'status': 'fake_status',
                 'share_id': 'fake_share_id',
             },
-            {'id': 'id3', 'display_name': 'n3'},
+            {
+                'id': 'id3',
+                'display_name': 'n3',
+                'status': 'fake_status',
+                'share_id': 'fake_share_id',
+            },
         ]
 
         self.mock_object(share_api.API, 'get_all_snapshots',
@@ -349,6 +376,32 @@ class ShareSnapshotAPITest(test.TestCase):
         }
         self.assertEqual(expected, res_dict)
 
+    def test_snapshot_list_status_none(self):
+        snapshots = [
+            {
+                'id': 2,
+                'share_id': 'fakeshareid',
+                'size': 1,
+                'status': 'fakesnapstatus',
+                'name': 'displaysnapname',
+                'description': 'displaysnapdesc',
+            },
+            {
+                'id': 3,
+                'share_id': 'fakeshareid',
+                'size': 1,
+                'status': None,
+                'name': 'displaysnapname',
+                'description': 'displaysnapdesc',
+            }
+        ]
+        self.mock_object(share_api.API, 'get_all_snapshots',
+                         mock.Mock(return_value=snapshots))
+        req = fakes.HTTPRequest.blank('/snapshots')
+        result = self.controller.index(req)
+        self.assertEqual(1, len(result['snapshots']))
+        self.assertEqual(snapshots[0]['id'], result['snapshots'][0]['id'])
+
     def test_snapshot_updates_description(self):
         snp = self.snp_example
         body = {"snapshot": snp}