Skip to content

better handling of ftp features #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 10, 2017
Merged

better handling of ftp features #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbd9b9b
better handling of ftp features
willmcgugan Oct 10, 2017
8ade6f6
linter cleanup
willmcgugan Oct 10, 2017
2353fc0
changelog
willmcgugan Oct 10, 2017
139a4fb
e -> error
willmcgugan Oct 10, 2017
61f7a67
call base class
willmcgugan Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

- settext, appendtext, appendbytes, setbytes now raise a TypeError if
the type is wrong, rather than ValueError
- More efficient feature detection for FTPFS
- Fixes for `fs.filesize`

## [2.0.11]

Expand Down
143 changes: 77 additions & 66 deletions fs/ftpfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

from contextlib import contextmanager
from ftplib import FTP
from ftplib import error_reply
from ftplib import error_perm
from ftplib import error_temp

Expand Down Expand Up @@ -44,16 +43,18 @@ def ftp_errors(fs, path=None):
raise errors.RemoteConnectionError(
msg='unable to connect to {}'.format(fs.host)
)
except error_temp as e:
except error_temp as error:
if path is not None:
raise errors.ResourceError(
path,
msg="ftp error on resource '{}' ({})".format(path, e)
msg="ftp error on resource '{}' ({})".format(path, error)
)
else:
raise errors.OperationFailed(msg='ftp error ({})'.format(e))
except error_perm as e:
code, message = parse_ftp_error(e)
raise errors.OperationFailed(
msg='ftp error ({})'.format(error)
)
except error_perm as error:
code, message = _parse_ftp_error(error)
if code == 552:
raise errors.InsufficientStorage(
path=path,
Expand All @@ -76,8 +77,10 @@ def manage_ftp(ftp):
except: # pragma: nocover
pass

def parse_ftp_error(e):
code, _, message = text_type(e).partition(' ')

def _parse_ftp_error(error):
"""Extract code and message from ftp error."""
code, _, message = text_type(error).partition(' ')
if code.isdigit():
code = int(code)
return code, message
Expand All @@ -98,6 +101,7 @@ def _decode(st, _):
class FTPFile(io.IOBase):

def __init__(self, ftpfs, path, mode):
super(FTPFile, self).__init__()
self.fs = ftpfs
self.path = path
self.mode = Mode(mode)
Expand All @@ -108,7 +112,8 @@ def __init__(self, ftpfs, path, mode):
self._write_conn = None

def _open_ftp(self):
ftp = self.fs._open_ftp(self.fs.ftp.encoding)
"""Open an ftp object for the file."""
ftp = self.fs._open_ftp()
ftp.voidcmd(str('TYPE I'))
return ftp

Expand Down Expand Up @@ -302,6 +307,7 @@ def __init__(self,
self.timeout = timeout
self.port = port

self.encoding = 'latin-1'
self._ftp = None
self._welcome = None
self._features = None
Expand All @@ -317,21 +323,52 @@ def __str__(self):
)
return _fmt.format(host=self.host, port=self.port)

def _open_ftp(self, encoding="utf-8"):
@classmethod
def _parse_features(cls, feat_response):
"""Parse a dict of features from FTP feat response."""
features = {}
if feat_response.split('-')[0] == '211':
for line in feat_response.splitlines():
if line.startswith(' '):
key, _, value = line[1:].partition(' ')
features[key] = value
return features

def _open_ftp(self):
"""Open a new ftp object."""
_ftp = FTP()
_ftp.set_debuglevel(0)
with ftp_errors(self):
_ftp.encoding = encoding
_ftp.connect(self.host, self.port, self.timeout)
_ftp.login(self.user, self.passwd, self.acct)
self._features = {}
try:
feat_response = _decode(_ftp.sendcmd("FEAT"), 'latin-1')
except error_perm:
self.encoding = 'latin-1'
else:
self._features = self._parse_features(feat_response)
self.encoding = (
'utf-8'
if 'UTF8' in self._features
else 'latin-1'
)
if not PY2:
_ftp.file = _ftp.sock.makefile(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how much of a hack this is. The file attribute of FTP objects is the only thing that references the encoding, by re-creating it, we can modify the encoding based on features.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine, I don't think the ftplib implementation will change since it is part of the stdlib.

Isn't your linter complaining about the seemingly-useless self.ftp lines in other methods though ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine, I don't think the ftplib implementation will change since it is part of the stdlib.

That's my thinking.

Isn't your linter complaining about the seemingly-useless self.ftp lines in other methods though ?

It is, and a few more complaints besides. I'll see if I can clear some of those up.

'r',
encoding=self.encoding
)
_ftp.encoding = self.encoding
self._welcome = _ftp.welcome
return _ftp

def _manage_ftp(self):
ftp = self._open_ftp(self.ftp.encoding)
ftp = self._open_ftp()
return manage_ftp(ftp)

@property
def ftp_url(self):
"""Get the FTP url this filesystem will open."""
url = (
"ftp://{}".format(self.host)
if self.port == 21
Expand All @@ -342,39 +379,17 @@ def ftp_url(self):
@property
def ftp(self):
"""Get a FTP (ftplib) object."""
return self._get_ftp()

def _get_ftp(self):
if self._ftp is None:
_ftp = self._open_ftp('latin-1')
try:
encoding = (
'utf-8'
if 'UTF8' in _ftp.sendcmd('FEAT')
else 'latin-1'
)
except error_perm:
encoding = 'latin-1'
self._ftp = (
_ftp
if encoding == 'latin-1'
else self._open_ftp(encoding)
)
self._welcome = self._ftp.getwelcome()
self._ftp = self._open_ftp()
return self._ftp

@property
def features(self):
"""Get features dict from FTP server."""
if self._features is None:
try:
response = _decode(self.ftp.sendcmd("FEAT"), "ascii")
except error_perm:
self._features = {}
else:
self._features = {}
if response.split('-')[0] == '211':
for line in response.splitlines():
if line.startswith(' '):
k, _, v = line[1:].partition(' ')
self._features[k] = v
self._get_ftp()
return self._features

def _read_dir(self, path):
Expand All @@ -395,6 +410,7 @@ def _read_dir(self, path):

@property
def supports_mlst(self):
"""Check if server supports MLST feature."""
return 'MLST' in self.features

def create(self, path, wipe=False):
Expand All @@ -408,14 +424,15 @@ def create(self, path, wipe=False):
)

@classmethod
def _parse_ftp_time(cls, t):
def _parse_ftp_time(cls, time_text):
"""Parse a time from an ftp directory listing."""
try:
tm_year = int(t[0:4])
tm_month = int(t[4:6])
tm_day = int(t[6:8])
tm_hour = int(t[8:10])
tm_min = int(t[10:12])
tm_sec = int(t[12:14])
tm_year = int(time_text[0:4])
tm_month = int(time_text[4:6])
tm_day = int(time_text[6:8])
tm_hour = int(time_text[8:10])
tm_min = int(time_text[10:12])
tm_sec = int(time_text[12:14])
except ValueError:
return None
epoch_time = calendar.timegm((
Expand All @@ -433,11 +450,11 @@ def _parse_facts(cls, line):
name = None
facts = {}
for fact in line.split(';'):
k, sep, v = fact.partition('=')
key, sep, value = fact.partition('=')
if sep:
k = k.strip().lower()
v = v.strip()
facts[k] = v
key = key.strip().lower()
value = value.strip()
facts[key] = value
else:
name = basename(fact.rstrip('/').strip())
return name if name not in ('.', '..') else None, facts
Expand Down Expand Up @@ -514,6 +531,7 @@ def getinfo(self, path, namespaces=None):

def getmeta(self, namespace="standard"):
_meta = {}
self._get_ftp()
if namespace == "standard":
_meta = self._meta.copy()
_meta['unicode_paths'] = "UTF8" in self.features
Expand Down Expand Up @@ -542,8 +560,8 @@ def makedir(self, path, permissions=None, recreate=False):
if not (recreate and self.isdir(path)):
try:
self.ftp.mkd(_encode(_path, self.ftp.encoding))
except error_perm as e:
code, _ = parse_ftp_error(e)
except error_perm as error:
code, _ = _parse_ftp_error(error)
if code == 550:
if self.isdir(path):
raise errors.DirectoryExists(path)
Expand All @@ -569,13 +587,12 @@ def openbin(self, path, mode="r", buffering=-1, **options):
raise errors.FileExpected(path)
if _mode.exclusive:
raise errors.FileExists(path)
f = FTPFile(self, _path, mode)
return f
ftp_file = FTPFile(self, _path, mode)
return ftp_file

def remove(self, path):
self.check()
_path = self.validatepath(path)
dir_name, file_name = split(_path)
with self._lock:
if self.isdir(path):
raise errors.FileExpected(path=path)
Expand All @@ -586,13 +603,12 @@ def removedir(self, path):
_path = self.validatepath(path)
if _path == '/':
raise errors.RemoveRootError()
_dir_name, file_name = split(_path)

with ftp_errors(self, path):
try:
self.ftp.rmd(_encode(_path, self.ftp.encoding))
except error_perm as e:
code, _ = parse_ftp_error(e)
except error_perm as error:
code, _ = _parse_ftp_error(error)
if code == 550:
if self.isfile(path):
raise errors.DirectoryExpected(path)
Expand All @@ -611,7 +627,7 @@ def _scandir(self, path, namespaces=None):
str("MLSD ") + _encode(_path, self.ftp.encoding),
lambda l: lines.append(_decode(l, self.ftp.encoding))
)
except error_perm as e:
except error_perm:
if not self.getinfo(path).is_dir:
raise errors.DirectoryExpected(path)
raise # pragma: no cover
Expand Down Expand Up @@ -661,8 +677,8 @@ def getbytes(self, path):
str("RETR ") + _encode(_path, self.ftp.encoding),
data.write
)
except error_perm as e:
code, _ = parse_ftp_error(e)
except error_perm as error:
code, _ = _parse_ftp_error(error)
if code == 550:
if self.isdir(path):
raise errors.FileExpected(path)
Expand All @@ -679,8 +695,3 @@ def close(self):
pass
self._ftp = None
super(FTPFS, self).close()


if __name__ == "__main__": # pragma: no cover
fs = FTPFS('ftp.mirror.nl', 'anonymous', '[email protected]')
print(list(fs.scandir('/')))
6 changes: 0 additions & 6 deletions tests/test_ftpfs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,6 @@ def test_connection_error(self):
with self.assertRaises(errors.RemoteConnectionError):
fs.open('foo.txt')

def test_features(self):
def broken_sendcmd(cmd):
raise ftplib.error_perm('nope')
self.fs.ftp.sendcmd = broken_sendcmd
self.assertEqual(self.fs.features, {})

def test_getmeta_unicode_path(self):
self.assertTrue(self.fs.getmeta().get('unicode_paths'))
self.fs.features
Expand Down