Repository: minio/minio-py
Pull request #875 (2020-04-08 21:06:51)

Rename urlencode and urldecode to quote and unquote
Quote and unquote are the names of the urllib functions being wrapped, this PR is a minor refactor to use those names.

Motivation is to avoid confusion when needing to use the real `urlencode` function (cf [comment](https://github.com/minio/minio-py/pull/874#discussion_r405801164)).



hardbyte: @Praveenrajmani that makes sense - I just wanted to make minimal changes.

I'll quickly refactor with what I understood your idea to be.
hardbyte: So all the tests pass if we delete the custom wrapper:

```python
def _quote(resource):
    """
    This implementation of urllib.quote supports all unicode characters

    :param: resource: Resource value to be url encoded.
    """
    if isinstance(resource, str):
        return quote(resource.encode('utf-8'))

    return quote(resource)
```

If we do this the `minio.compat` module can export `quote` and `unquote` without needing to alias either.
Praveenrajmani: > So the remaining issue is whether `minio.compat` module should export the Python standard library `quote` as well as the minio wrapper for `quote` (which I've exposed as `_quote`):
> 
> ```python
> def _quote(resource):
>     """
>     This implementation of urllib.quote supports all unicode characters
> 
>     :param: resource: Resource value to be url encoded.
>     """
>     if isinstance(resource, str):
>         return quote(resource.encode('utf-8'))
> 
>     return quote(resource)
> ```
> 
> There is no overriding function for `unquote` so I assume that exposing the standard library version as `minio.compat.unquote` is fine.

@hardbyte If there are no any overriding definitions of `unquote`, then lets us directly use it from `urllib.request` instead of chaining it over through `.compat` lib

To make sure we should not leave the other contributors to go check for the overriding definition of `unquote` and figure out that it is actually pointing to the standard one :P

Otherwise the PR looks good to me.
hardbyte: I don't think it is quite that simple because minio-py is still trying to support Python2 and I assumed having one sure place to import from (e.g. `minio.compat.unquote`) is the main idea.

`unquote` in Python 2 is found in [`urllib.unquote`](https://docs.python.org/2/library/urllib.html#urllib.unquote), in Python 3 it is found in [`urllib.parse.unquote`](https://docs.python.org/3/library/urllib.parse.html#urllib.parse.unquote) (in Python 3 `unquote` is also exported by `urllib.request`). I'll push a tweak to use the real source for Python3.
Praveenrajmani: PTAL @sinhaashish 

Patch:
diff --git a/.github/workflows/pythonpackage-windows.yml b/.github/workflows/pythonpackage-windows.yml
index cf4e509..4731cf9 100644
--- a/.github/workflows/pythonpackage-windows.yml
+++ b/.github/workflows/pythonpackage-windows.yml
@@ -46,7 +46,7 @@ jobs:
       run: |
           New-Item -ItemType Directory -Path "$env:temp/minio-config/certs/"
           Copy-Item -Path tests\certs\* -Destination "$env:temp/minio-config/certs/"
-          Invoke-WebRequest -Uri https://dl.minio.io/server/minio/release/windows-amd64/archive/minio.RELEASE.2020-03-09T18-26-53Z -OutFile $HOME/minio.exe
+          Invoke-WebRequest -Uri https://dl.minio.io/server/minio/release/windows-amd64/minio.exe -OutFile $HOME/minio.exe
           Start-Process -NoNewWindow -FilePath "$HOME/minio.exe" -ArgumentList "-C", "$env:temp/minio-config", "server", "$env:temp/fs"
           $env:SSL_CERT_FILE = "$env:temp/minio-config/certs/public.crt"
           python tests/functional/tests.py
diff --git a/minio/compat.py b/minio/compat.py
index 1743eef..ae5e964 100644
--- a/minio/compat.py
+++ b/minio/compat.py
@@ -40,15 +40,9 @@ if _is_py2:
     from Queue import Empty
     queue_empty = Empty
 
-    from urllib import quote
-    _urlencode = quote
+    from urllib import quote, unquote
 
-    from urllib import unquote
-    urldecode = unquote
-
-    import urlparse
-    urlsplit = urlparse.urlsplit
-    parse_qs = urlparse.parse_qs
+    from urlparse import urlsplit, parse_qs
 
     ## Create missing types.
     bytes = str
@@ -68,15 +62,7 @@ elif _is_py3:
     from queue import Empty
     queue_empty = Empty
 
-    from urllib.request import quote
-    _urlencode = quote
-
-    from urllib.request import unquote
-    urldecode = unquote
-
-    import urllib.parse
-    urlsplit = urllib.parse.urlsplit
-    parse_qs = urllib.parse.parse_qs
+    from urllib.parse import quote, unquote, urlsplit, parse_qs
 
     ## Create types to compat with py2.
     builtin_range = range
@@ -93,16 +79,19 @@ elif _is_py3:
 
 numeric_types = (int, long, float)
 
-def urlencode(resource):
+
+# Note earlier versions of minio.compat exposed urllib.quote as urlencode
+def _quote(resource):
     """
-    This implementation of urlencode supports all unicode characters
+    This implementation of urllib.quote supports all unicode characters
 
     :param: resource: Resource value to be url encoded.
     """
     if isinstance(resource, str):
-        return _urlencode(resource.encode('utf-8'))
+        return quote(resource.encode('utf-8'))
+
+    return quote(resource)
 
-    return _urlencode(resource)
 
 def queryencode(query):
     """
@@ -110,4 +99,4 @@ def queryencode(query):
 
     :param: query: Query value to be url encoded.
     """
-    return urlencode(query).replace('/', '%2F')
+    return _quote(query).replace('/', '%2F')
diff --git a/minio/helpers.py b/minio/helpers.py
index 61e960c..5eb57ad 100644
--- a/minio/helpers.py
+++ b/minio/helpers.py
@@ -42,7 +42,7 @@ import os
 import errno
 import math
 
-from .compat import (urlsplit, urlencode, queryencode,
+from .compat import (urlsplit, _quote, queryencode,
                      str, bytes, basestring, _is_py3, _is_py2)
 from .error import (InvalidBucketError, InvalidEndpointError,
                     InvalidArgumentError)
@@ -599,7 +599,7 @@ def encode_object_name(object_name):
     :return: URL encoded input object name.
     """
     is_non_empty_string(object_name)
-    return urlencode(object_name)
+    return _quote(object_name)
 
 class Hasher(object):
     """
diff --git a/minio/parsers.py b/minio/parsers.py
index ee05fac..a6e5d90 100644
--- a/minio/parsers.py
+++ b/minio/parsers.py
@@ -35,7 +35,7 @@ import pytz
 
 # minio specific.
 from .error import (ETREE_EXCEPTIONS, InvalidXMLError, MultiDeleteError)
-from .compat import urldecode
+from .compat import unquote
 from .definitions import (Object, Bucket, IncompleteUpload,
                           UploadPart, MultipartUploadResult,
                           CopyObjectResult)
@@ -112,7 +112,7 @@ class S3Element(object):
         """
         text = self.get_child_text(name, strict)
         # strictness is already enforced above.
-        return urldecode(text) if text is not None else None
+        return unquote(text) if text is not None else None
 
     def get_etag_elem(self, strict=True):
         """Fetches an 'ETag' child element suitably processed.


Test patch:
diff --git a/tests/unit/sign_test.py b/tests/unit/sign_test.py
index 8c3ca64..42624c6 100644
--- a/tests/unit/sign_test.py
+++ b/tests/unit/sign_test.py
@@ -24,7 +24,7 @@ from minio.signer import (generate_canonical_request, generate_string_to_sign,
                           generate_signing_key, generate_authorization_header,
                           presign_v4, sign_v4)
 from minio.error import InvalidArgumentError
-from minio.compat import urlsplit, urlencode, queryencode
+from minio.compat import urlsplit, _quote, queryencode
 from minio.fold_case_dict import FoldCaseDict
 from minio.credentials import Credentials, Static
 from minio.helpers import get_target_url
@@ -139,20 +139,20 @@ class SignV4Test(TestCase):
         eq_(hdrs['Authorization'], 'AWS4-HMAC-SHA256 Credential=minio/20150620/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=a2f4546f647981732bd90dfa5a7599c44dca92f44bea48ecc7565df06032c25b')
 
 class UnicodeEncodeTest(TestCase):
-    def test_unicode_urlencode(self):
-        eq_(urlencode('/test/123/汉字'), '/test/123/%E6%B1%89%E5%AD%97')
+    def test_unicode_quote(self):
+        eq_(_quote('/test/123/汉字'), '/test/123/%E6%B1%89%E5%AD%97')
 
     def test_unicode_queryencode(self):
         eq_(queryencode('/test/123/汉字'), '%2Ftest%2F123%2F%E6%B1%89%E5%AD%97')
 
-    def test_unicode_urlencode_u(self):
-        eq_(urlencode(u'/test/123/汉字'), '/test/123/%E6%B1%89%E5%AD%97')
+    def test_unicode_quote_u(self):
+        eq_(_quote(u'/test/123/汉字'), '/test/123/%E6%B1%89%E5%AD%97')
 
     def test_unicode_queryencode_u(self):
         eq_(queryencode(u'/test/123/汉字'), '%2Ftest%2F123%2F%E6%B1%89%E5%AD%97')
 
-    def test_unicode_urlencode_b(self):
-        eq_(urlencode(b'/test/123/\xe6\xb1\x89\xe5\xad\x97'), '/test/123/%E6%B1%89%E5%AD%97')
+    def test_unicode_quote_b(self):
+        eq_(_quote(b'/test/123/\xe6\xb1\x89\xe5\xad\x97'), '/test/123/%E6%B1%89%E5%AD%97')
 
     def test_unicode_queryencode_b(self):
         eq_(queryencode(b'/test/123/\xe6\xb1\x89\xe5\xad\x97'), '%2Ftest%2F123%2F%E6%B1%89%E5%AD%97')