openstack
/
python-cinderclient
Code Issues Proposed changes

Merge "Fix encoding of query parameters"

This commit is contained in:
Zuul 2018-10-03 17:54:19 +00:00 committed by Gerrit Code Review
parent c73844df2d 223d754f61
commit 4e17e1d191
12 changed files with 64 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
cinderclient/apiclient/base.py
View File

@ -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)

7
cinderclient/base.py
View File

@ -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:

23
cinderclient/tests/unit/test_utils.py
View File

@ -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)
@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"
self.assertEqual(expected, result_1)
self.assertFalse(result_2)
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

17
cinderclient/tests/unit/v1/test_shell.py
View File

@ -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')

2
cinderclient/tests/unit/v2/test_shell.py
View File

@ -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')

29
cinderclient/tests/unit/v3/test_shell.py
View File

@ -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 '

18
cinderclient/utils.py
View File

@ -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,)

2
cinderclient/v1/volume_snapshots.py
View File

@ -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:

2
cinderclient/v1/volumes.py
View File

@ -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:

5
cinderclient/v2/limits.py
View File

@ -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")

2
cinderclient/v3/group_snapshots.py
View File

@ -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:

10
cinderclient/v3/groups.py
View File

@ -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: