Fixed unicode error raised when using Jinja2 template with upload_template. #593

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@zyegfryed

Hi,
I've stumbled upon this issue recently. The issue appears when using non-ascii characters on Jinja2 template.
In fact, when trying to submit data through SFTP connection, the resulted StringIO - passed to put function - is converted to bytes (str) which raise the exception.
The documentation says that temporary files are used when dealing with template, but the code was missing. I've added the code to deal with temporary files and a test.
Feel free to merge and to give any feedback on the patch.
Regards,
Sébastien

@fabianbuechler

Subscribing to this. Thanks for fixing.

@jstasiak

Not sure about the temporary file part which seems unnecessary to me, but +1 in general.

@bitprophet
Member

Fabric was still using a temp file before this change - see here: https://github.com/zyegfryed/fabric/blob/9d4a4364d58dc9f92f543811e1925ede73446eff/fabric/sftp.py#L218

(Specifically, doing so was a workaround for a lack of feature in Paramiko - which has since been fixed, & now Fabric just hands off file-like objects directly. Paramiko then consumes them in large read() chunks and sends down the pipe.)

So - not being a Unicode expert - I suspect the issue was specific to using StringIO. Will poke at @zyegfryed's test to see if I can replicate and then figure out whether there's a workaround not involving the local tempfile.

@bitprophet
Member

Yea, the issue (at least, the one I get here) is that it's a Unicode string at the time Paramiko's internal file representation gets ahold of it, which calls str, which then asplodes:

Traceback (most recent call last):
  File "/Users/jforcier/Code/fabric/fabric/operations.py", line 389, in put
    mode, local_is_path)
  File "/Users/jforcier/Code/fabric/fabric/sftp.py", line 227, in put
    rattrs = putter(local_path, remote_path)
  File "/Users/jforcier/Code/paramiko/paramiko/sftp_client.py", line 572, in putfo
    fr.write(data)
  File "/Users/jforcier/Code/paramiko/paramiko/file.py", line 314, in write
    self._write_all(data)
  File "/Users/jforcier/Code/paramiko/paramiko/file.py", line 439, in _write_all
    count = self._write(data)
  File "/Users/jforcier/Code/paramiko/paramiko/sftp_file.py", line 169, in _write
    self._reqs.append(self.sftp._async_request(type(None), CMD_WRITE, self.handle, long(self._realpos), str(data[:chunk])))
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 1: ordinal not in range(128)

In SFTPClient, the local file-like object is simply the StringIO in question, .read() is called on it which yields the unicode object data, of which (naturally) [:chunk] is still unicode.

The original workaround avoids this by writing to, then loading, a "real" file handle, which is sorta like Python 3's bytes type in that it doesn't care about encoding. Good for this particular problem, but crummy in that it requires returning to a filesystem bounce approach.

Will poke a bit more, but I can't really prioritize this much longer. If somebody else can suggest/implement the "right way" to handle this, or at least modify things to be based off current 1.6 (or 1.7 if it's out) release branch + only use the filesystem if the template appears to be Unicode, I'd merge that.

@bitprophet
Member

The only way I see offhand to deal with this is to force encoding, but I think it's also the "right" way (going by my limited understanding of Unicode concerns). Jinja2 appears to always assume/force templates & rendered results to be unicode objects, meaning that we should be able to just do .encode('utf-8') prior to handing off to StringIO.

This should, AFAIK, be safe for non-truly-Unicode-leveraging templaets and thus be backwards compatible.

@bitprophet bitprophet added a commit that referenced this pull request Jul 6, 2013
@bitprophet bitprophet Attempted fix re: #593 3f3d15d
@bitprophet bitprophet added a commit that referenced this pull request Jul 6, 2013
@bitprophet bitprophet Changelog re #593, fixes #593 0246d13
@bitprophet bitprophet added a commit that closed this pull request Jul 6, 2013
@bitprophet bitprophet Changelog re #593, fixes #593 0246d13
@bitprophet bitprophet closed this in 0246d13 Jul 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment