Bazaar

Merge lp:~mnn282/bzr/sftp-unsupported-operation-more-info into lp:bzr

  1. sftp-unsupported-operation-more-info
  2. Merge into bzr.dev
Proposed by mnn
Status: Needs review
Proposed branch: lp:~mnn282/bzr/sftp-unsupported-operation-more-info
Merge into: lp:bzr
Diff against target: 218 lines (+70/-15)
4 files modified
bzrlib/errors.py (+10/-0)
bzrlib/tests/test_sftp_transport.py (+37/-0)
bzrlib/transport/__init__.py (+5/-4)
bzrlib/transport/sftp.py (+18/-11)
To merge this branch: bzr merge lp:~mnn282/bzr/sftp-unsupported-operation-more-info
Reviewer Review Type Date Requested Status
Martin Packman (community) Needs Fixing
Review via email: mp+116849@code.launchpad.net

Description of the change

Unsupported operations now have more information in SFTP transport.

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :

Idea of adding a new exception here seems reasonable.

You'll want to add it inside _translate_io_exception rather than adding a special separate branch before hand. That might mean tweaking that method a little, but it's private so there's no reason to to adapt it.

You want some tests as well, you can probably just test the translation itself rather than trying to fake out paramiko to raise the exception you're observing.

review: Needs Fixing
Reply
lp:~mnn282/bzr/sftp-unsupported-operation-more-info updated
6546. By mnn

Moved handling of unsupported operations into SFTPTransport._translate_io_exception, which has now optional argument 'operation'

SFTPTransport._translate_io_exception and Transport._translate_error are now static methods

Revision history for this message
mnn (mnn282) wrote :

Martin Packman: So, the handling has been moved into _translate_io_exception. I also added tests, as you requested.

I was told on IRC that I should make _translate_io_exception static/class method, so tests actually won't be depended on SFTP transport itself - I had to make Transport._translate_error static too, because _translate_io_exception was invoking it.

Reply
Revision history for this message
Martin Packman (gz) wrote :

Thanks for making those changes, not all the suggestions I made were completely correct so I hope you don't mind poking some of the minor details again.

Style nit, two lines between top level things in modules, see:
<http://www.python.org/dev/peps/pep-0008/#blank-lines>

+ self.message = "\n" + message

So, I said 'msg' wasn't a great name, but annoyingly 'message' is worse due to some legacy Python exception stuff. How about 'description' or something?

Also, having english text as a param prevents that part from being translated, but this isn't too important.

Including the path, and just using the operation name rather than the text form would work for everything but the symlink/rename case, which could just be an optional target_path param to the exception.

+ self.assertEquals(e.message, "\nunable to symlink")
\ No newline at end of file

As with the other mp, this needs a terminal newline or bt.test_source fails.

+ @staticmethod
+ def _translate_io_exception(e, path, more_info='',
...
- self._translate_error(e, path, raise_generic=False)
+ Transport._translate_error(e, path, raise_generic=False)

The neat way around needing the Transport import for this is to instead do:

    @classmethod
    def _translate_io_exception(cls, e, path, more_info='',
...
        cls._translate_error(e, path, raise_generic=False)

Leaving _translate_error as @staticerror is fine.

+ # raise errors.TransportNotPossible(more_info)

Just delete this.

review: Needs Fixing
Reply
lp:~mnn282/bzr/sftp-unsupported-operation-more-info updated
6547. By mnn

- SFTPTransport._translate_io_exception changed to @classmethod
- changed TransportOperationNotSupported.message to description

Revision history for this message
mnn (mnn282) wrote :

> Also, having english text as a param prevents that part from being translated, but this isn't too important.
But other exception handling code in SFTPTransport has only english text as parameter too.

> Including the path, and just using the operation name rather than the text form would work for everything but the symlink/rename case, which could just be an optional target_path param to the exception.
This would require altering _translate_io_exception parameters even more.

Reply
Revision history for this message
Martin Packman (gz) wrote :

> But other exception handling code in SFTPTransport has only english text as
> parameter too.

Right I wouldn't worry to much about doing this.

> This would require altering _translate_io_exception parameters even more.

I think passing the path to the exception is potentially useful, and shouldn't mean too much extra fiddling.

Reply

Unmerged revisions

6547. By mnn

- SFTPTransport._translate_io_exception changed to @classmethod
- changed TransportOperationNotSupported.message to description

6546. By mnn

Moved handling of unsupported operations into SFTPTransport._translate_io_exception, which has now optional argument 'operation'

SFTPTransport._translate_io_exception and Transport._translate_error are now static methods

6545. By mnn

Display more information for TransportOperationNotSupported (for rename - path from and path to)

6544. By mnn

Unsupported operations now have more information in SFTP transport

- stubs for unsupported transport operations (TransportOperationNotSupported) - used by rename operations in SFTP transport

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/errors.py'
2--- bzrlib/errors.py 2012-07-19 19:27:22 +0000
3+++ bzrlib/errors.py 2012-07-31 15:32:30 +0000
4@@ -1619,6 +1619,16 @@
5 self.traceback_text = ''.join(traceback_strings)
6
7
8+class TransportOperationNotSupported(TransportError):
9+
10+ _fmt = "Transport operation is not supported: %(operation)s%(description)s"
11+
12+ def __init__(self, operation, description=None):
13+ if description != None:
14+ self.description = "\n" + description
15+ self.operation = operation
16+
17+
18 # A set of semi-meaningful errors which can be thrown
19 class TransportNotPossible(TransportError):
20
21
22=== modified file 'bzrlib/tests/test_sftp_transport.py'
23--- bzrlib/tests/test_sftp_transport.py 2012-02-23 23:26:35 +0000
24+++ bzrlib/tests/test_sftp_transport.py 2012-07-31 15:32:30 +0000
25@@ -497,3 +497,40 @@
26 # No prompts should've been printed, stdin shouldn't have been read
27 self.assertEquals("", stdout.getvalue())
28 self.assertEquals(0, ui.ui_factory.stdin.tell())
29+
30+class TestTranslateIOException(TestCase):
31+ """Test that unsupported operation exception from Paramiko is correctly
32+ translated into TransportOperationNotSupported."""
33+
34+ def test_rename_translation(self):
35+ """Test unsupported rename operation by matching raised exception"""
36+
37+ e = self.assertRaises(
38+ errors.TransportOperationNotSupported,
39+ _mod_sftp.SFTPTransport._translate_io_exception,
40+ IOError("Operation unsupported"), "",
41+ "unable to rename", operation="rename")
42+ self.assertEquals(e.operation, "rename")
43+ self.assertEquals(e.message, "\nunable to rename")
44+
45+ def test_readlink_translation(self):
46+ """Test unsupported readlink operation by matching raised exception"""
47+
48+ e = self.assertRaises(
49+ errors.TransportOperationNotSupported,
50+ _mod_sftp.SFTPTransport._translate_io_exception,
51+ IOError("Operation unsupported"), "",
52+ "unable to readlink", operation="readlink")
53+ self.assertEquals(e.operation, "readlink")
54+ self.assertEquals(e.message, "\nunable to readlink")
55+
56+ def test_symlink_translation(self):
57+ """Test unsupported symlink operation by matching raised exception"""
58+
59+ e = self.assertRaises(
60+ errors.TransportOperationNotSupported,
61+ _mod_sftp.SFTPTransport._translate_io_exception,
62+ IOError("Operation unsupported"), "",
63+ "unable to symlink", operation="symlink")
64+ self.assertEquals(e.operation, "symlink")
65+ self.assertEquals(e.message, "\nunable to symlink")
66
67=== modified file 'bzrlib/transport/__init__.py'
68--- bzrlib/transport/__init__.py 2012-03-13 17:25:29 +0000
69+++ bzrlib/transport/__init__.py 2012-07-31 15:32:30 +0000
70@@ -244,7 +244,7 @@
71 def fdatasync(self):
72 """Force data out to physical disk if possible.
73
74- :raises TransportNotPossible: If this transport has no way to
75+ :raises TransportNotPossible: If this transport has no way to
76 flush to disk.
77 """
78 raise errors.TransportNotPossible(
79@@ -321,7 +321,7 @@
80 # where the biggest benefit between combining reads and
81 # and seeking is. Consider a runtime auto-tune.
82 _bytes_to_read_before_seek = 0
83-
84+
85 hooks = TransportHooks()
86
87 def __init__(self, base):
88@@ -330,7 +330,8 @@
89 (self._raw_base, self._segment_parameters) = (
90 urlutils.split_segment_parameters(base))
91
92- def _translate_error(self, e, path, raise_generic=True):
93+ @staticmethod
94+ def _translate_error(e, path, raise_generic=True):
95 """Translate an IOError or OSError into an appropriate bzr error.
96
97 This handles things like ENOENT, ENOTDIR, EEXIST, and EACCESS
98@@ -1631,7 +1632,7 @@
99
100 def get_transport_from_url(url, possible_transports=None):
101 """Open a transport to access a URL.
102-
103+
104 :param base: a URL
105 :param transports: optional reusable transports list. If not None, created
106 transports will be added to the list.
107
108=== modified file 'bzrlib/transport/sftp.py'
109--- bzrlib/transport/sftp.py 2011-12-19 13:23:58 +0000
110+++ bzrlib/transport/sftp.py 2012-07-31 15:32:30 +0000
111@@ -55,6 +55,7 @@
112 _file_streams,
113 ssh,
114 ConnectedTransport,
115+ Transport
116 )
117
118 # Disable one particular warning that comes from paramiko in Python2.5; if
119@@ -670,8 +671,9 @@
120 _file_streams[self.abspath(relpath)] = handle
121 return FileFileStream(self, relpath, handle)
122
123- def _translate_io_exception(self, e, path, more_info='',
124- failure_exc=PathError):
125+ @classmethod
126+ def _translate_io_exception(cls, e, path, more_info='',
127+ failure_exc=PathError, operation=None):
128 """Translate a paramiko or IOError into a friendlier exception.
129
130 :param e: The original exception
131@@ -683,9 +685,10 @@
132 no more information.
133 If this parameter is set, it defines the exception
134 to raise in these cases.
135+ :param operation: Operation that failed (needs to check if it's supported)
136 """
137 # paramiko seems to generate detailless errors.
138- self._translate_error(e, path, raise_generic=False)
139+ Transport._translate_error(e, path, raise_generic=False)
140 if getattr(e, 'args', None) is not None:
141 if (e.args == ('No such file or directory',) or
142 e.args == ('No such file',)):
143@@ -703,7 +706,7 @@
144 or getattr(e, 'errno', None) == errno.ENOTEMPTY):
145 raise errors.DirectoryNotEmpty(path, str(e))
146 if e.args == ('Operation unsupported',):
147- raise errors.TransportNotPossible()
148+ raise errors.TransportOperationNotSupported(operation, more_info)
149 mutter('Raising exception with args %s', e.args)
150 if getattr(e, 'errno', None) is not None:
151 mutter('Raising exception with errno %s', e.errno)
152@@ -732,7 +735,7 @@
153 self._remote_path(rel_to))
154 except (IOError, paramiko.SSHException), e:
155 self._translate_io_exception(e, rel_from,
156- ': unable to rename to %r' % (rel_to))
157+ ': unable to rename to %r' % (rel_to), "rename")
158
159 def _rename_and_overwrite(self, abs_from, abs_to):
160 """Do a fancy rename on the remote server.
161@@ -746,7 +749,7 @@
162 unlink_func=sftp.remove)
163 except (IOError, paramiko.SSHException), e:
164 self._translate_io_exception(e, abs_from,
165- ': unable to rename to %r' % (abs_to))
166+ ': unable to rename to %r' % (abs_to), operation="rename")
167
168 def move(self, rel_from, rel_to):
169 """Move the item at rel_from to the location at rel_to"""
170@@ -760,7 +763,8 @@
171 try:
172 self._get_sftp().remove(path)
173 except (IOError, paramiko.SSHException), e:
174- self._translate_io_exception(e, path, ': unable to delete')
175+ self._translate_io_exception(e, path, ': unable to delete',
176+ operation="delete")
177
178 def external_url(self):
179 """See bzrlib.transport.Transport.external_url."""
180@@ -793,7 +797,8 @@
181 try:
182 return self._get_sftp().rmdir(path)
183 except (IOError, paramiko.SSHException), e:
184- self._translate_io_exception(e, path, ': failed to rmdir')
185+ self._translate_io_exception(e, path, ': failed to rmdir',
186+ operation="rmdir")
187
188 def stat(self, relpath):
189 """Return the stat information for a file."""
190@@ -809,7 +814,8 @@
191 try:
192 return self._get_sftp().readlink(path)
193 except (IOError, paramiko.SSHException), e:
194- self._translate_io_exception(e, path, ': unable to readlink')
195+ self._translate_io_exception(e, path, ': unable to readlink',
196+ operation="readlink")
197
198 def symlink(self, source, link_name):
199 """See Transport.symlink."""
200@@ -823,7 +829,8 @@
201 )
202 except (IOError, paramiko.SSHException), e:
203 self._translate_io_exception(e, link_name,
204- ': unable to create symlink to %r' % (source))
205+ ': unable to create symlink to %r' % (source),
206+ operation="symlink")
207
208 def lock_read(self, relpath):
209 """
210@@ -884,7 +891,7 @@
211 return SFTPFile(self._get_sftp(), handle, 'wb', -1)
212 except (paramiko.SSHException, IOError), e:
213 self._translate_io_exception(e, abspath, ': unable to open',
214- failure_exc=FileExists)
215+ failure_exc=FileExists, operation="open")
216
217 def _can_roundtrip_unix_modebits(self):
218 if sys.platform == 'win32':