IdTool: handle group_id on python3

group_id is used as key of OOBtree and as documented, it's not
possible to mix keys that can not be compared, so we can not have a mix
of string and bytes, for consistency with other BTrees, such as the
ones used for OFS.
group_id is also used in a SQL column which is BINARY, this is
problematic on py3 because the selected values will be returned as bytes,
but we expect str here. Because we don't want to run a data migration,
we adjust the select methods to convert to str while selecting.

Since years there was a warning that id_group must be a string, now we
make it a bit stricter, we also enforce that the id_group is valid UTF-8.

A few more tests and assertions were also added.

Reviewed-on: nexedi/erp5!1980

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

@@ -28,7 +28,9 @@
 #
 ##############################################################################
 
+import six
 import unittest
+import warnings
 
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
 from Products.ERP5Type.tests.utils import createZODBPythonScript
@@ -146,6 +148,56 @@ class TestIdTool(ERP5TypeTestCase):
     self.assertEqual(21, self.id_tool.generateNewId(id_generator=id_generator,
                                       id_group='d02', default=3))
 
+    # no collations, hé and he are different
+    self.assertEqual(0, self.id_tool.generateNewId(id_generator=id_generator, id_group='hé'))
+    self.assertEqual(1, self.id_tool.generateNewId(id_generator=id_generator, id_group='hé'))
+    self.assertEqual(0, self.id_tool.generateNewId(id_generator=id_generator, id_group='he'))
+
+    # generateNewId expect str, but convert id_group when passed a wrong type
+    with warnings.catch_warnings(record=True) as recorded:
+      warnings.simplefilter("always")
+      self.assertEqual(
+        self.id_tool.generateNewId(
+          id_generator=id_generator,
+          id_group=('d', 1),
+        ), 0)
+    self.assertEqual(
+      [(type(w.message), str(w.message)) for w in recorded],
+      [(DeprecationWarning, 'id_group must be a string, other types are deprecated.')],
+    )
+    # conversion is done using `repr`
+    self.assertEqual(
+      self.id_tool.generateNewId(
+        id_generator=id_generator,
+        id_group=repr(('d', 1)),
+      ), 1)
+
+    # on python3, it understands bytes and converts to string
+    self.assertEqual(
+      self.id_tool.generateNewId(
+        id_generator=id_generator,
+        id_group='bytes',
+      ), 0)
+    with warnings.catch_warnings(record=True) as recorded:
+      warnings.simplefilter("always")
+      self.assertEqual(
+        self.id_tool.generateNewId(
+          id_generator=id_generator,
+          id_group=b'bytes',
+        ), 1)
+      if six.PY3:
+        self.assertEqual(
+          [(type(w.message), str(w.message)) for w in recorded],
+          [(BytesWarning, 'id_group must be a string, not bytes.')],
+        )
+    if six.PY2:
+      self.assertRaises(
+        ValueError,
+        self.id_tool.generateNewId,
+        id_generator=id_generator,
+        id_group=u'hé'.encode('latin1'),
+      )
+
   def test_02a_generateNewIdWithZODBGenerator(self):
     """
       Check the generateNewId with a zodb id generator
@@ -234,6 +286,59 @@ class TestIdTool(ERP5TypeTestCase):
                                       id_generator=id_generator,
                                       id_group='d03', default=3, id_count=2))
 
+    # no collations, hé and he are different
+    self.assertEqual([0], self.id_tool.generateNewIdList(id_generator=id_generator, id_group='hé'))
+    self.assertEqual([1], self.id_tool.generateNewIdList(id_generator=id_generator, id_group='hé'))
+    self.assertEqual([0], self.id_tool.generateNewIdList(id_generator=id_generator, id_group='he'))
+
+    # generateNewIdList expect str, but convert id_group when passed a wrong type
+    with warnings.catch_warnings(record=True) as recorded:
+      warnings.simplefilter("always")
+      self.assertEqual(
+        self.id_tool.generateNewIdList(
+          id_generator=id_generator,
+          id_group=('d', 1),
+          id_count=1,), [0])
+    self.assertEqual(
+      [(type(w.message), str(w.message)) for w in recorded],
+      [(DeprecationWarning, 'id_group must be a string, other types are deprecated.')],
+    )
+    # conversion is done using `repr`
+    self.assertEqual(
+      self.id_tool.generateNewIdList(
+        id_generator=id_generator,
+        id_group=repr(('d', 1)),
+      ), [1])
+
+    # on python3, it understands bytes and converts to string
+    self.assertEqual(
+      self.id_tool.generateNewIdList(
+        id_generator=id_generator,
+        id_group='bytes',
+        id_count=1,
+        ), [0])
+    with warnings.catch_warnings(record=True) as recorded:
+      warnings.simplefilter("always")
+      self.assertEqual(
+        self.id_tool.generateNewIdList(
+          id_generator=id_generator,
+          id_group=b'bytes',
+          id_count=1,
+        ), [1])
+      if six.PY3:
+        self.assertEqual(
+          [(type(w.message), str(w.message)) for w in recorded],
+          [(BytesWarning, 'id_group must be a string, not bytes.')],
+        )
+    if six.PY2:
+      self.assertRaises(
+        ValueError,
+        self.id_tool.generateNewIdList,
+        id_generator=id_generator,
+        id_group=u'hé'.encode('latin1'),
+        id_count=1,
+      )
+
   def test_03a_generateNewIdListWithZODBGenerator(self):
     """
       Check the generateNewIdList with zodb generator

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

@@ -60,7 +60,7 @@ class TestIdToolUpgrade(ERP5TypeTestCase):
     self.tic()
 
   def beforeTearDown(self):
-    self.portal.portal_caches.clearAllCache()
+    self.portal.portal_caches.clearAllCache()  # for IdTool._getLatestIdGenerator
     self.id_tool.clearGenerator(all=True)
     self.tic()
 
@@ -298,6 +298,35 @@ class TestIdToolUpgrade(ERP5TypeTestCase):
     id_generator.clearGenerator() # clear stored data
     self._checkDataStructureMigration(id_generator)
 
+  def test_portal_ids_table_id_group_column_binary(self):
+    """portal_ids.id_group is now created as VARCHAR,
+    but it use to be binary. There is no data migration, the
+    SQL method has been adjusted to cast during select.
+    This checks that id generator works well when the column
+    is VARBINARY, like it's the case for old instances.
+    """
+    self.assertEqual(
+      self.sql_generator.generateNewId(id_group=self.id()),
+      0)
+    exported = self.sql_generator.exportGeneratorIdDict()
+    self.tic()
+    self.portal.portal_ids.IdTool_zCommit()
+
+    self.portal.erp5_sql_connection.manage_test(
+      'ALTER TABLE portal_ids MODIFY COLUMN id_group VARBINARY(255)'
+    )
+    self.tic()
+    self.assertEqual(
+      self.sql_generator.generateNewId(id_group=self.id()),
+      1)
+    self.tic()
+    self.sql_generator.importGeneratorIdDict(exported, clear=True)
+    self.tic()
+    self.assertEqual(
+      self.sql_generator.generateNewId(id_group=self.id()),
+      1)
+
+
 def test_suite():
   suite = unittest.TestSuite()
   suite.addTest(unittest.makeSuite(TestIdToolUpgrade))

product/ERP5/Document/SQLNonContinuousIncreasingIdGenerator.py

@@ -73,6 +73,8 @@ class SQLNonContinuousIncreasingIdGenerator(IdGenerator):
     # Check the arguments
     if id_group in (None, 'None'):
       raise ValueError('%r is not a valid group Id.' % id_group)
+    if not isinstance(id_group, str):
+      raise TypeError('id_group must be str')
     if default is None:
       default = 0
 
@@ -134,6 +136,7 @@ class SQLNonContinuousIncreasingIdGenerator(IdGenerator):
     # the last id stored in the sql table
     for line in self._getValueListFromTable():
       id_group = line['id_group']
+      assert isinstance(id_group, str)
       last_id = line['last_id']
       if id_group in self.last_max_id_dict and \
         self.last_max_id_dict[id_group].value > last_id:
@@ -197,6 +200,7 @@ class SQLNonContinuousIncreasingIdGenerator(IdGenerator):
             getattr(portal_ids, 'dict_length_ids', None) is None):
       dump_dict = portal_ids.dict_length_ids
       for id_group, last_id in dump_dict.items():
+        assert isinstance(id_group, str)
         last_insert_id = get_last_id_method(id_group=id_group)
         last_id = int(last_id.value)
         if len(last_insert_id) != 0:

product/ERP5/Document/ZODBContinuousIncreasingIdGenerator.py

@@ -59,6 +59,8 @@ class ZODBContinuousIncreasingIdGenerator(IdGenerator):
     """
     if id_group in (None, 'None'):
       raise ValueError('%r is not a valid group Id.' % id_group)
+    if not isinstance(id_group, str):
+      raise TypeError('id_group must be str')
     if default is None:
       default = 0
     last_id_dict = getattr(self, 'last_id_dict', None)
@@ -108,6 +110,7 @@ class ZODBContinuousIncreasingIdGenerator(IdGenerator):
     if getattr(portal_ids, 'dict_ids', None) is not None:
       for id_group, last_id in portal_ids.dict_ids.items():
         if not isinstance(id_group, str):
+          assert not isinstance(id_group, bytes), id_group
           id_group = repr(id_group)
         if id_group in self.last_id_dict and \
            self.last_id_dict[id_group] > last_id:
@@ -148,7 +151,9 @@ class ZODBContinuousIncreasingIdGenerator(IdGenerator):
       self.clearGenerator()
     if not isinstance(id_dict, dict):
       raise TypeError('the argument given is not a dictionary')
-    for value in id_dict.values():
+    for key, value in id_dict.items():
+      if not isinstance(key, str):
+        raise TypeError('key %r given in dictionary is not str' % (key, ))
       if not isinstance(value, six.integer_types):
         raise TypeError('the value given in dictionary is not a integer')
     self.last_id_dict.update(id_dict)

product/ERP5/Tool/IdTool.py

@@ -118,10 +118,19 @@ class IdTool(BaseTool):
     if id_group in (None, 'None'):
       raise ValueError('%r is not a valid id_group' % id_group)
     # for compatibilty with sql data, must not use id_group as a list
+    if six.PY3 and isinstance(id_group, bytes):
+      warnings.warn('id_group must be a string, not bytes.', BytesWarning)
+      id_group = id_group.decode('utf-8')
     if not isinstance(id_group, str):
       id_group = repr(id_group)
       warnings.warn('id_group must be a string, other types '
                     'are deprecated.', DeprecationWarning)
+    if six.PY2:
+      try:
+        id_group.decode('utf-8')
+      except UnicodeDecodeError:
+        raise ValueError(
+          '%r is not a valid id_group, only valid UTF-8 strings are allowed' % id_group)
     if id_generator is None:
       id_generator = 'document'
     if method is not _marker:
@@ -177,11 +186,20 @@ class IdTool(BaseTool):
     """
     if id_group in (None, 'None'):
       raise ValueError('%r is not a valid id_group' % id_group)
+    if six.PY3 and isinstance(id_group, bytes):
+      warnings.warn('id_group must be a string, not bytes.', BytesWarning)
+      id_group = id_group.decode('utf-8')
     # for compatibilty with sql data, must not use id_group as a list
     if not isinstance(id_group, str):
       id_group = repr(id_group)
       warnings.warn('id_group must be a string, other types '
                     'are deprecated.', DeprecationWarning)
+    if six.PY2:
+      try:
+        id_group.decode('utf-8')
+      except UnicodeDecodeError:
+        raise ValueError(
+          '%r is not a valid id_group, only valid UTF-8 strings are allowed' % id_group)
     if id_generator is None:
       id_generator = 'uid'
     if store is not _marker:

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/IdTool_zCreateEmptyTable.sql

 CREATE TABLE `portal_ids` (
-  `id_group` VARBINARY(255),
+  `id_group` VARCHAR(255) BINARY,
   `last_id` BIGINT UNSIGNED,
   PRIMARY KEY  (`id_group`)
 ) ENGINE=InnoDB;
\ No newline at end of file

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/IdTool_zCreateTable.sql

 CREATE TABLE `portal_ids` (
-  `id_group` VARBINARY(255),
+  `id_group` VARCHAR(255) BINARY,
   `last_id` BIGINT UNSIGNED,
   PRIMARY KEY  (`id_group`)
 ) ENGINE=InnoDB

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/IdTool_zDump.sql

-select id_group, last_id from portal_ids
\ No newline at end of file
+select id_group, cast(id_group as CHAR) id_group from portal_ids
\ No newline at end of file

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/IdTool_zGetValueList.sql

-select id_group, last_id from portal_ids 
-  <dtml-if id_group>where id_group > "<dtml-var id_group>"</dtml-if>
+select cast(id_group as CHAR) id_group, last_id from portal_ids
+  <dtml-if id_group>where id_group > <dtml-sqlvar id_group type="string"></dtml-if>
   order by id_group
\ No newline at end of file

product/ERP5Type/Base.py

@@ -3646,7 +3646,7 @@ class Base(
       next_id = default
     new_next_id = None if poison else next_id + count
     id_generator_state[group].value = new_next_id
-    return range(next_id, new_next_id)
+    return ensure_list(range(next_id, new_next_id))
 
 InitializeClass(Base)