Merge lp:~vila/bzr/106898-put-bytes-raises-TypeError into lp:bzr

[Vincent Ladeuil]

Make all transport put_bytes() raises TypeError when given unicode strings rather than bytes

While looking at https://bugs.launchpad.net/bzr/+bug/1524066 (which is a paramiko issue starting with version 1.16), some other failures were mentioned for per_transport.TransportTests.test_put_file_unicode.

These failures also occur on ubuntu wily and xenial.

From there I came across https://bugs.launchpad.net/bzr/+bug/106898 which was indeed proposing the right fix for test_put_bytes_unicode() and implementing it, I realize that test_put_file_unicode() was just bogus: it's the only place in the code base that try to force a file content to be unicode when we take care everywhere else to treat a file content as bytes.

[Richard Wilbur (richard-wilbur)]

Looks like a valuable realignment of the code to be more consistent. This should also serve us well as we prepare to take on python 3 bytes.

I've suggested a few places where it looks like the parameter documentation could be updated to reflect the changes.

Other than that, +2.

review: Needs Fixing

[Vincent Ladeuil (vila)]

Good catches ! Thanks ! Fixed.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2015-10-23 09:05:09 +0000
+++ bzrlib/tests/per_transport.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011, 2015 Canonical Ltd
+# Copyright (C) 2005-2011, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -520,32 +520,11 @@
         self.assertTransportMode(t, 'dir777', 0777)
 
     def test_put_bytes_unicode(self):
-        # Expect put_bytes to raise AssertionError or UnicodeEncodeError if
-        # given unicode "bytes".  UnicodeEncodeError doesn't really make sense
-        # (we don't want to encode unicode here at all, callers should be
-        # strictly passing bytes to put_bytes), but we allow it for backwards
-        # compatibility.  At some point we should use a specific exception.
-        # See https://bugs.launchpad.net/bzr/+bug/106898.
         t = self.get_transport()
         if t.is_readonly():
             return
         unicode_string = u'\u1234'
-        self.assertRaises(
-            (AssertionError, UnicodeEncodeError),
-            t.put_bytes, 'foo', unicode_string)
-
-    def test_put_file_unicode(self):
-        # Like put_bytes, except with a StringIO.StringIO of a unicode string.
-        # This situation can happen (and has) if code is careless about the type
-        # of "string" they initialise/write to a StringIO with.  We cannot use
-        # cStringIO, because it never returns unicode from read.
-        # Like put_bytes, UnicodeEncodeError isn't quite the right exception to
-        # raise, but we raise it for hysterical raisins.
-        t = self.get_transport()
-        if t.is_readonly():
-            return
-        unicode_file = pyStringIO(u'\u1234')
-        self.assertRaises(UnicodeEncodeError, t.put_file, 'foo', unicode_file)
+        self.assertRaises(TypeError, t.put_bytes, 'foo', unicode_string)
 
     def test_mkdir(self):
         t = self.get_transport()
 
=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py	2015-10-23 09:05:09 +0000
+++ bzrlib/tests/test_smart_transport.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2015 Canonical Ltd
+# Copyright (C) 2006-2014, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 
=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2012-03-13 17:25:29 +0000
+++ bzrlib/transport/__init__.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -873,21 +873,21 @@
             yield self.get(relpath)
             count += 1
 
-    def put_bytes(self, relpath, bytes, mode=None):
+    def put_bytes(self, relpath, raw_bytes, mode=None):
         """Atomically put the supplied bytes into the given location.
 
         :param relpath: The location to put the contents, relative to the
             transport base.
-        :param bytes: A bytestring of data.
+        :param raw_bytes: A bytestring of data.
         :param mode: Create the file with the given mode.
         :return: None
         """
-        if not isinstance(bytes, str):
-            raise AssertionError(
-                'bytes must be a plain string, not %s' % type(bytes))
-        return self.put_file(relpath, StringIO(bytes), mode=mode)
+        if not isinstance(raw_bytes, str):
+            raise TypeError(
+                'raw_bytes must be a plain string, not %s' % type(raw_bytes))
+        return self.put_file(relpath, StringIO(raw_bytes), mode=mode)
 
-    def put_bytes_non_atomic(self, relpath, bytes, mode=None,
+    def put_bytes_non_atomic(self, relpath, raw_bytes, mode=None,
                              create_parent_dir=False,
                              dir_mode=None):
         """Copy the string into the target location.
@@ -896,8 +896,8 @@
         Transport.put_bytes_non_atomic for more information.
 
         :param relpath: The remote location to put the contents.
-        :param bytes:   A string object containing the raw bytes to write into
-                        the target file.
+        :param raw_bytes:   A string object containing the raw bytes to write
+                        into the target file.
         :param mode:    Possible access permissions for new file.
                         None means do not set remote permissions.
         :param create_parent_dir: If we cannot create the target file because
@@ -905,10 +905,10 @@
                         create it, and then try again.
         :param dir_mode: Possible access permissions for new directories.
         """
-        if not isinstance(bytes, str):
-            raise AssertionError(
-                'bytes must be a plain string, not %s' % type(bytes))
-        self.put_file_non_atomic(relpath, StringIO(bytes), mode=mode,
+        if not isinstance(raw_bytes, str):
+            raise TypeError(
+                'raw_bytes must be a plain string, not %s' % type(raw_bytes))
+        self.put_file_non_atomic(relpath, StringIO(raw_bytes), mode=mode,
                                  create_parent_dir=create_parent_dir,
                                  dir_mode=dir_mode)
 
 
=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py	2012-03-13 17:25:29 +0000
+++ bzrlib/transport/local.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2012, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -184,13 +184,15 @@
             fp.close()
         return length
 
-    def put_bytes(self, relpath, bytes, mode=None):
+    def put_bytes(self, relpath, raw_bytes, mode=None):
         """Copy the string into the location.
 
         :param relpath: Location to put the contents, relative to base.
-        :param bytes:   String
+        :param raw_bytes:   String
         """
-
+        if not isinstance(raw_bytes, str):
+            raise TypeError(
+                'raw_bytes must be a plain string, not %s' % type(raw_bytes))
         path = relpath
         try:
             path = self._abspath(relpath)
@@ -200,7 +202,7 @@
             self._translate_error(e, path)
         try:
             if bytes:
-                fp.write(bytes)
+                fp.write(raw_bytes)
             fp.commit()
         finally:
             fp.close()
 
=== modified file 'bzrlib/transport/memory.py'
--- bzrlib/transport/memory.py	2011-12-19 13:23:58 +0000
+++ bzrlib/transport/memory.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2010 Canonical Ltd
+# Copyright (C) 2005-2011, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -148,15 +148,9 @@
         """See Transport.put_file()."""
         _abspath = self._abspath(relpath)
         self._check_parent(_abspath)
-        bytes = f.read()
-        if type(bytes) is not str:
-            # Although not strictly correct, we raise UnicodeEncodeError to be
-            # compatible with other transports.
-            raise UnicodeEncodeError(
-                'undefined', bytes, 0, 1,
-                'put_file must be given a file of bytes, not unicode.')
-        self._files[_abspath] = (bytes, mode)
-        return len(bytes)
+        raw_bytes = f.read()
+        self._files[_abspath] = (raw_bytes, mode)
+        return len(raw_bytes)
 
     def mkdir(self, relpath, mode=None):
         """See Transport.mkdir()."""
 
=== modified file 'bzrlib/transport/readonly.py'
--- bzrlib/transport/readonly.py	2015-10-23 09:05:09 +0000
+++ bzrlib/transport/readonly.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2009, 2010, 2011, 2015 Canonical Ltd
+# Copyright (C) 2006, 2007, 2009, 2010, 2011, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 
=== modified file 'bzrlib/transport/remote.py'
--- bzrlib/transport/remote.py	2012-03-13 17:25:29 +0000
+++ bzrlib/transport/remote.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2010 Canonical Ltd
+# Copyright (C) 2006-2012, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -247,23 +247,18 @@
         transport._file_streams[self.abspath(relpath)] = result
         return result
 
-    def put_bytes(self, relpath, upload_contents, mode=None):
-        # FIXME: upload_file is probably not safe for non-ascii characters -
-        # should probably just pass all parameters as length-delimited
-        # strings?
-        if type(upload_contents) is unicode:
-            # Although not strictly correct, we raise UnicodeEncodeError to be
-            # compatible with other transports.
-            raise UnicodeEncodeError(
-                'undefined', upload_contents, 0, 1,
-                'put_bytes must be given bytes, not unicode.')
-        resp = self._call_with_body_bytes('put',
+    def put_bytes(self, relpath, raw_bytes, mode=None):
+        if not isinstance(raw_bytes, str):
+            raise TypeError(
+                'raw_bytes must be a plain string, not %s' % type(raw_bytes))
+        resp = self._call_with_body_bytes(
+            'put',
             (self._remote_path(relpath), self._serialise_optional_mode(mode)),
-            upload_contents)
+            raw_bytes)
         self._ensure_ok(resp)
-        return len(upload_contents)
+        return len(raw_bytes)
 
-    def put_bytes_non_atomic(self, relpath, bytes, mode=None,
+    def put_bytes_non_atomic(self, relpath, raw_bytes, mode=None,
                              create_parent_dir=False,
                              dir_mode=None):
         """See Transport.put_bytes_non_atomic."""
@@ -276,7 +271,7 @@
             'put_non_atomic',
             (self._remote_path(relpath), self._serialise_optional_mode(mode),
              create_parent_str, self._serialise_optional_mode(dir_mode)),
-            bytes)
+            raw_bytes)
         self._ensure_ok(resp)
 
     def put_file(self, relpath, upload_file, mode=None):
 
=== modified file 'bzrlib/transport/sftp.py'
--- bzrlib/transport/sftp.py	2011-12-19 13:23:58 +0000
+++ bzrlib/transport/sftp.py	2016-01-22 08:02:39 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2010 Canonical Ltd
+# Copyright (C) 2005-2011, 2016 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -593,11 +593,15 @@
                                     create_parent_dir=create_parent_dir,
                                     dir_mode=dir_mode)
 
-    def put_bytes_non_atomic(self, relpath, bytes, mode=None,
+    def put_bytes_non_atomic(self, relpath, raw_bytes, mode=None,
                              create_parent_dir=False,
                              dir_mode=None):
+        if not isinstance(raw_bytes, str):
+            raise TypeError(
+                'raw_bytes must be a plain string, not %s' % type(raw_bytes))
+
         def writer(fout):
-            fout.write(bytes)
+            fout.write(raw_bytes)
         self._put_non_atomic_helper(relpath, writer, mode=mode,
                                     create_parent_dir=create_parent_dir,
                                     dir_mode=dir_mode)
 
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt	2016-01-21 21:44:58 +0000
+++ doc/en/release-notes/bzr-2.7.txt	2016-01-22 08:02:39 +0000
@@ -66,6 +66,10 @@
 .. Major internal changes, unlikely to be visible to users or plugin 
    developers, but interesting for bzr developers.
 
+* Make all transport put_bytes() raises TypeError instead of AssertionError
+  or UnicodeEncodeError when given unicode strings rather than bytes.
+  (Vincent Ladeuil, #106898)
+
 Changed Behaviour
 *****************