IdTool: expect bytes for group_id

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.

🤔 a BTree can not contain str and bytes as keys (the same way that on
python2 it can not contain unicode and str), basically we just need a
consistent type and something compatible with how we transform the data
from python2 databases.

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

@@ -29,11 +29,14 @@
 ##############################################################################
 
 import unittest
+import warnings
 
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
 from Products.ERP5Type.tests.utils import createZODBPythonScript
 from MySQLdb import ProgrammingError
 from six.moves import range
+import six
+
 
 class TestIdTool(ERP5TypeTestCase):
 
@@ -146,6 +149,36 @@ class TestIdTool(ERP5TypeTestCase):
     self.assertEqual(21, self.id_tool.generateNewId(id_generator=id_generator,
                                       id_group='d02', default=3))
 
+    # generateNewId expect str, but convert id_group when passed a wrong type
+    with warnings.catch_warnings(record=True) as recorded:
+      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.')],
+    )
+
+    # 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:
+      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.')],
+        )
+
   def test_02a_generateNewIdWithZODBGenerator(self):
     """
       Check the generateNewId with a zodb id generator
@@ -234,6 +267,38 @@ class TestIdTool(ERP5TypeTestCase):
                                       id_generator=id_generator,
                                       id_group='d03', default=3, id_count=2))
 
+    # generateNewIdList expect str, but convert id_group when passed a wrong type
+    with warnings.catch_warnings(record=True) as recorded:
+      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.')],
+    )
+
+    # 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:
+      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.')],
+        )
+
   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,9 @@ class TestIdToolUpgrade(ERP5TypeTestCase):
     self.tic()
 
   def beforeTearDown(self):
+    self.portal.portal_caches.clearAllCache()
     self.id_tool.clearGenerator(all=True)
+    self.tic()
 
   def createGenerators(self):
     """
@@ -296,7 +298,26 @@ class TestIdToolUpgrade(ERP5TypeTestCase):
     id_generator.clearGenerator() # clear stored data
     self._checkDataStructureMigration(id_generator)
 
-def test_suite():
-  suite = unittest.TestSuite()
-  suite.addTest(unittest.makeSuite(TestIdToolUpgrade))
-  return suite
+  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.sql_generator.importGeneratorIdDict(exported, clear=True)
+    self.tic()
+    self.assertEqual(
+      self.sql_generator.generateNewId(id_group=self.id()),
+      1)

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)
@@ -109,6 +111,8 @@ class ZODBContinuousIncreasingIdGenerator(IdGenerator):
       for id_group, last_id in portal_ids.dict_ids.items():
         if not isinstance(id_group, str):
           id_group = repr(id_group)
+        if isinstance(id_group, bytes):
+          raise NotImplementedErro('TODO' + repr(id_group))
         if id_group in self.last_id_dict and \
            self.last_id_dict[id_group] > last_id:
           continue
@@ -148,7 +152,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,6 +118,9 @@ 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 '
@@ -177,6 +180,9 @@ 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)

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),
   `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),
   `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

@@ -3655,7 +3655,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)