search

pombreda / pyftpdlib

Automatically exported from code.google.com/p/pyftpdlib
Other
0 stars 0 forks source link

Patch to add unicode support to trunk #198

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Hey guys - we've forked the code and ported the unicode support from the py3k 
branch over to it. I've rebased against trunk today and you can find our code 
here:

https://github.com/iwebhosting/pyftpdlib/

Purpose of code changes on this branch:
To add utf-8 support to trunk/python 2.x

When reviewing my code changes, please focus on:
We have one failing test that I'm not sure what to do with - test_oob_abor. 

One of these two tests may be incorrect. The Python 2 version sends 0xf4 0xff 
but the Python 3 version sends 0xc3 0xb4 0xc3 0xb4 - according to 
http://support.microsoft.com/kb/231866 it looks like it *should* be 0xff 0xf4 
(the *reverse* of the python 2 test) - which version should we attempt to get 
the code to pass?

Here's the 3k test: 
http://code.google.com/p/pyftpdlib/source/browse/branches/trunk-3k/test/test_ftp
d.py#1563

Also I've omitted test_wrong_encoding:

http://code.google.com/p/pyftpdlib/source/browse/branches/trunk-3k/test/test_ftp
d.py#2277

It wasn't raising a UnicodeDecodeError because all of the bytes sent fit into 
utf-8.

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by darren.w...@gmail.com on 10 Jan 2012 at 4:42

GoogleCodeExporter commented 9 years ago 
Oh! This is interesting.
I took a quick look at what you've done.
Starting from py3k branch was a good idea but it shouldn't be imported into 
trunk as-is as it uses some statements which will work on python >= 2.6 only 
and we need to support python 2.4.
I mean this:

return b''.join(buffer)
self.set_terminator(b"\r\n")
...

Basically all the b'' occurrences must be replaced with ''.
Also, any other reference which refers to the python 3.x porting must be 
removed. Example:

str(line, 'utf8')

Other than that and your failing test issue, how's the status of this?
Have you tried it in production? 
With what encodings?

Basically, if you successfully manage to add UTF8 support in your branch and 
enrich the test suite to prove it works and doesn't break the current 
bytes-based implementation, the next step would consist in producing a patch 
from your branch and apply it to main pyftpdlib trunk.

Original comment by g.rodola on 10 Jan 2012 at 5:44

GoogleCodeExporter commented 9 years ago 
Yep, we're using it with a low volume of traffic and its working fine for us 
using utf-8. I wasnt aware of the >=python 2.4 requirement, I'll get that 
sorted and remove those shamelessly copied docstrings :)

There are a couple of tests (MKD and MSLT) but I'll put a few more in there now 
I'm more familiar with how the tests work - thanks for the feedback. We have 
ran through a suite of operations using Cyberduck and Filezilla though, and 
they're fine in those clients at least 
(upload/download/mkdir/list/rename/delete).

Original comment by darren.w...@gmail.com on 10 Jan 2012 at 5:57

GoogleCodeExporter commented 9 years ago 
Ok, I think thats covered what you asked for - tests pass (except for the 
oob_abor) on python 2.4-2.7 (tested on Ubuntu 2.6.38 x64). I've attached a 
patch against trunk - how does that look?

Original comment by darren.w...@gmail.com on 11 Jan 2012 at 10:50

Attachments:

GoogleCodeExporter commented 9 years ago 
Lets try that again :)

Original comment by darren.w...@gmail.com on 11 Jan 2012 at 10:55

Attachments:

GoogleCodeExporter commented 9 years ago 
I'm probably missing something but I extracted the tests from your patch (tests 
only, no changes to ftpserver.py) and they work fine with the current 
bytes-based version.
Tried with filezilla as a client, and I was able to delete:

127.0.0.1:41796 <== DELE ☃ci☃ao
127.0.0.1:41796 ==> 250 File removed.

I'm confused here. Wasn't that supposed to NOT work? =)
And yes, I admit I've never fully understood how to work with unicode in python.

Original comment by g.rodola on 11 Jan 2012 at 9:56

GoogleCodeExporter commented 9 years ago 
Heh, I'm not 100% sure either :)

When we forked the code before Christmas (October or so I think) it didnt work, 
maybe something has changed since then? We only rebased against trunk a few 
days ago.

Original comment by darren.w...@gmail.com on 11 Jan 2012 at 10:15

GoogleCodeExporter commented 9 years ago 
Ok, I see what's happened here.

http://docs.python.org/howto/unicode.html#unicode-filenames

listdir() will return unicode strings *if called* with a unicode string, which 
is what we were doing in our filesystem subclass. If we switch to byte strings, 
pyftpdlib doesn't care and the clients figure out the encoding on their own.

So it's up to you if you want to keep this patch - it will explicitly make sure 
the that we're dealing with bytes. May as well keep the tests in either case :) 
Sorry for the confusion!

Original comment by darren.w...@gmail.com on 12 Jan 2012 at 3:31

GoogleCodeExporter commented 9 years ago 
I had a chat with Josiah Carlson who kindly explained to me how this is 
supposed to work (chat log is in attachment).
Basically what we have to do is this:

- read data (bytes) from the socket and convert that to unicode (line = 
sock.read(1024); line = line.decode('utf8'))
- pass the unicode string the the underlying filesystem functions (e.g. ls = 
os.listdir(u'path')) which will return unicode strings rather than bytes
- before sending the result back to client, encode the unicode string (ls = 
ls.encode('utf8'); sock.sendall(ls))

This is a pretty huge change but I think it's worth the effort as it should 
make the python 3 porting a lot easier.
Being a major change, it's likely we're going to break existing code. As such, 
this should happen in a major release (0.8.0 maybe).

The only thing which is not clear to me is what to do in case the client sends 
a line with a malformed encoding (!= UTF8).
RFC-2640 is not clear about this.

Original comment by g.rodola on 13 Jan 2012 at 6:25

Attachments:

GoogleCodeExporter commented 9 years ago 
I added your tests as r958.

Original comment by g.rodola on 14 Jan 2012 at 2:18

GoogleCodeExporter commented 9 years ago 
Ok, preliminary patch in attachment (applied to r963) adds 2.x unicode support.
It seems to work and also passes all tests.
I fixed OOB test failure was with:

line = line.decode('utf8', errors='replace')

Not sure whether it is better to return an error response though.

Original comment by g.rodola on 14 Jan 2012 at 5:13

Attachments:

GoogleCodeExporter commented 9 years ago 
New patch (based on r967) including test changes I previously left out by 
accident.

Original comment by g.rodola on 14 Jan 2012 at 8:20

Attachments:

GoogleCodeExporter commented 9 years ago 
Sorry for the big delay - busy work week and a new born at home is keeping me 
occupied :)

The patch looks great to me - I've ran it internally and it certainly behaves 
the same as well.

Is the 0.7 release imminent?

Original comment by darren.w...@gmail.com on 22 Jan 2012 at 5:37

GoogleCodeExporter commented 9 years ago 
Yep, I hope to release it next week.
After then, we can start working on this.
We'll first introduce unicode support for python 2.x, stabilize it as much as 
possible, and then start the transition towards python 3.x as I already did 
with psutil:
http://code.google.com/p/psutil/issues/detail?id=75
That way both python 2.x and 3.x will share the same code base and, most 
important, the same API.

It's likely that the new version supporting unicode and python 3 will be marked 
as 1.0.0 since that's going to break existent code using a customized 
filesystem class.

Original comment by g.rodola on 22 Jan 2012 at 7:08

GoogleCodeExporter commented 9 years ago 
Issue 121 has been merged into this issue.

Original comment by g.rodola on 22 May 2012 at 12:16

GoogleCodeExporter commented 9 years ago 
Ladies and gents, we made it: we now have full unicode support (RFC-2640) as 
part of the process which ported pyftpdlib to python 3.
Attached is a patch including all the changes which were submitted against the 
py3-unicode branch and which are now merged back into trunk as r1046.

The change was massive in that we now use unicode instead of bytes pretty much 
everywhere, so it's likely that existent code bases overriding the base 
authorizer or filesystem classes won't work.
I'll write down instructions on how to make the porting later on.

Original comment by g.rodola on 22 May 2012 at 12:33

Attachments:

GoogleCodeExporter commented 9 years ago 
Releasing 1.0.0 just now. Closing.

Original comment by g.rodola on 19 Feb 2013 at 12:49