fixup! fixup! fixup! fixup! XMLExportImport: more support pickle protocol 3 🚧 ( repair python2 )

bt5/erp5_core_test/TestTemplateItem/portal_components/test.erp5.testXMLPickle.py

 ##############################################################################
-#
+# coding: utf-8
 # Copyright (c) 2008 Nexedi SA and Contributors. All Rights Reserved.
 #                    TAHARA Yusei <yusei@nexedi.com>
 #
@@ -26,16 +26,17 @@
 #
 ##############################################################################
 
+import base64
 import unittest
 import zodbpickle
 import zodbpickle.fastpickle as pickle
 import re
-import xml.sax
-from io import BytesIO, StringIO
-
-from Products.ERP5Type.XMLExportImport import ppml
+from io import BytesIO
+from six import StringIO
+from Products.ERP5Type.XMLExportImport import importXML, ppml
 import six
 
+
 class DummyClass:
   """
   A dummy data class
@@ -50,26 +51,37 @@ class XMLPickleTestCase(unittest.TestCase):
   def dump_to_xml(self, obj):
     pickled_string = pickle.dumps(obj, protocol=self._pickle_protocol)
     f = BytesIO(pickled_string)
-    return str(ppml.ToXMLUnpickler(f).load())
-
-  def load_from_xml(self, xml_string):
-    output = StringIO()
-    F=ppml.xmlPickler()
-    F.file = output
-    F.binary = 1 # XML pickle actually only supports the case of binary = 1
-
-    content_handler = xml.sax.handler.ContentHandler()
-    content_handler.startElement = F.unknown_starttag
-    content_handler.endElement = F.unknown_endtag
-    content_handler.characters = F.handle_data
-    xml.sax.parseString(xml_string, content_handler)
-
-    reconstructed_pickled_data = F._stack[0][0]
-    return pickle.loads(reconstructed_pickled_data)
+    xml = ppml.ToXMLUnpickler(f).load().__str__()
+    if six.PY2 and isinstance(xml, six.text_type):
+      xml = xml.encode('utf-8')
+    return xml
+
+  def load_from_xml(self, xml_string, persistent_load=None):
+    assertEqual = self.assertEqual
+    class DummyJar:
+      loaded = None
+      """follow interface expected by importXML"""
+      def importFile(self, file, clue):
+        assertEqual(clue, 'ignored')
+        assertEqual(file.read(4), b'ZEXP')
+        unpickler = pickle.Unpickler(file)
+        if persistent_load:
+          unpickler.persistent_load = persistent_load
+        self.loaded = unpickler.load()
+
+    jar = DummyJar()
+    xml_string = '<?xml version="1.0"?>\n<ZopeData>%s</ZopeData>' % xml_string
+    importXML(jar, StringIO(xml_string), clue='ignored')
+    return jar.loaded
 
   def dump_and_load(self, obj):
     return self.load_from_xml(self.dump_to_xml(obj))
 
+  def check_and_load(self, v):
+    reconstructed = self.dump_and_load(v)
+    self.assertEqual(reconstructed, v)
+    self.assertIs(type(reconstructed), type(v))
+
 
 class TestXMLPickle(XMLPickleTestCase):
 
@@ -101,76 +113,95 @@ class TestXMLPickle(XMLPickleTestCase):
     self.assertIs(self.dump_and_load(False), False)
 
   def test_int(self):
-    def check_int(v):
-      reconstructed = self.dump_and_load(v)
-      self.assertEqual(reconstructed, v)
-      self.assertIs(type(reconstructed), int)
-    check_int(-0)
-    check_int(1)
-    check_int(-1)
-    check_int(0xff)
-    check_int(0xff1)
-    check_int(0xffff)
-    check_int(0xffff1)
+    self.check_and_load(-0)
+    self.check_and_load(1)
+    self.check_and_load(-1)
+    self.check_and_load(0xff)
+    self.check_and_load(0xff1)
+    self.check_and_load(0xffff)
+    self.check_and_load(2**128)
+    # long4
+    # https://github.com/python/cpython/blob/4d4a6f1b/Lib/test/pickletester.py#L2049-L2050
+    self.check_and_load(12345678910111213141516178920 << (256*8))
+
+
+  if six.PY2:
+    def test_long(self):
+      self.check_and_load(long(-0))
+      self.check_and_load(long(1))
+      self.check_and_load(long(-1))
+      self.check_and_load(long(0xff))
+      self.check_and_load(long(0xff1))
+      self.check_and_load(long(0xffff))
+      self.check_and_load(long(2**128))
+      self.check_and_load(12345678910111213141516178920 << (256*8))
 
   def test_float(self):
-    def check_float(v):
-      reconstructed = self.dump_and_load(v)
-      self.assertEqual(reconstructed, v)
-      self.assertIs(type(reconstructed), float)
-    check_float(-0.0)
-    check_float(1.0)
-    check_float(-1.0)
-    check_float(.1 + .2)
+    self.check_and_load(-0.0)
+    self.check_and_load(1.0)
+    self.check_and_load(-1.0)
+    self.check_and_load(.33)
 
   def test_None(self):
     self.assertIs(
       self.dump_and_load(None), None)
 
   def test_bytes(self):
-    self.assertEqual(self.dump_and_load(b"bytes"), b"bytes")
-    self.assertEqual(self.dump_and_load(b"long bytes" * 100), b"long bytes" * 100)
-    self.assertEqual(
-      self.dump_and_load(zodbpickle.binary(b"bytes")),
-      zodbpickle.binary(b"bytes"))
-    self.assertIs(type(self.dump_and_load(zodbpickle.binary(b"bytes"))), zodbpickle.binary)
+    self.check_and_load(b"bytes")
+    self.check_and_load(b"long bytes" * 100)
+    self.check_and_load(zodbpickle.binary(b"bytes"))
+    self.check_and_load(zodbpickle.binary(b""))
 
-  def test_unicode(self):
+  def test_unicode(self):  # BBB PY2
     self.assertIs(type(self.dump_and_load(u"OK")), six.text_type)
-    self.assertEqual(self.dump_and_load(u"short"), u"short")
-    self.assertEqual(self.dump_and_load(u"unicode 👍"), u"unicode 👍")
-    self.assertEqual(self.dump_and_load(u"long" * 100), u"long" * 100)
-    self.assertEqual(self.dump_and_load(u"long…" * 100), u"long…" * 100)
-    self.assertEqual(self.dump_and_load(u">"), u">")
-    self.assertEqual(self.dump_and_load(u"a\nb"), u"a\nb")
+    self.check_and_load(u"short")
+    self.check_and_load(u"unicode 👍")
+    self.check_and_load(u"long" * 100)
+    self.check_and_load(u"long…" * 100)
+    self.check_and_load(u">")
+    self.check_and_load(u"a\nb")
+    self.check_and_load(u" with spaces ")
+    self.check_and_load(u"\twith\ttabs\t")
+    self.check_and_load(u"")
+
+  def test_str(self):
+    self.assertIs(type(self.dump_and_load("OK")), str)
+    self.check_and_load("short")
+    self.check_and_load("unicode 👍")
+    self.check_and_load("long" * 100)
+    self.check_and_load("long…" * 100)
+    self.check_and_load(">")
+    self.check_and_load("a\nb")
+    self.check_and_load(" with spaces ")
+    self.check_and_load("\twith\ttabs\t")
+    self.check_and_load("")
 
   def test_dict(self):
-    self.assertEqual(
-      self.dump_and_load({'a': 1, 'b': 2}), {'a': 1, 'b': 2})
+    self.check_and_load({'a': 1, 'b': 2})
+    self.check_and_load({'hé': 'ho'})
+    self.check_and_load(dict.fromkeys(range(3000)))
 
   def test_tuple(self):
-    self.assertEqual(
-      self.dump_and_load((1, )), (1, ))
-    self.assertEqual(
-      self.dump_and_load((1, 'two')), (1, 'two'))
-    self.assertEqual(
-      self.dump_and_load((1, 'two', 3.0)), (1, 'two', 3.0))
-    self.assertEqual(
-      self.dump_and_load(tuple([1] * 1000)), tuple([1] * 1000))
-    self.assertEqual(
-      self.dump_and_load(()), ())
+    self.check_and_load((1, ))
+    self.check_and_load((1, 'two'))
+    self.check_and_load((1, 'two', 3.0))
+    self.check_and_load(tuple([1] * 1000))
+    self.check_and_load(())
+    self.check_and_load(('hé',))
+    self.check_and_load(('hé', 'hé'))
+    self.check_and_load(('hé', 'hé', 'hé'))
+    self.check_and_load(('hé', 'hé', 'hé', 'hé'))
 
   def test_list(self):
-    self.assertEqual(
-      self.dump_and_load([1]), [1])
-    self.assertEqual(
-      self.dump_and_load([]), [])
-    self.assertEqual(
-      self.dump_and_load([1] * 1000), [1] * 1000)
+    self.check_and_load([1])
+    self.check_and_load([])
+    self.check_and_load([1] * 1000)
+    self.check_and_load(['hé'])
 
   def test_set(self):
-    self.assertEqual(
-      self.dump_and_load(set('abc')), set('abc'))
+    self.check_and_load(set('abc'))
+    self.check_and_load(set('hé'))
+    self.check_and_load(set([]))
 
   def test_reference(self):
     ref = []
@@ -178,6 +209,14 @@ class TestXMLPickle(XMLPickleTestCase):
     self.assertEqual(reconstructed, [ref, ref, ref])
     self.assertIs(reconstructed[0], reconstructed[1])
 
+  def test_reference_long(self):
+    # same as reference (which is using BINPUT/BINGET but with large data
+    # to use LONG_BINPUT/LONG_BINGET)
+    ref = [list() for _ in range(256)]
+    reconstructed = self.dump_and_load([ref, ref, ref])
+    self.assertEqual(reconstructed, [ref, ref, ref])
+    self.assertIs(reconstructed[0], reconstructed[1])
+
 
 class TestXMLPickleStringEncoding(XMLPickleTestCase):
   def test_string_base64(self):
@@ -214,19 +253,39 @@ class TestXMLPickleStringEncoding(XMLPickleTestCase):
 class TestXMLPickleStringHeuristics(XMLPickleTestCase):
   """Heuristics to map python2 str to unicode or bytes in business templates.
   """
-  def test_oid_base64(self):
-    # if it looks like an oid, it's bytes
-    self.assertEqual(
-      self.load_from_xml("""
-        <pickle><string encoding="base64">AAAAAAAAAAE=</string></pickle>
-      """),
-      b"\x00\x00\x00\x00\x00\x00\x00\x01")
 
   def test_bytes_base64(self):
-    # if it does not decode as utf-8 it's bytes
+    # if it does not decode as utf-8, it's bytes
     self.assertEqual(
       self.load_from_xml("""
         <pickle><string encoding="base64">/wA=</string></pickle>
       """),
       b"\xFF\x00")
 
+  def test_long_bytes_base64(self):
+    # if it does not decode as utf-8, it's bytes
+    long_bytes = b"\xFF\x00" * 256
+    self.assertEqual(
+      self.load_from_xml("""
+        <pickle><string encoding="base64">%s</string></pickle>
+      """ % base64.b64encode(long_bytes).decode()),
+      long_bytes)
+
+  def test_string_persistent_id_base64(self):
+    # persistent ids are loaded as bytes
+    persistent_ids = []
+    def persistent_load(oid):
+      persistent_ids.append(oid)
+    self.assertEqual(
+      self.load_from_xml("""
+      <pickle>
+        <persistent>
+          <string encoding="base64">AAAAAAAAAAE=</string>
+        </persistent>
+      </pickle>
+      """,
+      persistent_load=persistent_load),
+      None)
+    self.assertEqual(
+      persistent_ids,
+      [b'\x00\x00\x00\x00\x00\x00\x00\x01'])

product/ERP5Type/XMLExportImport/__init__.py

@@ -363,12 +363,6 @@ def exportXML(jar, oid, file=None):
         p = getReorderedPickle(oid)
         write(XMLrecord(oid, len(p), p, id_mapping))
     write('</ZopeData>\n')
-    if 0:
-      try:
-        print(file.getvalue())
-      except AttributeError:
-        pass
-      import pdb; pdb.set_trace()
     return file
 
 class zopedata:
@@ -421,7 +415,6 @@ def importXML(jar, file, clue=''):
         F.end_handlers['record'] = save_record
         F.end_handlers['ZopeData'] = save_zopedata
         F.start_handlers['ZopeData'] = start_zopedata
-        F.binary=1
         F.file=outfile
         # <patch>
         # Our BTs XML files don't declare encoding but have accented chars in them

product/ERP5Type/XMLExportImport/ppml.py

@@ -24,8 +24,10 @@ import struct
 import six
 if six.PY2:
   from base64 import encodestring as base64_encodebytes, decodestring as base64_decodebytes
+  from zodbpickle.pickle_2 import decode_long
 else:
   from base64 import encodebytes as base64_encodebytes, decodebytes as base64_decodebytes
+  from zodbpickle.pickle_3 import decode_long
 import re
 from marshal import loads as mloads
 from .xyap import NoBlanks
@@ -201,12 +203,14 @@ class String(Scalar):
             self.mapping.setImmutable(self.id, Immutable(value = result))
         return '%s%s\n' % (' '*indent, result)
 
+
 class Unicode(String):
     def tag_name(self):
         if six.PY3:
             return 'string'
         return super(Unicode, self).tag_name()
 
+
 class Bytes(String):
     pass
 
@@ -226,7 +230,7 @@ class Wrapper(object):
         v=self._v
         i=' '*indent
         if isinstance(v, Scalar):
-            return '%s<%s%s> %s </%s>\n' % (i, name, id, str(v)[:-1], name)
+            return '%s<%s%s> %s </%s>\n' % (i, name, id, v.__str__()[:-1], name)
         else:
             v=v.__str__(indent+2)
             return '%s<%s%s>\n%s%s</%s>\n' % (i, name, id, v, i, name)
@@ -479,6 +483,18 @@ class MinimalMapping(IdentityMapping):
     def __str__(self, a):
       return "Error here"
 
+
+class UnsupportedOpCode(AssertionError):
+    """Error when encountering an opcode that is not supposed to be used
+    by this implementation.
+    """
+
+def unsupported_opcode(opcode):
+    def handler(self):
+        raise UnsupportedOpCode(opcode)
+    return handler
+
+
 class ToXMLUnpickler(Unpickler):
     def load(self, id_mapping=None):
       if id_mapping is None:
@@ -493,13 +509,6 @@ class ToXMLUnpickler(Unpickler):
     def persistent_load(self, v):
         return Persistent(v, self.id_mapping)
 
-    def load_persid(self):
-        pid = self.readline()[:-1]
-        self.append(self.persistent_load(String(pid, self.id_mapping)))
-    if six.PY2:
-        dispatch[PERSID] = load_persid
-    dispatch[PERSID[0]] = load_persid
-
     def load_binpersid(self):
         pid = self.stack.pop()
         self.append(self.persistent_load(pid))
@@ -513,12 +522,6 @@ class ToXMLUnpickler(Unpickler):
         dispatch[NONE] = load_none
     dispatch[NONE[0]] = load_none
 
-    def load_int(self):
-        self.append(Int(int(self.readline()[:-1]), self.id_mapping))
-    if six.PY2:
-        dispatch[INT] = load_int
-    dispatch[INT[0]] = load_int
-
     def load_binint(self):
         self.append(Int(mloads(b'i' + self.read(4)), self.id_mapping))
     if six.PY2:
@@ -537,17 +540,24 @@ class ToXMLUnpickler(Unpickler):
         dispatch[BININT2] = load_binint2
     dispatch[BININT2[0]] = load_binint2
 
-    def load_long(self):
-        self.append(Long(long_(self.readline()[:-1], 0), self.id_mapping))
+    def load_long1(self):
+        n = ord(self.read(1))
+        data = self.read(n)
+        self.append(Long(decode_long(data), self.id_mapping))
     if six.PY2:
-        dispatch[LONG] = load_long
-    dispatch[LONG[0]] = load_long
-
-    def load_float(self):
-        self.append(Float(float(self.readline()[:-1]), self.id_mapping))
+        dispatch[LONG1] = load_long1
+    dispatch[LONG1[0]] = load_long1
+
+    def load_long4(self):
+        n = mloads(b'i' + self.read(4))
+        if n < 0:
+            # Corrupt or hostile pickle -- we never write one like this
+            raise UnpicklingError("LONG pickle has negative byte count");
+        data = self.read(n)
+        self.append(Long(decode_long(data), self.id_mapping))
     if six.PY2:
-        dispatch[FLOAT] = load_float
-    dispatch[FLOAT[0]] = load_float
+        dispatch[LONG4] = load_long4
+    dispatch[LONG4[0]] = load_long4
 
     def load_true(self):
         self.append(Bool(True, self.id_mapping))
@@ -567,13 +577,6 @@ class ToXMLUnpickler(Unpickler):
         dispatch[BINFLOAT] = load_binfloat
     dispatch[BINFLOAT[0]] = load_binfloat
 
-    def load_string(self):
-        self.append(String(ast.literal_eval(self.readline()[:-1],
-                                {'__builtins__': {}}), self.id_mapping)) # Let's be careful
-    if six.PY2:
-        dispatch[STRING] = load_string
-    dispatch[STRING[0]] = load_string
-
     def load_binstring(self):
         len = mloads(b'i' + self.read(4))
         self.append(String(self.read(len), self.id_mapping))
@@ -581,14 +584,6 @@ class ToXMLUnpickler(Unpickler):
         dispatch[BINSTRING] = load_binstring
     dispatch[BINSTRING[0]] = load_binstring
 
-    def load_unicode(self):
-        line = self.readline()
-        self.append(Unicode(six.text_type(ast.literal_eval(line[:-1],
-                                         {'__builtins__': {}})), self.id_mapping)) # Let's be careful
-    if six.PY2:
-        dispatch[UNICODE] = load_unicode
-    dispatch[UNICODE[0]] = load_unicode
-
     def load_binunicode(self):
         len = mloads(b'i' + self.read(4))
         self.append(Unicode(six.text_type(self.read(len), 'utf-8'), self.id_mapping))
@@ -619,8 +614,6 @@ class ToXMLUnpickler(Unpickler):
 
     def load_tuple(self):
         k = self.marker()
-        #LOG('load_tuple, k',0,k)
-        #LOG('load_tuple, stack[k+1:]',0,self.stack[k+1:])
         self.stack[k:] = [Tuple(self.id_mapping, v=self.stack[k+1:])]
     if six.PY2:
         dispatch[TUPLE] = load_tuple
@@ -742,12 +735,6 @@ class ToXMLUnpickler(Unpickler):
 
     idprefix=''
 
-    def load_get(self):
-        self.append(Get(self.idprefix+self.readline()[:-1], self.id_mapping))
-    if six.PY2:
-        dispatch[GET] = load_get
-    dispatch[GET[0]] = load_get
-
     def load_binget(self):
         i = mloads(b'i' + self.read(1) + b'\000\000\000')
         self.append(Get(self.idprefix+repr(i), self.id_mapping))
@@ -782,6 +769,11 @@ class ToXMLUnpickler(Unpickler):
         dispatch[LONG_BINPUT] = load_long_binput
     dispatch[LONG_BINPUT[0]] = load_long_binput
 
+    for code in PERSID, INT, LONG, FLOAT, STRING, UNICODE, GET, PUT:
+        if six.PY2:
+            dispatch[code] = unsupported_opcode(code)
+        dispatch[code[0]] = unsupported_opcode(code)
+
     class LogCall:
       def __init__(self, func):
         self.func = func
@@ -809,23 +801,19 @@ def start_pickle(self, tag, attrs):
     return [tag, attrs]
 
 def save_int(self, tag, data):
-    if self.binary:
-        v = int(name(self, tag, data))
-        if v >= 0:
-            if v <= 0xff:
-                return BININT1 + six.int2byte(v)
-            if v <= 0xffff:
-                return BININT2 + b'%c%c' % (v & 0xff, v >> 8)
-        hb = v >> 31
-        if hb == 0 or hb == -1:
-            return BININT + struct.pack('<i', v)
+    v = int(name(self, tag, data))
+    if v >= 0:
+        if v <= 0xff:
+            return BININT1 + six.int2byte(v)
+        if v <= 0xffff:
+            return BININT2 + b'%c%c' % (v & 0xff, v >> 8)
+    hb = v >> 31
+    if hb == 0 or hb == -1:
+        return BININT + struct.pack('<i', v)
     return INT + name(self, tag, data) + b'\n'
 
 def save_float(self, tag, data):
-    if self.binary:
-        return BINFLOAT + struct.pack('>d', float(name(self, tag, data)))
-    else:
-        return FLOAT + name(self, tag, data) + b'\n'
+    return BINFLOAT + struct.pack('>d', float(name(self, tag, data)))
 
 def save_put(self, v, attrs):
     id = attrs.get('id', '')
@@ -835,14 +823,11 @@ def save_put(self, v, attrs):
             id = id[prefix + 1:]
         elif id[0] == 'i':
             id = id[1:]
-        if self.binary:
-            id = int(id)
-            if id < 256:
-                id = BINPUT + six.int2byte(id)
-            else:
-                id = LONG_BINPUT + struct.pack('<i', id)
+        id = int(id)
+        if id < 256:
+            id = BINPUT + six.int2byte(id)
         else:
-            id = PUT + repr(id).encode() + b'\n'
+            id = LONG_BINPUT + struct.pack('<i', id)
         return v + id
     return v
 
@@ -852,56 +837,52 @@ def save_string(self, tag, data):
     encoding = a.get('encoding', 'repr') # JPS: repr is default encoding
     if encoding != '':
         v = unconvert(encoding, v)
-    if self.binary:
-        l = len(v)
-        if l < 256:
-            if encoding == 'base64':
-              # TODO: zope4py3 (all this is unfinished)
-              # We can be here for two reasons:
-              # - the input was a string with \n or similar control characters
-              # that are not allowed in XML, so the str was exported as base64.
-              # - the input was a _p_oid exported from python2, in that case
-              # we want to get a zodbpickle.binary back
-              # XXX all this seems a bad idea, we need more context if we want
-              # to have such heuristics
-              if len(v) == 8:
-                  # looks like a _p_oid, assume it is a persistent_id -> bytes
-                  op = SHORT_BINBYTES
-              else:
-                  # if it's a valid UTF-8 string -> str
-                  try:
-                      v.decode('utf-8')
-                      # XXX maybe check with repr_re ?
-                      op = BINUNICODE if six.PY3 else BINSTRING
-                      v = op + struct.pack('<i', l) + v
-                      return save_put(self, v, a)
-                  except UnicodeDecodeError:
-                      # not valid utf-8 -> bytes
-                      op = SHORT_BINBYTES
+    l = len(v)
+    if l < 256:
+        if encoding == 'base64':
+            # We can be here for two reasons:
+            # - the input was a string with \n or similar control characters
+            # that are not allowed in XML, so the str was exported as base64.
+            # - the input was a persistent id exported from python2, in that case
+            # we want to get a zodbpickle.binary back
+            if len(v) == 8 and self._stack[-1][0] in ('persistent', ):
+                # looks like a persistent id, assume it is a persistent_id -> bytes
+                op = SHORT_BINBYTES
             else:
-              # XXX this branch seems wrong
-              op = SHORT_BINSTRING
-              try:
-                  v.decode('ascii')
-                  # XXX zope4py3 we could also create an unpickler with encoding utf-8 ?
-              except UnicodeDecodeError:
-                  op = BINUNICODE if six.PY3 else BINSTRING
-                  v = op + struct.pack('<i', l) + v
-                  return save_put(self, v, a)
-
-            v = op + six.int2byte(l) + v
+                # if it's a valid UTF-8 string -> str
+                try:
+                    v.decode('utf-8')
+                    # XXX maybe check with repr_re ?
+                    op = BINUNICODE if six.PY3 else BINSTRING
+                    v = op + struct.pack('<i', l) + v
+                    return save_put(self, v, a)
+                except UnicodeDecodeError:
+                    # not valid utf-8 -> bytes
+                    op = SHORT_BINBYTES
         else:
-            # TODO: zope4py3 see assumption above for SHORT_BINBYTES / SHORT_BINSTRING
-            # TODO no! check this more ...
-            # op = BINSTRING
-            if encoding == 'base64':
-                op = BINBYTES
-            else:
-                op = BINSTRING if six.PY2 else BINUNICODE
-
-            v = op + struct.pack('<i', l) + v
+            op = SHORT_BINSTRING
+            try:
+                v.decode('ascii')
+            except UnicodeDecodeError:
+                op = BINUNICODE if six.PY3 else BINSTRING
+                v = op + struct.pack('<i', l) + v
+                return save_put(self, v, a)
+
+        v = op + six.int2byte(l) + v
     else:
-        v = STRING + repr(v).encode() + b'\n'
+        if encoding == 'base64':
+            op = BINBYTES
+            # if it's a valid UTF-8 string -> str
+            try:
+                v.decode('utf-8')
+                op = BINUNICODE if six.PY3 else BINSTRING
+            except UnicodeDecodeError:
+                # not valid utf-8 -> bytes
+                pass
+        else:
+            op = BINSTRING if six.PY2 else BINUNICODE
+
+        v = op + struct.pack('<i', l) + v
     return save_put(self, v, a)
 
 def save_bytes(self, tag, data):
@@ -910,22 +891,17 @@ def save_bytes(self, tag, data):
     encoding = a.get('encoding', 'repr')
     if encoding:
         v = unconvert(encoding, v)
-    if self.binary:
-        l = len(v)
-        if l < 256:
-            op = SHORT_BINBYTES
-            v = op + six.int2byte(l) + v
-        else:
-            op = BINBYTES
-            v = op + struct.pack('<i', l) + v
+    l = len(v)
+    if l < 256:
+        op = SHORT_BINBYTES
+        v = op + six.int2byte(l) + v
     else:
-        # XXX used ??? seems wrong
-        v = BYTES + repr(v) + '\n'
+        op = BINBYTES
+        v = op + struct.pack('<i', l) + v
     return save_put(self, v, a)
 
 
 def save_unicode(self, tag, data):
-    binary=self.binary
     v=b''
     a=data[1]
     if len(data)>2:
@@ -934,11 +910,9 @@ def save_unicode(self, tag, data):
     encoding=a.get('encoding','repr') # JPS: repr is default encoding
     if encoding != '':
         v=unconvert(encoding,v)
-    if binary:
-        l=len(v)
-        s=mdumps(l)[1:]
-        v=BINUNICODE+s+v
-    else: v=UNICODE+"'"+v+"'\012"
+    l=len(v)
+    s=mdumps(l)[1:]
+    v=BINUNICODE+s+v
     return save_put(self, v, a)
 
 def save_tuple(self, tag, data):
@@ -949,26 +923,16 @@ def save_tuple(self, tag, data):
 
 def save_list(self, tag, data):
     L = data[2:]
-    if self.binary:
-        v = save_put(self, EMPTY_LIST, data[1])
-        if L:
-            v = v + MARK + b''.join(L) + APPENDS
-    else:
-        v = save_put(self, MARK + LIST, data[1])
-        if L:
-            v = APPEND.join(L) + APPEND
+    v = save_put(self, EMPTY_LIST, data[1])
+    if L:
+        v = v + MARK + b''.join(L) + APPENDS
     return v
 
 def save_dict(self, tag, data):
     D = data[2:]
-    if self.binary:
-        v = save_put(self, EMPTY_DICT, data[1])
-        if D:
-            v = v + MARK + b''.join(D) + SETITEMS
-    else:
-        v = save_put(self, MARK + DICT, data[1])
-        if D:
-            v = v + SETITEM.join(D) + SETITEM
+    v = save_put(self, EMPTY_DICT, data[1])
+    if D:
+        v = v + MARK + b''.join(D) + SETITEMS
     return v
 
 def save_reference(self, tag, data):
@@ -977,14 +941,11 @@ def save_reference(self, tag, data):
     prefix = id.rfind('.')
     if prefix >= 0:
         id = id[prefix + 1:]
-    if self.binary:
-        id = int(id)
-        if id < 256:
-            return BINGET + six.int2byte(id)
-        else:
-            return LONG_BINGET + struct.pack('<i', id)
+    id = int(id)
+    if id < 256:
+        return BINGET + six.int2byte(id)
     else:
-        return GET + repr(id).encode() + b'\n'
+        return LONG_BINGET + struct.pack('<i', id)
 
 def save_object(self, tag, data):
     if len(data)==5:
@@ -1012,10 +973,7 @@ def save_global(self, tag, data):
 
 def save_persis(self, tag, data):
     v = data[2]
-    if self.binary:
-        return v + BINPERSID
-    else:
-        return PERSID + v
+    return v + BINPERSID
 
 def save_pickle_start(self, tag, attrs):
     return [tag, attrs]