url: resource and query encoding should support unicode #530

harshavardhana · 2017-06-06T02:28:33Z

python2.7 lacks proper unicode support, so we need
a proper wrapper function for url encoding. With
this the url and query encodings works for all
unicode characters.

Fixes #529

Yurmusp · 2017-06-06T19:53:53Z

minio/api.py

@@ -44,7 +44,8 @@

 # Internal imports
 from . import __title__, __version__
-from .compat import urlsplit, range, urlencode, basestring
+from .compat import (urlsplit, queryencode,


I see issue in your files/code.
It depends what you target for. I see it from the standpoint what I would target for.

I would target for have everything in your lib unicode. I mean all coming and outgoing strings.
For that typically programmer needs to have two things enabled shown as two lines below.

# encoding: utf-8 from __future__ import unicode_literals

First line says that this file is UTF-8.
Second line makes every string to be unicode string in this file (not just base string with UTF-8 encoding in it).
What you have now is only first line out of two in this file.
So, your file has all base strings with UTF-8. NOT really unicode strings everywhere.
I'm not sure what you want.
You still can try to go with base strings and UTF-8, but that seems assumed to be more complicated way
with many side effects that needs to be handled.
Hope this can be useful.
All this message is from python 2.7 point of view of course. Not 3.x

This is a good idea @Yurmusp will test it out and see if there are any problems for 3.x

Not sure what was meant. Who will test it?

"will" means "we will" shorter way to saying it :-)

In my understanding. Change I suggested shall have no impact on 3.x.
Because this change is actually to make everything more like 3.x
(every string to be unicode) but within 2.x python.

We won't be doing this for now this PR works as is.. We will take this activity as such an issue comes in future. We don't want to introduce subtle changes like this which can have unexpected consequences.

As long as it works, it's fine I think.

Yurmusp · 2017-06-06T20:13:22Z

:) OK. Sounds good.

…

On Tue, Jun 6, 2017 at 1:11 PM, Harshavardhana ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In minio/api.py <#530 (comment)>: > @@ -44,7 +44,8 @@ # Internal imports from . import __title__, __version__ -from .compat import urlsplit, range, urlencode, basestring +from .compat import (urlsplit, queryencode, "will" means "we will" shorter way to saying it :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWEzQmbMs2WY3qzGnwd8ox4rRGosGdf3ks5sBbKKgaJpZM4NwyXQ> .

Yurmusp · 2017-06-06T20:28:55Z

Please, let me know when done.
How long you think it will take?

deekoder · 2017-06-06T21:40:23Z

We have a process to review and test. As soon that's done it will be merged.

Yurmusp · 2017-06-06T21:42:30Z

Is it days, weeks, months? can you tell what is a ball park usually?

…

On Tue, Jun 6, 2017 at 2:40 PM, Dee Koder ***@***.***> wrote: We have a process to review and test. As soon that's done it will be merged. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWEzQoxDIvXa0iSnjxU_-z0YidhHVG6Cks5sBcdHgaJpZM4NwyXQ> .

harshavardhana · 2017-06-06T21:49:55Z

Is it days, weeks, months?
can you tell what is a ball park usually?

Usually 2-3 days.. @Yurmusp

Yurmusp · 2017-06-06T21:51:03Z

Sounds good. Thank you.

…

On Tue, Jun 6, 2017 at 2:49 PM, Harshavardhana ***@***.***> wrote: Is it days, weeks, months? can you tell what is a ball park usually? Usually 2-3 days.. @Yurmusp <https://github.com/yurmusp> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWEzQtMX_pq1GH07ygit6tZPcYhNzj1uks5sBcmEgaJpZM4NwyXQ> .

deekoder · 2017-06-12T01:58:00Z

@Yurmusp its taking a bit longer because we are right now doing some fixes in minio/minio. Please stay tuned on this one.

Yurmusp · 2017-06-12T20:33:20Z

Yes. Waiting. Please, do not delay too much if possible. Thanks.

…

On Sun, Jun 11, 2017 at 6:58 PM, Dee Koder ***@***.***> wrote: @Yurmusp <https://github.com/yurmusp> its taking a bit longer because we are right now doing some fixes in minio/minio. Please stay tuned on this one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWEzQikK2j3d7IjaNhxxTooPlqxFrUBnks5sDJspgaJpZM4NwyXQ> .

Yurmusp · 2017-06-16T23:20:14Z

Guys, Any progress?

…

On Mon, Jun 12, 2017 at 1:32 PM, Yuriy Musatenko ***@***.***> wrote: Yes. Waiting. Please, do not delay too much if possible. Thanks. On Sun, Jun 11, 2017 at 6:58 PM, Dee Koder ***@***.***> wrote: > @Yurmusp <https://github.com/yurmusp> its taking a bit longer because we > are right now doing some fixes in minio/minio. Please stay tuned on this > one. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#530 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AWEzQikK2j3d7IjaNhxxTooPlqxFrUBnks5sDJspgaJpZM4NwyXQ> > . >

poornas

LGTM.

python2.7 lacks proper unicode support, so we need a proper wrapper function for url encoding. With this the url and query encodings works for all unicode characters. Fixes minio#529

Yurmusp · 2017-06-19T21:38:31Z

When you guys expect it to be released?

Yurmusp · 2017-06-27T01:48:48Z

Thank you guys. Looks like this works.

harshavardhana force-pushed the fix-unicode branch 2 times, most recently from f7a8e17 to 0208fd3 Compare June 6, 2017 02:36

harshavardhana mentioned this pull request Jun 6, 2017

Unicode object name on python 2.7 causes exception #529

Closed

harshavardhana requested review from vadmeste and donatello June 6, 2017 14:25

Yurmusp reviewed Jun 6, 2017

View reviewed changes

deekoder requested review from poornas and removed request for donatello June 12, 2017 01:58

harshavardhana force-pushed the fix-unicode branch from 0208fd3 to c46bdc7 Compare June 16, 2017 23:32

harshavardhana requested review from donatello and removed request for vadmeste June 16, 2017 23:34

poornas approved these changes Jun 16, 2017

View reviewed changes

harshavardhana force-pushed the fix-unicode branch 6 times, most recently from 5f06810 to 57daf09 Compare June 17, 2017 01:05

url: resource and query encoding should support unicode

c6926bd

python2.7 lacks proper unicode support, so we need a proper wrapper function for url encoding. With this the url and query encodings works for all unicode characters. Fixes minio#529

harshavardhana force-pushed the fix-unicode branch from 57daf09 to c6926bd Compare June 17, 2017 01:37

donatello approved these changes Jun 19, 2017

View reviewed changes

donatello merged commit 4cc6b09 into minio:master Jun 19, 2017

harshavardhana deleted the fix-unicode branch June 19, 2017 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: resource and query encoding should support unicode #530

url: resource and query encoding should support unicode #530

harshavardhana commented Jun 6, 2017

Yurmusp Jun 6, 2017 •

edited

Loading

harshavardhana Jun 6, 2017

Yurmusp Jun 6, 2017

harshavardhana Jun 6, 2017

Yurmusp Jun 16, 2017

harshavardhana Jun 16, 2017

Yurmusp Jun 16, 2017

Yurmusp commented Jun 6, 2017 via email •

edited

Loading

Yurmusp commented Jun 6, 2017

deekoder commented Jun 6, 2017

Yurmusp commented Jun 6, 2017 via email

harshavardhana commented Jun 6, 2017

Yurmusp commented Jun 6, 2017 via email

deekoder commented Jun 12, 2017

Yurmusp commented Jun 12, 2017 via email

Yurmusp commented Jun 16, 2017 via email

poornas left a comment

Yurmusp commented Jun 19, 2017

Yurmusp commented Jun 27, 2017

url: resource and query encoding should support unicode #530

url: resource and query encoding should support unicode #530

Conversation

harshavardhana commented Jun 6, 2017

Yurmusp Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

harshavardhana Jun 6, 2017

Choose a reason for hiding this comment

Yurmusp Jun 6, 2017

Choose a reason for hiding this comment

harshavardhana Jun 6, 2017

Choose a reason for hiding this comment

Yurmusp Jun 16, 2017

Choose a reason for hiding this comment

harshavardhana Jun 16, 2017

Choose a reason for hiding this comment

Yurmusp Jun 16, 2017

Choose a reason for hiding this comment

Yurmusp commented Jun 6, 2017 via email • edited Loading

Yurmusp commented Jun 6, 2017

deekoder commented Jun 6, 2017

Yurmusp commented Jun 6, 2017 via email

harshavardhana commented Jun 6, 2017

Yurmusp commented Jun 6, 2017 via email

deekoder commented Jun 12, 2017

Yurmusp commented Jun 12, 2017 via email

Yurmusp commented Jun 16, 2017 via email

poornas left a comment

Choose a reason for hiding this comment

Yurmusp commented Jun 19, 2017

Yurmusp commented Jun 27, 2017

Yurmusp Jun 6, 2017 •

edited

Loading

Yurmusp commented Jun 6, 2017 via email •

edited

Loading