diff options
| -rw-r--r-- | cinderclient/apiclient/base.py | 6 | ||||
| -rw-r--r-- | cinderclient/base.py | 7 | ||||
| -rw-r--r-- | cinderclient/tests/unit/test_utils.py | 25 | ||||
| -rw-r--r-- | cinderclient/tests/unit/v1/test_shell.py | 17 | ||||
| -rw-r--r-- | cinderclient/tests/unit/v2/test_shell.py | 2 | ||||
| -rw-r--r-- | cinderclient/tests/unit/v3/test_shell.py | 29 | ||||
| -rw-r--r-- | cinderclient/utils.py | 18 | ||||
| -rw-r--r-- | cinderclient/v1/volume_snapshots.py | 2 | ||||
| -rw-r--r-- | cinderclient/v1/volumes.py | 2 | ||||
| -rw-r--r-- | cinderclient/v2/limits.py | 5 | ||||
| -rw-r--r-- | cinderclient/v3/group_snapshots.py | 2 | ||||
| -rw-r--r-- | cinderclient/v3/groups.py | 10 |
12 files changed, 65 insertions, 60 deletions
diff --git a/cinderclient/apiclient/base.py b/cinderclient/apiclient/base.py index 9011d8f..f1febe3 100644 --- a/cinderclient/apiclient/base.py +++ b/cinderclient/apiclient/base.py @@ -28,9 +28,9 @@ import copy from requests import Response import six -from six.moves.urllib import parse from cinderclient.apiclient import exceptions +from cinderclient import utils from oslo_utils import encodeutils from oslo_utils import strutils @@ -325,7 +325,7 @@ class CrudManager(BaseManager): return self._list( '%(base_url)s%(query)s' % { 'base_url': self.build_url(base_url=base_url, **kwargs), - 'query': '?%s' % parse.urlencode(kwargs) if kwargs else '', + 'query': utils.build_query_param(kwargs), }, self.collection_key) @@ -364,7 +364,7 @@ class CrudManager(BaseManager): rl = self._list( '%(base_url)s%(query)s' % { 'base_url': self.build_url(base_url=base_url, **kwargs), - 'query': '?%s' % parse.urlencode(kwargs) if kwargs else '', + 'query': '?%s' % utils.build_query_param(kwargs), }, self.collection_key) num = len(rl) diff --git a/cinderclient/base.py b/cinderclient/base.py index 44d1149..85dffb6 100644 --- a/cinderclient/base.py +++ b/cinderclient/base.py @@ -24,7 +24,6 @@ import hashlib import os import six -from six.moves.urllib import parse from cinderclient.apiclient import base as common_base from cinderclient import exceptions @@ -170,10 +169,8 @@ class Manager(common_base.HookableMixin): query_params = utils.unicode_key_value_to_string(query_params) # Transform the dict to a sequence of two-element tuples in fixed # order, then the encoded string will be consistent in Python 2&3. - query_string = "" - if query_params: - params = sorted(query_params.items(), key=lambda x: x[0]) - query_string = "?%s" % parse.urlencode(params) + + query_string = utils.build_query_param(query_params, sort=True) detail = "" if detailed: diff --git a/cinderclient/tests/unit/test_utils.py b/cinderclient/tests/unit/test_utils.py index 2ed4299..0cb12a6 100644 --- a/cinderclient/tests/unit/test_utils.py +++ b/cinderclient/tests/unit/test_utils.py @@ -162,6 +162,7 @@ class CaptureStdout(object): self.read = self.stringio.read +@ddt.ddt class BuildQueryParamTestCase(test_utils.TestCase): def test_build_param_without_sort_switch(self): @@ -187,19 +188,17 @@ class BuildQueryParamTestCase(test_utils.TestCase): expected = "?key1=val1&key2=val2&key3=val3" self.assertEqual(expected, result) - def test_build_param_with_none(self): - dict_param = { - 'key1': 'val1', - 'key2': None, - 'key3': False, - 'key4': '' - } - result_1 = utils.build_query_param(dict_param) - result_2 = utils.build_query_param(None) - - expected = "?key1=val1" - self.assertEqual(expected, result_1) - self.assertFalse(result_2) + @ddt.data({}, + None, + {'key1': 'val1', 'key2': None, 'key3': False, 'key4': ''}) + def test_build_param_with_nones(self, dict_param): + result = utils.build_query_param(dict_param) + + expected = ("key1=val1", "key3=False") if dict_param else () + for exp in expected: + self.assertIn(exp, result) + if not expected: + self.assertEqual("", result) @ddt.ddt diff --git a/cinderclient/tests/unit/v1/test_shell.py b/cinderclient/tests/unit/v1/test_shell.py index 3f4d771..698542c 100644 --- a/cinderclient/tests/unit/v1/test_shell.py +++ b/cinderclient/tests/unit/v1/test_shell.py @@ -101,7 +101,7 @@ class ShellTest(utils.TestCase): def test_list(self): self.run_command('list') # NOTE(jdg): we default to detail currently - self.assert_called('GET', '/volumes/detail') + self.assert_called('GET', '/volumes/detail?all_tenants=0') def test_list_filter_tenant_with_all_tenants(self): self.run_command('list --tenant=123 --all-tenants 1') @@ -184,11 +184,13 @@ class ShellTest(utils.TestCase): def test_list_filter_status(self): self.run_command('list --status=available') - self.assert_called('GET', '/volumes/detail?status=available') + self.assert_called('GET', + '/volumes/detail?all_tenants=0&status=available') def test_list_filter_display_name(self): self.run_command('list --display-name=1234') - self.assert_called('GET', '/volumes/detail?display_name=1234') + self.assert_called('GET', + '/volumes/detail?all_tenants=0&display_name=1234') def test_list_all_tenants(self): self.run_command('list --all-tenants=1') @@ -200,7 +202,7 @@ class ShellTest(utils.TestCase): def test_list_limit(self): self.run_command('list --limit=10') - self.assert_called('GET', '/volumes/detail?limit=10') + self.assert_called('GET', '/volumes/detail?all_tenants=0&limit=10') def test_show(self): self.run_command('show 1234') @@ -231,12 +233,13 @@ class ShellTest(utils.TestCase): def test_snapshot_list_filter_volume_id(self): self.run_command('snapshot-list --volume-id=1234') - self.assert_called('GET', '/snapshots/detail?volume_id=1234') + self.assert_called('GET', + '/snapshots/detail?all_tenants=0&volume_id=1234') def test_snapshot_list_filter_status_and_volume_id(self): self.run_command('snapshot-list --status=available --volume-id=1234') self.assert_called('GET', '/snapshots/detail?' - 'status=available&volume_id=1234') + 'all_tenants=0&status=available&volume_id=1234') def test_rename(self): # basic rename with positional arguments @@ -483,7 +486,7 @@ class ShellTest(utils.TestCase): def test_list_transfer(self): self.run_command('transfer-list') - self.assert_called('GET', '/os-volume-transfer/detail') + self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0') def test_list_transfer_all_tenants(self): self.run_command('transfer-list --all-tenants=1') diff --git a/cinderclient/tests/unit/v2/test_shell.py b/cinderclient/tests/unit/v2/test_shell.py index fd301a6..6284122 100644 --- a/cinderclient/tests/unit/v2/test_shell.py +++ b/cinderclient/tests/unit/v2/test_shell.py @@ -1176,7 +1176,7 @@ class ShellTest(utils.TestCase): def test_list_transfer(self): self.run_command('transfer-list') - self.assert_called('GET', '/os-volume-transfer/detail') + self.assert_called('GET', '/os-volume-transfer/detail?all_tenants=0') def test_list_transfer_all_tenants(self): self.run_command('transfer-list --all-tenants=1') diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py index 6661080..a5ed614 100644 --- a/cinderclient/tests/unit/v3/test_shell.py +++ b/cinderclient/tests/unit/v3/test_shell.py @@ -131,37 +131,37 @@ class ShellTest(utils.TestCase): {'command': 'list --filters name~=456', 'expected': - '/volumes/detail?name%7E=456'}, + '/volumes/detail?name~=456'}, {'command': u'list --filters name~=Σ', 'expected': - '/volumes/detail?name%7E=%CE%A3'}, + '/volumes/detail?name~=%CE%A3'}, # testcases for list group {'command': 'group-list --filters name=456', 'expected': - '/groups/detail?name=456'}, + '/groups/detail?all_tenants=0&name=456'}, {'command': 'group-list --filters status=available', 'expected': - '/groups/detail?status=available'}, + '/groups/detail?all_tenants=0&status=available'}, {'command': 'group-list --filters name~=456', 'expected': - '/groups/detail?name%7E=456'}, + '/groups/detail?all_tenants=0&name~=456'}, # testcases for list group-snapshot {'command': 'group-snapshot-list --status=error --filters status=available', 'expected': - '/group_snapshots/detail?status=available'}, + '/group_snapshots/detail?all_tenants=0&status=available'}, {'command': 'group-snapshot-list --filters availability_zone=123', 'expected': - '/group_snapshots/detail?availability_zone=123'}, + '/group_snapshots/detail?all_tenants=0&availability_zone=123'}, {'command': 'group-snapshot-list --filters status~=available', 'expected': - '/group_snapshots/detail?status%7E=available'}, + '/group_snapshots/detail?all_tenants=0&status~=available'}, # testcases for list message {'command': 'message-list --event_id=123 --filters event_id=456', @@ -174,7 +174,7 @@ class ShellTest(utils.TestCase): {'command': 'message-list --filters request_id~=123', 'expected': - '/messages?request_id%7E=123'}, + '/messages?request_id~=123'}, # testcases for list attachment {'command': 'attachment-list --volume-id=123 --filters volume_id=456', @@ -187,7 +187,7 @@ class ShellTest(utils.TestCase): {'command': 'attachment-list --filters volume_id~=456', 'expected': - '/attachments?volume_id%7E=456'}, + '/attachments?volume_id~=456'}, # testcases for list backup {'command': 'backup-list --volume-id=123 --filters volume_id=456', @@ -200,7 +200,7 @@ class ShellTest(utils.TestCase): {'command': 'backup-list --filters volume_id~=456', 'expected': - '/backups/detail?volume_id%7E=456'}, + '/backups/detail?volume_id~=456'}, # testcases for list snapshot {'command': 'snapshot-list --volume-id=123 --filters volume_id=456', @@ -213,7 +213,7 @@ class ShellTest(utils.TestCase): {'command': 'snapshot-list --filters volume_id~=456', 'expected': - '/snapshots/detail?volume_id%7E=456'}, + '/snapshots/detail?volume_id~=456'}, # testcases for get pools {'command': 'get-pools --filters name=456 --detail', @@ -632,7 +632,7 @@ class ShellTest(utils.TestCase): def test_group_list(self): self.run_command('--os-volume-api-version 3.13 group-list') - self.assert_called_anytime('GET', '/groups/detail') + self.assert_called_anytime('GET', '/groups/detail?all_tenants=0') def test_group_list__with_all_tenant(self): self.run_command( @@ -691,7 +691,8 @@ class ShellTest(utils.TestCase): def test_group_snapshot_list(self): self.run_command('--os-volume-api-version 3.14 group-snapshot-list') - self.assert_called_anytime('GET', '/group_snapshots/detail') + self.assert_called_anytime('GET', + '/group_snapshots/detail?all_tenants=0') def test_group_snapshot_show(self): self.run_command('--os-volume-api-version 3.14 ' diff --git a/cinderclient/utils.py b/cinderclient/utils.py index f9af58d..28c458d 100644 --- a/cinderclient/utils.py +++ b/cinderclient/utils.py @@ -206,15 +206,27 @@ def unicode_key_value_to_string(src): def build_query_param(params, sort=False): """parse list to url query parameters""" - if params is None: - params = {} + if not params: + return "" + if not sort: param_list = list(params.items()) else: param_list = list(sorted(params.items())) query_string = parse.urlencode( - [(k, v) for (k, v) in param_list if v]) + [(k, v) for (k, v) in param_list if v not in (None, '')]) + + # urllib's parse library used to adhere to RFC 2396 until + # python 3.7. The library moved from RFC 2396 to RFC 3986 + # for quoting URL strings in python 3.7 and '~' is now + # included in the set of reserved characters. [1] + # + # Below ensures "~" is never encoded. See LP 1784728 [2] for more details. + # [1] https://docs.python.org/3/library/urllib.parse.html#url-quoting + # [2] https://bugs.launchpad.net/python-cinderclient/+bug/1784728 + query_string = query_string.replace("%7E=", "~=") + if query_string: query_string = "?%s" % (query_string,) diff --git a/cinderclient/v1/volume_snapshots.py b/cinderclient/v1/volume_snapshots.py index b7840bd..922071a 100644 --- a/cinderclient/v1/volume_snapshots.py +++ b/cinderclient/v1/volume_snapshots.py @@ -107,7 +107,7 @@ class SnapshotManager(base.ManagerWithFind): :rtype: list of :class:`Snapshot` """ - query_string = utils.build_query_param(search_opts, True) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v1/volumes.py b/cinderclient/v1/volumes.py index 699c4ea..8e25f40 100644 --- a/cinderclient/v1/volumes.py +++ b/cinderclient/v1/volumes.py @@ -203,7 +203,7 @@ class VolumeManager(base.ManagerWithFind): if limit: search_opts['limit'] = limit - query_string = utils.build_query_param(search_opts, True) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v2/limits.py b/cinderclient/v2/limits.py index 80c5bf9..dd1666d 100644 --- a/cinderclient/v2/limits.py +++ b/cinderclient/v2/limits.py @@ -14,9 +14,8 @@ # limitations under the License. """Limits interface (v2 extension)""" -from six.moves.urllib import parse - from cinderclient import base +from cinderclient import utils class Limits(base.Resource): @@ -95,6 +94,6 @@ class LimitsManager(base.Manager): if tenant_id: opts['tenant_id'] = tenant_id - query_string = "?%s" % parse.urlencode(opts) if opts else "" + query_string = utils.build_query_param(opts) return self._get("/limits%s" % query_string, "limits") diff --git a/cinderclient/v3/group_snapshots.py b/cinderclient/v3/group_snapshots.py index ed7fe79..9225995 100644 --- a/cinderclient/v3/group_snapshots.py +++ b/cinderclient/v3/group_snapshots.py @@ -96,7 +96,7 @@ class GroupSnapshotManager(base.ManagerWithFind): :param search_opts: search options :rtype: list of :class:`GroupSnapshot` """ - query_string = utils.build_query_param(search_opts) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: diff --git a/cinderclient/v3/groups.py b/cinderclient/v3/groups.py index c042e28..e42f750 100644 --- a/cinderclient/v3/groups.py +++ b/cinderclient/v3/groups.py @@ -14,8 +14,6 @@ # under the License. """Group interface (v3 extension).""" -from six.moves.urllib import parse - from cinderclient import api_versions from cinderclient.apiclient import base as common_base from cinderclient import base @@ -140,11 +138,7 @@ class GroupManager(base.ManagerWithFind): :rtype: :class:`Group` """ query_params = utils.unicode_key_value_to_string(kwargs) - - query_string = "" - if query_params: - params = sorted(query_params.items(), key=lambda x: x[0]) - query_string = "?%s" % parse.urlencode(params) + query_string = utils.build_query_param(query_params, sort=True) return self._get("/groups/%s" % group_id + query_string, "group") @@ -159,7 +153,7 @@ class GroupManager(base.ManagerWithFind): if not search_opts: search_opts = {} search_opts['list_volume'] = True - query_string = utils.build_query_param(search_opts) + query_string = utils.build_query_param(search_opts, sort=True) detail = "" if detailed: |