From bc5ddf1e6a1e396c3d8a29b42369f48b0b5f2302 Mon Sep 17 00:00:00 2001 From: Rui Chen Date: Wed, 29 Jun 2016 11:24:41 +0800 Subject: [PATCH] Fix batch deleting issue in volume_type.unset_keys() cinderclient volume_type.unset_keys() only delete the first key even if multiple keys are applied. The response of manager._delete() is tuple object, it is not "None" if the deleting success, so the batch operation would be broken when the first key is deleted successfully. The patch fix this issue. Change-Id: I60378a32cdc52aacdf869d69b246dec7eb6cdb77 Closes-Bug: #1596511 --- cinderclient/tests/unit/utils.py | 4 ++-- cinderclient/tests/unit/v1/fakes.py | 3 +++ cinderclient/tests/unit/v1/test_types.py | 8 +++++++- cinderclient/tests/unit/v2/fakes.py | 3 +++ cinderclient/tests/unit/v2/test_types.py | 17 ++++++++++++----- cinderclient/v1/volume_types.py | 14 +++++--------- cinderclient/v3/volume_types.py | 15 +++++++++------ 7 files changed, 41 insertions(+), 23 deletions(-) diff --git a/cinderclient/tests/unit/utils.py b/cinderclient/tests/unit/utils.py index f9c59fa08..db697a075 100644 --- a/cinderclient/tests/unit/utils.py +++ b/cinderclient/tests/unit/utils.py @@ -40,9 +40,9 @@ class TestCase(testtools.TestCase): stderr = self.useFixture(fixtures.StringStream('stderr')).stream self.useFixture(fixtures.MonkeyPatch('sys.stderr', stderr)) - def _assert_request_id(self, obj): + def _assert_request_id(self, obj, count=1): self.assertTrue(hasattr(obj, 'request_ids')) - self.assertEqual(REQUEST_ID, obj.request_ids) + self.assertEqual(REQUEST_ID * count, obj.request_ids) def assert_called_anytime(self, method, url, body=None, partial_body=None): diff --git a/cinderclient/tests/unit/v1/fakes.py b/cinderclient/tests/unit/v1/fakes.py index 77a4cfca4..2f7cb6465 100644 --- a/cinderclient/tests/unit/v1/fakes.py +++ b/cinderclient/tests/unit/v1/fakes.py @@ -472,6 +472,9 @@ class FakeHTTPClient(base_client.HTTPClient): def delete_types_1_extra_specs_k(self, **kw): return(204, {}, None) + def delete_types_1_extra_specs_m(self, **kw): + return(204, {}, None) + def delete_types_1(self, **kw): return (202, {}, None) diff --git a/cinderclient/tests/unit/v1/test_types.py b/cinderclient/tests/unit/v1/test_types.py index 824140015..4c799ece1 100644 --- a/cinderclient/tests/unit/v1/test_types.py +++ b/cinderclient/tests/unit/v1/test_types.py @@ -37,11 +37,17 @@ class TypesTest(utils.TestCase): '/types/1/extra_specs', {'extra_specs': {'k': 'v'}}) - def test_unsset_keys(self): + def test_unset_keys(self): t = cs.volume_types.get(1) t.unset_keys(['k']) cs.assert_called('DELETE', '/types/1/extra_specs/k') + def test_unset_multiple_keys(self): + t = cs.volume_types.get(1) + t.unset_keys(['k', 'm']) + cs.assert_called_anytime('DELETE', '/types/1/extra_specs/k') + cs.assert_called_anytime('DELETE', '/types/1/extra_specs/m') + def test_delete(self): cs.volume_types.delete(1) cs.assert_called('DELETE', '/types/1') diff --git a/cinderclient/tests/unit/v2/fakes.py b/cinderclient/tests/unit/v2/fakes.py index 199482653..294070d56 100644 --- a/cinderclient/tests/unit/v2/fakes.py +++ b/cinderclient/tests/unit/v2/fakes.py @@ -689,6 +689,9 @@ class FakeHTTPClient(base_client.HTTPClient): def delete_types_1_extra_specs_k(self, **kw): return(204, {}, None) + def delete_types_1_extra_specs_m(self, **kw): + return(204, {}, None) + def delete_types_1(self, **kw): return (202, {}, None) diff --git a/cinderclient/tests/unit/v2/test_types.py b/cinderclient/tests/unit/v2/test_types.py index b12611174..0a6fb8c45 100644 --- a/cinderclient/tests/unit/v2/test_types.py +++ b/cinderclient/tests/unit/v2/test_types.py @@ -81,17 +81,24 @@ class TypesTest(utils.TestCase): def test_set_key(self): t = cs.volume_types.get(1) - t.set_keys({'k': 'v'}) + res = t.set_keys({'k': 'v'}) cs.assert_called('POST', '/types/1/extra_specs', {'extra_specs': {'k': 'v'}}) - self._assert_request_id(t) + self._assert_request_id(res) - def test_unsset_keys(self): + def test_unset_keys(self): t = cs.volume_types.get(1) - t.unset_keys(['k']) + res = t.unset_keys(['k']) cs.assert_called('DELETE', '/types/1/extra_specs/k') - self._assert_request_id(t) + self._assert_request_id(res) + + def test_unset_multiple_keys(self): + t = cs.volume_types.get(1) + res = t.unset_keys(['k', 'm']) + cs.assert_called_anytime('DELETE', '/types/1/extra_specs/k') + cs.assert_called_anytime('DELETE', '/types/1/extra_specs/m') + self._assert_request_id(res, count=2) def test_delete(self): t = cs.volume_types.delete(1) diff --git a/cinderclient/v1/volume_types.py b/cinderclient/v1/volume_types.py index 7e1a779f6..6e5b0af1f 100644 --- a/cinderclient/v1/volume_types.py +++ b/cinderclient/v1/volume_types.py @@ -62,16 +62,12 @@ class VolumeType(base.Resource): """ # NOTE(jdg): This wasn't actually doing all of the keys before - # the return in the loop resulted in ony ONE key being unset. - # since on success the return was NONE, we'll only interrupt the loop - # and return if there's an error - resp = None + # the return in the loop resulted in only ONE key being unset, + # since on success the return was None, we'll only interrupt + # the loop and if an exception is raised. for k in keys: - resp = self.manager._delete( - "/types/%s/extra_specs/%s" % ( - base.getid(self), k)) - if resp is not None: - return resp + self.manager._delete("/types/%s/extra_specs/%s" % + (base.getid(self), k)) class VolumeTypeManager(base.ManagerWithFind): diff --git a/cinderclient/v3/volume_types.py b/cinderclient/v3/volume_types.py index 251e75b9f..0581d6c9a 100644 --- a/cinderclient/v3/volume_types.py +++ b/cinderclient/v3/volume_types.py @@ -17,6 +17,7 @@ """Volume Type interface.""" from cinderclient import base +from cinderclient.openstack.common.apiclient import base as common_base class VolumeType(base.Resource): @@ -63,15 +64,17 @@ class VolumeType(base.Resource): """ # NOTE(jdg): This wasn't actually doing all of the keys before - # the return in the loop resulted in ony ONE key being unset. - # since on success the return was NONE, we'll only interrupt the loop - # and return if there's an error + # the return in the loop resulted in only ONE key being unset, + # since on success the return was ListWithMeta class, we'll only + # interrupt the loop and if an exception is raised. + response_list = [] for k in keys: - resp = self.manager._delete( + resp, body = self.manager._delete( "/types/%s/extra_specs/%s" % ( base.getid(self), k)) - if resp is not None: - return resp + response_list.append(resp) + + return common_base.ListWithMeta([], response_list) class VolumeTypeManager(base.ManagerWithFind):