Merge "Fix encoding of query parameters"

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)

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:
+        query_string = utils.build_query_param(query_params, sort=True)
-            params = sorted(query_params.items(), key=lambda x: x[0])
-            query_string = "?%s" % parse.urlencode(params)
 
         detail = ""
         if detailed:

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):
+    @ddt.data({},
-        dict_param = {
+              None,
-            'key1': 'val1',
+              {'key1': 'val1', 'key2': None, 'key3': False, 'key4': ''})
-            'key2': None,
+    def test_build_param_with_nones(self, dict_param):
-            'key3': False,
+        result = utils.build_query_param(dict_param)
-            'key4': ''
-        }
-        result_1 = utils.build_query_param(dict_param)
-        result_2 = utils.build_query_param(None)
 
-        expected = "?key1=val1"
+        expected = ("key1=val1", "key3=False") if dict_param else ()
-        self.assertEqual(expected, result_1)
+        for exp in expected:
-        self.assertFalse(result_2)
+            self.assertIn(exp, result)
+        if not expected:
+            self.assertEqual("", result)
 
 
 @ddt.ddt

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

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

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 '

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:
+    if not params:
-        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,)
 

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:

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:

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

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:

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 = utils.build_query_param(query_params, sort=True)
-        query_string = ""
-        if query_params:
-            params = sorted(query_params.items(), key=lambda x: x[0])
-            query_string = "?%s" % parse.urlencode(params)
 
         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: