WIP: Why?

bt5/erp5_interface_post/TestTemplateItem/portal_components/test.erp5.testInterfacePost.py

@@ -32,6 +32,7 @@ if six.PY2:
   from email import message_from_string as message_from_bytes
 else:
   from email import message_from_bytes
+from email.generator import Generator
 
 from Products.ERP5Type.tests.ERP5TypeLiveTestCase import ERP5TypeTestCase
 from Products.ERP5Type.tests.Sequence import SequenceList
@@ -39,6 +40,41 @@ from Products.ERP5Type.Utils import bytes2str, str2bytes
 from Products.ZSQLCatalog.SQLCatalog import SimpleQuery
 from DateTime import DateTime
 
+from six import StringIO
+import re
+
+def normalize_email_bytes(email_bytes):
+  # type: (bytes) -> str
+  """
+  Normalizes the representation of email text, so that it can be compared.
+
+  The fields of the message are written in a predefined order, with
+  the `unixfrom` field removed and no line wrapping.
+
+  The code is intended to be compatible with both Python 2 and Python 3.
+
+  Args:
+    email_bytes: Content of the email, including headers.
+
+  Returns:
+    Normalized string representation of the e-mail contents.
+  """
+  # Unfolding removes newlines followed by whitespace, as per RFC5322.
+  # This SHOULD be done by Python itself, but seemingly no-one cared
+  # enough.
+  email_bytes_unfolded = re.sub(
+    br"\r?\n(?P<space>\s)",
+    br"\g<space>",
+    email_bytes,
+  )
+  msg = message_from_bytes(email_bytes_unfolded)
+
+  fp = StringIO()
+  g = Generator(fp, mangle_from_=False, maxheaderlen=0)
+  g.flatten(msg)
+
+  return fp.getvalue()
+
 
 class TestInterfacePost(ERP5TypeTestCase):
   """
@@ -253,7 +289,10 @@ class TestInterfacePost(ERP5TypeTestCase):
     last_message, = self.portal.MailHost._message_list
     self.assertNotEqual((), last_message)
     _, _, message_text = last_message
-    self.assertIn(message_text, sequence['internet_message_post'].getData())
+    self.assertEqual(
+      normalize_email_bytes(message_text),
+      normalize_email_bytes(sequence['internet_message_post'].getData()),
+    )
 
   def _getMailHostMessageForRecipient(self, recipient_email_address):
     message_list = self.portal.MailHost._message_list
@@ -274,7 +313,10 @@ class TestInterfacePost(ERP5TypeTestCase):
       self.assertEqual(len(message_list), 1)
       message = message_list[0]
       _, _, message_text = message
-      self.assertIn(message_text, post.getData())
+      self.assertEqual(
+        normalize_email_bytes(message_text),
+        normalize_email_bytes(post.getData()),
+      )
 
   def stepCheckMailMessagePreviewDisplaysLatestInternetMessagePostData(self, sequence=None, sequence_list=None):
     mail_message = sequence['mail_message']

bt5/erp5_interfaces/DocumentTemplateItem/portal_components/document.erp5.SOAPBinding.py

@@ -31,6 +31,9 @@ from Products.ERP5Type import PropertySheet
 from Products.ERP5Type.Permissions import AccessContentsInformation
 from Products.ERP5Type.Base import Base
 import six
+import zope.component
+import zope.interface
+from ZPublisher.interfaces import IXmlrpcChecker
 try:
   from spyne import MethodContext
 except ImportError:
@@ -42,6 +45,20 @@ else:
   from spyne.server.http import HttpBase
 
 
+_default_xmrpc_checker = zope.component.queryUtility(IXmlrpcChecker)
+
+
+@zope.interface.implementer(IXmlrpcChecker)
+def soap_xmlrpc_checker(request):
+  if request.getHeader('SOAPACTION'):
+    return False
+  return _default_xmrpc_checker is None or _default_xmrpc_checker(request)
+
+
+zope.component.getGlobalSiteManager().registerUtility(
+  soap_xmlrpc_checker, IXmlrpcChecker)
+
+
 class SOAPBinding(Base):
 
   meta_type = 'ERP5 SOAP Binding'

bt5/erp5_joblib/TestTemplateItem/portal_components/test.erp5.testJoblibActivityUseCase.py

@@ -40,7 +40,7 @@ class Test(ERP5TypeTestCase):
     self.tic()
     active_process = self.portal.portal_activities.unrestrictedTraverse(path)
     result = active_process.getResultList()
-    self.assertAlmostEqual(0.98444444444444446, result[0].result)
+    self.assertAlmostEqual(0.9, result[0].result, 0)
 
   def test_UnderRootOfSquaresFunction(self):
     path = self.portal.Base_driverScriptSquareRoot()

bt5/erp5_json_form/ModuleComponentTemplateItem/portal_components/document.erp5.JSONForm.py

@@ -82,7 +82,7 @@ class JSONForm(JSONType, TextDocument):
       if list_error:
         validator = jsonschema.validators.validator_for(defined_schema)(defined_schema, format_checker=jsonschema.FormatChecker())
         return {
-          defined_schema["$id"].decode(): [
+          defined_schema["$id"]: [
             ("Validation Error", x.message) for x in sorted(validator.iter_errors(json_data), key=lambda e: e.path)
           ]
         }

bt5/erp5_run_my_doc/ExtensionTemplateItem/portal_components/extension.erp5.ERP5RunMyDocs.py

@@ -53,7 +53,7 @@ def extractTest(text):
           # Include Macros as it is defined by the user.
           testcode += row[0].text
         else:
-          testcode += lxml.html.tostring(row)
+          testcode += lxml.html.tostring(row, encoding='unicode')
   return testcode.strip()
 
 """

bt5/erp5_simplified_invoicing/TestTemplateItem/portal_components/test.erp5.testTradeModelLine.py

@@ -62,6 +62,10 @@ class TestTradeModelLineMixin(TestBPMMixin, UserDict):
   order_date = DateTime()
   amount_generator_line_portal_type = 'Trade Model Line'
 
+  # XXX so that unittest.suite._isnotsuite return False
+  def __iter__(self):
+    raise TypeError()
+
   def setBaseAmountQuantityMethod(self, base_amount_id, text):
     """Populate TradeModelLine_getBaseAmountQuantityMethod shared script
 
@@ -416,6 +420,7 @@ class TestTradeModelLine(TestTradeModelLineMixin):
     expected_result_dict = self[delivery.getPath()]
 
     for line in delivery.getMovementList():
+      currency_precision = line.getPricePrecision()
       simulation_movement_list_list = self.getTradeModelSimulationMovementList(line)
       self.assertEqual(len(simulation_movement_list_list), 1)
       simulation_movement_list = simulation_movement_list_list[0]
@@ -426,7 +431,7 @@ class TestTradeModelLine(TestTradeModelLineMixin):
       for use in 'discount', 'tax':
         total_price = expected_result_dict[use].get(line.getId()) or 0.0
         sm = result_dict.pop(use)
-        self.assertEqual(str(sm.getTotalPrice() or 0.0), str(total_price))
+        self.assertEqual(round(sm.getTotalPrice() or 0.0, currency_precision), round(total_price, currency_precision))
         self.assertEqual(3, len(sm.getCausalityValueList()))
         self.assertEqual(1, len(sm.getCausalityValueList(
           portal_type=self.business_link_portal_type)))
@@ -466,12 +471,12 @@ class TestTradeModelLine(TestTradeModelLineMixin):
     rounded_total_price = round(line_dict['normal'], currency_precision)
     rounded_tax_price = round(line_dict['tax'], currency_precision)
     rounded_discount_price = round(line_dict['discount'], currency_precision)
-    self.assertEqual(str(abs(line_dict['payable_receivable'])),
-        str(rounded_total_price + rounded_tax_price + rounded_discount_price))
-    self.assertEqual(str(abs(line_dict['vat'])),
-        str(rounded_tax_price))
-    self.assertEqual(str(abs(line_dict['income_expense'])),
-        str(rounded_total_price + rounded_discount_price))
+    self.assertEqual(round(abs(line_dict['payable_receivable']), currency_precision),
+        round(rounded_total_price + rounded_tax_price + rounded_discount_price, currency_precision))
+    self.assertEqual(round(abs(line_dict['vat']), currency_precision),
+        rounded_tax_price)
+    self.assertEqual(round(abs(line_dict['income_expense']), currency_precision),
+        round(rounded_total_price + rounded_discount_price, currency_precision))
 
   def buildPackingLists(self):
     self.portal.portal_alarms.packing_list_builder_alarm.activeSense()

bt5/erp5_web/TestTemplateItem/portal_components/test.erp5.testERP5Web.py

@@ -260,6 +260,7 @@ class TestERP5Web(ERP5TypeTestCase):
     page.edit(text_content='<p>Hé Hé Hé!</p>', content_type='text/html')
     self.tic()
     self.assertEqual('Hé Hé Hé!', page.asText().strip())
+    self.assertIn('Hé Hé Hé!', page.getSearchableText())
 
   def test_WebPageAsTextHTMLEntities(self):
     """Check if Web Page's asText() converts html entities properly

bt5/erp5_web_renderjs_ui_test/SkinTemplateItem/portal_skins/erp5_web_renderjs_ui_test/MailHost_redirectPageToURLFromLastMessage.py

-last_message = context.getLastLog()[-1]
-line_list = filter(None, last_message.replace("=\n","").split("\n"))
-for line in line_list:
-  if "http" in line:
-    return context.REQUEST.RESPONSE.redirect(line.replace("=3D", "="))
-
-raise RuntimeError("URL not found in the email")
+import re
+last_message_text = context.getMessageList()[-1][2]
+return container.REQUEST.RESPONSE.redirect(re.findall(r"http.*", last_message_text)[0])

bt5/erp5_web_shadir/ExtensionTemplateItem/portal_components/extension.erp5.ShaDir.py

@@ -80,6 +80,7 @@ def WebSection_setObject(self, id, ob, **kw):
     Make any change related to the file uploaded.
   """
   portal = self.getPortalObject()
+  ob = ob.getOriginalDocument()
   data = self.REQUEST.get('BODY')
   try:
     metadata, signature = loads(data)
@@ -128,7 +129,13 @@ def WebSection_putFactory(self, name, typ, body):
   document = portal.portal_contributions.newContent(data=body,
                                                     filename=name,
                                                     discover_metadata=False)
-  return document
+
+  # return a document for which getId() returns the name for _setObject to be
+  # called with id=name ( for WebSection_setObject ), but for which
+  # getRelativeUrl returns the relative url of the real document, for
+  # VirtualFolderMixin transactional variable cache between _setObject and
+  # _getOb
+  return document.asContext(getId=lambda: name)
 
 # The following scripts are helpers to search & clean up shadir entries.
 # XXX: Due to lack of View skin for shadir, external methods are currently

bt5/erp5_web_shadir/TestTemplateItem/portal_components/test.erp5.testShaDir.py

@@ -105,19 +105,23 @@ class TestShaDir(ShaDirMixin, ERP5TypeTestCase):
     data_set = self.portal.portal_catalog.getResultValue(
       reference=self.key)
     self.assertEqual(self.key, data_set.getReference())
+    self.assertNotEqual(self.key, data_set.getId())
     self.assertEqual('published', data_set.getValidationState())
+    self.assertEqual(len(self.portal.data_set_module.contentValues()), 1)
 
     # Asserting Document
     document = self.portal.portal_catalog.getResultValue(
       reference=self.sha512sum)
     self.assertEqual(self.sha512sum, document.getTitle())
     self.assertEqual(self.sha512sum, document.getReference())
+    self.assertNotEqual(self.sha512sum, document.getId())
     self.assertEqual(self.data, document.getData())
     self.assertEqual(data_set, document.getFollowUpValue())
     self.assertEqual(str(self.expiration_date),
                                     str(document.getExpirationDate()))
     self.assertEqual('application/json', document.getContentType())
     self.assertEqual('Published', document.getValidationStateTitle())
+    self.assertEqual(len(self.portal.document_module.contentValues()), 1)
 
   def test_get_information(self):
     """

product/ERP5/bootstrap/erp5_core/ToolComponentTemplateItem/portal_components/tool.erp5.SimulationTool.py

@@ -63,6 +63,7 @@ from hashlib import md5
 from warnings import warn
 from six.moves.cPickle import loads, dumps
 from copy import deepcopy
+import base64
 import six
 
 MYSQL_MIN_DATETIME_RESOLUTION = 1/86400.
@@ -1439,7 +1440,7 @@ class SimulationTool(BaseTool):
     if src__:
       sql_source_list.append(Resource_zGetInventoryCacheResult(src__=1, **inventory_cache_kw))
     if cached_sql_result:
-      brain_result = loads(cached_sql_result[0].result)
+      brain_result = loads(base64.b64decode(cached_sql_result[0].result))
       # Rebuild the brains
       cached_result = Results(
         (brain_result['items'], brain_result['data']),
@@ -1488,10 +1489,10 @@ class SimulationTool(BaseTool):
         self.Resource_zInsertInventoryCacheResult(
           query=sql_text_hash,
           date=cached_date,
-          result=dumps({
+          result=base64.b64encode(dumps({
             'items': result.__items__,
             'data': result._data,
-          }),
+          })),
         )
     else:
       # Cache miss and this getInventory() not specifying to_date,

product/ERP5OOo/tests/TestFormPrintoutMixin.py

@@ -48,7 +48,7 @@ class TestFormPrintoutMixin(ERP5TypeTestCase):
     '''return odf document from the printout
     '''
     document_file = getattr(self.portal, printout_form.template, None)
-    document_file = StringIO(document_file).read()
+    document_file = bytes(document_file)
     if document_file is not None:
       return document_file
     raise ValueError ('%s template not found' % printout_form.template)

product/ERP5OOo/tests/testDeferredStyle.py

@@ -391,7 +391,7 @@ class TestDeferredStyleBase(DeferredStyleTestCase):
     # after they are saved to DB and automatically migrated. The getProperty
     # above, which is also what ods_style does, only work after the report
     # state is updated.
-    report.__setstate__(aq_base(getattr(skin_folder, report_form_name)).__getstate__())
+    aq_base(report).__setstate__(aq_base(getattr(skin_folder, report_form_name)).__getstate__())
     self.assertEqual(report.getProperty('title'), self.id())
 
     # Report section method

product/ERP5Type/dynamic/lazy_class.py

@@ -376,7 +376,7 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
       for key, value in six.iteritems(attribute_dict):
         setattr(klass, key, value)
 
-      if getattr(klass.__setstate__, 'im_func', None) is \
+      if getattr(klass.__setstate__, '__func__', klass.__setstate__) is \
          persistent_migration.__setstate__:
         # optimization to reduce overhead of compatibility code
         klass.__setstate__ = persistent_migration.Base__setstate__

product/ERP5Type/dynamic/persistent_migration.py

@@ -34,6 +34,7 @@
 # class may be copied in the pickle of the container, and we can't access it
 # from __setstate__.
 
+import six
 import logging, re
 from AccessControl import ClassSecurityInfo
 from Acquisition import aq_base
@@ -97,7 +98,6 @@ class PickleUpdater(ObjectReader, ObjectWriter, object):
         if _setOb:
           if isinstance(_setOb, WorkflowMethod):
             _setOb = _setOb._m
-          import six
           if six.get_unbound_function(_setOb) is six.get_unbound_function(OFS_Folder._setOb):
             self.lazy = Ghost
         elif klass.__module__[:7] == 'BTrees.' and klass.__name__ != 'Length':
@@ -114,8 +114,12 @@ class PickleUpdater(ObjectReader, ObjectWriter, object):
         self.do_migrate = args != (klass.__module__, klass.__name__) and \
                           not isOldBTree('%s.%s' % args)
         unpickler.find_global = self._get_class
+        if six.PY3:
+          unpickler.find_class = self._get_class
         return self._get_class(*args)
       unpickler.find_global = find_global
+      if six.PY3:
+        unpickler.find_class = find_global
       unpickler.load() # class
       state = unpickler.load()
       if isinstance(self.lazy, LazyPersistent):

product/ERP5Type/tests/ERP5TypeFunctionalTestCase.py

@@ -42,7 +42,7 @@ from ZPublisher.HTTPResponse import HTTPResponse
 from zExceptions.ExceptionFormatter import format_exception
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
 from Products.ERP5Type.tests.runUnitTest import log_directory
-from Products.ERP5Type.Utils import stopProcess, PR_SET_PDEATHSIG
+from Products.ERP5Type.Utils import stopProcess, PR_SET_PDEATHSIG, unicode2str
 from lxml import etree
 from lxml.html.builder import E
 import certifi
@@ -397,7 +397,7 @@ class FunctionalTestRunner:
             for test_tr in test_table.xpath('.//tr[contains(@class, "status_failed")]'):
               test_tr.set('style', 'background-color: red;')
             details_attribute_dict = {}
-            if etree.tostring(test_table).find("expected failure") != -1:
+            if u"expected failure" in etree.tostring(test_table, encoding="unicode"):
               expected_failure_amount += 1
             else:
               failure_amount += 1
@@ -405,7 +405,7 @@ class FunctionalTestRunner:
               details_attribute_dict['open'] = 'true'
             detail_element = E.div()
             detail_element.append(E.details(E.summary(test_name), test_table, **details_attribute_dict))
-            detail += etree.tostring(detail_element)
+            detail += unicode2str(etree.tostring(detail_element, encoding="unicode"))
       tr_count += 1
     success_amount = tr_count - 1 - failure_amount - expected_failure_amount
     if detail:

product/PortalTransforms/transforms/text_pre_to_html.py

+import six
 from Products.PortalTransforms.interfaces import ITransform
 from zope.interface import implementer
 from DocumentTemplate.html_quote import html_quote
@@ -30,6 +31,7 @@ class TextPreToHTML:
         raise AttributeError(attr)
 
     def convert(self, orig, data, **kwargs):
+        orig = six.ensure_text(orig, errors='replace')
         data.setData('<pre class="data">%s</pre>' % html_quote(orig))
         return data
 

product/PortalTransforms/transforms/text_to_html.py

+import six
 from Products.PortalTransforms.interfaces import ITransform
 from zope.interface import implementer
 from DocumentTemplate.html_quote import html_quote
@@ -30,6 +31,7 @@ class TextToHTML:
         raise AttributeError(attr)
 
     def convert(self, orig, data, **kwargs):
+        orig = six.ensure_text(orig, errors='replace')
         # Replaces all line breaks with a br tag, and wraps it in a p tag.
         data.setData('<p>%s</p>' % html_quote(orig.strip()).replace('\n', '<br />'))
         return data

product/ZSQLCatalog/Extensions/zsqlbrain.py

@@ -46,7 +46,11 @@ class ZSQLBrain(Acquisition.Implicit):
     return self.path
 
   def getPath(self):
-    return self.path
+    path = self.path
+    # TODO py3: understand why this is bytes sometimes
+    if not isinstance(path, str):
+      path = path.decode()
+    return path
 
   def getUid(self):
     return self.uid

[Kazuhiko Shiozaki]

@arnau as written in the comment here, email.message.Message's as_string() result is different betweeh py2 and py3 (folded in py2, but not in py3 etc.). thus @jerome introduced this utility method to compare on both py2 and py3.

[Jérome Perrin]

yes but it was introduced by @vnmabus

BTW, https://www.python.org/downloads/release/python-3920/ was released this week with some fixes for email, this one looks similar but probably different:

gh-121650: email headers with embedded newlines are now quoted on output. The generator will now refuse to serialize (write) headers that are unsafely folded or delimited; see verify_generated_headers. That’s CVE-2024-6923.

[Arnaud Fontaine]

@kazuhiko Thanks. I merged it into the big commits.

[Kazuhiko Shiozaki]

Hmm, it's @jerome 's commit 😄

[Jérome Perrin]

The commit was ee4399c0 but there's no explanation. This is a change from https://github.com/zopefoundation/Zope/commit/0206850874bbf8dd4917f90209eec8afe4deb9e6 ( starting from zope 5.8.2 ), before this change when having HTTP_SOAPACTION in environ, there was no attempt at interpreting an XMLRPC request ( this was an elif ), after this change there is I don't know if that was really intentional, but Zope does not really support XMLRPC (see comment in the test so I don't really feel that it's wrong that we have to do this. Strictly speaking it's maybe a bug in Zope, but I don't think we need to fight more with this one.

The commit message for all the changes to this file could be something like this:

interfaces: register a ``IXmlrpcChecker` for Zope 5.8.2 compatibility

since Zope commit 020685087 (Allow ZPublisher to handle a query string together
with a request body (#1124), 2023-05-15) Zope tries to process all XML HTTP requests
as XML-RPC and we need to tell that these SOAP requests are not XML-RPC

[Arnaud Fontaine]

Thanks, I added a commit with the commit message you gave me.

[Jérome Perrin]

this must be changed to unicode2str, this comes from json.loads which returns unicode on python2

[Jérome Perrin]

The idea is that encoding='unicode' returns a str on py3 and a unicode on py2. run_my_doc is also one of these business templates made by a trainee and never changed since :)

[Jérome Perrin]

I remember this was tricky. This test inherits from UserDict so that the test can use "dict interface" as "storage" for values, it does self[some_key] = some_value, but on python3 this caused some errors in unittest framework.

I thought about refactoring the test to not use dict interface directly, but use another attribute for "storage" something like self._storage[some_key] = some_value, which would be less magic, but this trick made the test pass so I thought it's enough, this test refactoring looked like a big change.

[Arnaud Fontaine]

Thanks for the explanation. I added what you said as a comment and committed it to the big commit.

[Jérome Perrin]

I think I changed this, it seems more correct to compare currency values rounded with the currency precision, that's what we do in some places of the application. It was probably failing on py3 but I don't remember well

[Kazuhiko Shiozaki]

@arnau here, str -> round change is for the following reason.

• py2

>>> str(sum(0.1 for e in range(10))), str(1.0)
('1.0', '1.0')

• py3

>>> str(sum(0.1 for e in range(10))), str(1.0)
('0.9999999999999999', '1.0')

and here the intention of assertions should be based on rounded values.

[Arnaud Fontaine]

Thanks. Indeed, on Python 3.9.19 (version we currently use in ERP5 SR), it does return '0.9999999999999999' but not on Python 3.12.5 shipped by my GNU/Linux distribution:

Python 3.12.5 (main, Aug  9 2024, 08:20:41) [GCC 14.2.1 20240805] on linux
>>> str(sum(0.1 for e in range(10))), str(1.0)
('1.0', '1.0')

[Jérome Perrin]

that looks like a good news, my idea was to stay on python3.9 and merge all this with python3.9 and right after continue with more recent python.

I remember this behavior of str with many decimal was causing a test failure with a printed pay sheet, it was something like 0.1 on python2 and 0.099999999 on python3, so we might have some user visible bugs with this - and BTW if we have the problem is that we usually don't want to just str a float, we want to format it with specifying the number of decimals we want.

[Arnaud Fontaine]

So in this test, we should have done round(..., currency_precision) to properly format the float with the number of decimals we want. So that's not really Python-related and I will move it to Cosmetic changes and remove unused imports. (I will change this commit message because it's now basically all minor changes not related to py3), is that okay?

[Jérome Perrin]

getSearchableText was probably not tested, I think at some point it returned bytes

[Jérome Perrin]

I guess what we change is that we actually decode the email with the API (getMessageList probably decodes) instead of that "manual decoding" (.replace("=3D", "=")) which was probably only working on py2

[Arnaud Fontaine]

I see. I will change a bit the py3 code to keep the same flow (raising RuntimeError if http no in the string).

[Jérome Perrin]

I just remember this was hard :) something must have changed in PUT, with this approach in works on py2 and py3 but I don't remember more than this comment. Isn't it enough ?

[Arnaud Fontaine]

I don't really understand the comment but anyway I merged it into the big commit, thanks!

[Jérome Perrin]

( these are also more tests because "this was hard" )

[Jérome Perrin]

this was not explained in commit message ? I think we stored binary but the column is text so we have to "encode" in something like base64 which fits in text column. (Once again, this is not a precise comment, I'm writing what I remember now )

[Kazuhiko Shiozaki]

this was not explained in commit message ?

Yes.

commit ff4075db9307db04273744f20b263dbc60906a29
Author: Kazuhiko SHIOZAKI <kazuhiko@nexedi.com>
Date:   Fri Oct 14 20:59:25 2022 +0200
py2/py3: Base64 encode inventory cache, as Shared.DC.ZRDB.DA.SQL tries to decode bytes to str.

[Arnaud Fontaine]

Thanks, I cherry-picked that commit.

[Jérome Perrin]

probably the StringIO(document_file).read() was initially just a more complex way to do str(document_file) - something that we replaced everywhere else by bytes(document_file)

[Arnaud Fontaine]

Ok, thanks!

[Jérome Perrin]

I forgot the details, but this was to simulate the object being loaded again from database (not using the ZODB cache which returns the same instance if it's still there) and this is different on py3

[Jérome Perrin]

I would like to understand this one :)

[Kazuhiko Shiozaki]

Without this...

======================================================================
FAIL: testMigrateOldObject (testDynamicClassGeneration.TestPortalTypeClass)
Check migration of persistent objects with non-erp5.portal_type classes
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/(SR)/parts/erp5/Products/ERP5Type/tests/testDynamicClassGeneration.py", line 93, in testMigrateOldObject
    check(1)
  File "/(SR)/parts/erp5/Products/ERP5Type/tests/testDynamicClassGeneration.py", line 85, in check
    self.assertEqual(klass.__setstate__ is Persistent.__setstate__, migrated)
AssertionError: False != 1

----------------------------------------------------------------------

and here...

-> klass.__setstate__ = persistent_migration.Base__setstate__
(Pdb) l
378
379           if getattr(klass.__setstate__, '__func__', klass.__setstate__) is \
380              persistent_migration.__setstate__:
381             import pdb; pdb.set_trace()
382             # optimization to reduce overhead of compatibility code
383  ->         klass.__setstate__ = persistent_migration.Base__setstate__
384
385           for interface in interface_list:
386             classImplements(klass, interface)
387
388           # Skip this during the early Base Type/Types Tool generation because
(Pdb) klass.__setstate__
<function __setstate__ at 0x7faa574ad430>
(Pdb) klass
<class 'erp5.portal_type.Types Tool'>
(Pdb) klass.__setstate__.__func__
*** AttributeError: 'function' object has no attribute '__func__'
(Pdb) klass.__setstate__.im_func
*** AttributeError: 'function' object has no attribute 'im_func'

but I'm not sure what would be an appropriate commit message for this.

[Arnaud Fontaine]

I still don't understand why we have to do this with py3 and not py2?

[Kazuhiko Shiozaki]

Me neither. FYI, here is like that on py2

(Pdb) klass.__setstate__
<unbound method Base Type.__setstate__>
(Pdb) getattr(klass.__setstate__, '__func__')
<function __setstate__ at 0x7f78082d2250>
(Pdb) getattr(klass.__setstate__, 'im_func')
<function __setstate__ at 0x7f78082d2250>
(Pdb) persistent_migration.__setstate__
<function __setstate__ at 0x7f78082d2250>

[Arnaud Fontaine]

I investigated seriously and the reason is that there is no more unbound method in py3. Here is the diff of the fix I applied to the big commit:

diff --git a/product/ERP5Type/dynamic/lazy_class.py b/product/ERP5Type/dynamic/lazy_class.py
index 122d46bcd3..3cead40b5f 100644
--- a/product/ERP5Type/dynamic/lazy_class.py
+++ b/product/ERP5Type/dynamic/lazy_class.py
@@ -376,8 +376,11 @@ class PortalTypeMetaClass(GhostBaseMetaClass, PropertyHolder):
       for key, value in six.iteritems(attribute_dict):
         setattr(klass, key, value)
 
-      if getattr(klass.__setstate__, '__func__', klass.__setstate__) is \
-         persistent_migration.__setstate__:
+      if six.PY3:
+        klass__setstate__ = klass.__setstate__
+      else:
+        klass__setstate__ = getattr(klass.__setstate__, 'im_func', None)
+      if klass__setstate__ is persistent_migration.__setstate__:
         # optimization to reduce overhead of compatibility code
         klass.__setstate__ = persistent_migration.Base__setstate__

[Kazuhiko Shiozaki]

or we can use six.get_unbound_function that we already use in several places in a big commit a17bb910.

[Kazuhiko Shiozaki]

Hmm... six.get_unbound_function just does method.im_func but it can raise AttributeError.

2024-09-18 12:23:58.749 WARNING lazy_class.__getattribute__ Failed to load class : AttributeError("'method_descriptor' object has no attribute 'im_func'",)                                                                                     Traceback (most recent call last):                                                                                        File "/(SR)/parts/erp5/product/ERP5Type/dynamic/lazy_class.py", line 120, in __getattribute__                                                                             self.__class__.loadClass()                                                                                            File "/(SR)/parts/erp5/product/ERP5Type/dynamic/lazy_class.py", line 379, in loadClass
    if six.get_unbound_function(klass.__setstate__) is \
  File "/(SR)/eggs/six-1.16.0-py2.7.egg/six.py", line 571, in get_unbound_function
    return unbound.im_func
AttributeError: 'method_descriptor' object has no attribute 'im_func'

thus your change above should be better and perfect.

[Arnaud Fontaine]

Yes indeed. I used six.get_unbound_function() elsewhere but not here because of the AttributeError. There was no other function which could have been used in six...

[Jérome Perrin]

I think unpickler API changed between py2 ( https://docs.python.org/2/library/pickle.html?highlight=find_global#subclassing-unpicklers ) and py3 ( https://docs.python.org/3/library/pickle.html#pickle.Unpickler.find_class )

[Arnaud Fontaine]

Thanks for the pointer. At some point I got lazy to check changes one by one ;-).

[Jérome Perrin]

I am not sure, but with text conversion most transforms can be called with str (for text mime types) or bytes (for other mimetpes), this must be just another case.

[Arnaud Fontaine]

It seems suspicious to me to do errors='replace' on orig. I think I will just run the tests without this change and see if it's really needed or not.

[Jérome Perrin]

Yes, I don't remember why we use errors=replace here

[Jérome Perrin]

what I understood since this comment is that selecting a VARCHAR column gives a str on python3 and selecting a BINARY column gives bytes. But since I understood this I did not try to remove this XXX. It seems good to investigate this more.

[Arnaud Fontaine]

But path column is VARCHAR so why it would be a bytes() then?

[Jérome Perrin]

hehe yes, that's sill a mystery for now :) I vaguely remember that this might have to do with nested selects like

SELECT
  q1.*,
  @running_total_quantity := q1.total_quantity +
            @running_total_quantity AS running_total_quantity,
  @running_total_price := IFNULL(q1.total_price, 0) +
            @running_total_price AS running_total_price
FROM (
SELECT
  catalog.path as path,
  catalog.uid as uid,

in Resource_zGetMovementHistoryList but the best would be to run the tests without this change to this what breaks

[Jérome Perrin]

I looked at that, one case is with Base_zGetImplicitSuccessorValueList

It does

SELECT
    @current_path:=IF(@current_reference = reference, @current_path, path) AS path

and path is bytes.

This weird behavior can be reduced to

(Pdb) self.portal.erp5_sql_connection.manage_test('SELECT IF(1=0, 0, "path") as path')[0].path    
'path'
(Pdb) self.portal.erp5_sql_connection.manage_test('SELECT IF(1=0, @VAR, "path") as path')[0].path 
b'path'

when using a if, if the true branch is a @ variable, we get bytes, in other cases we get string.

Another workaround seems to cast in SQL

(Pdb) self.portal.erp5_sql_connection.manage_test('SELECT CAST(IF(1=0, @VAR, "path") as CHAR) as path')[0].path 
'path'

The workaround of casting in SQL seems a bit more elegant and we don't need this if not isinstance(path, str) all the time, only for the rare case where we use this pattern. What do you think ?

There might be other cases, I ran tests and noticed this one but there was lots of failures for the version I tested.

[Arnaud Fontaine]

Thanks for the investigation. Sounds good to me to do if not instance(path, str) as AFAIU it's coming from the Python code which converts the return value of SQL query to byte.

[Arnaud Fontaine]

If that's also okay with you, then I can add a comment before if not instance(path, str) and merge it to the big commit?

[Jérome Perrin]

The python code ( in python-mysql ) decides to convert to bytes or str depending on the column type.

It seems that if the column type is inferred from a NULL variable ( at the beginning we have SET @current_path = NULL ), then the column type is blob, so bytes in python.

MariaDB [test]> set @defined_as_null=null; drop table if exists tmp; create table tmp as (select @defined_as_null); show create table tmp;
+-------+------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                       |
+-------+------------------------------------------------------------------------------------------------------------------------------------+
| tmp   | CREATE TABLE `tmp` (
  `@defined_as_null` longblob DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci |
+-------+------------------------------------------------------------------------------------------------------------------------------------+

I feel it would be a bit better to do the conversion in SQL, but not too strongly, maybe because this initially looked like a hack to just decode without understanding and because if we do it in SQL then we this does not affect all users of getPath. If you prefer doing it in getPath I'm fine with that as well.

[Arnaud Fontaine]

You're right, seems a bit better to do the conversion in SQL.

[Kazuhiko Shiozaki]

Maybe not related, but is there any reason to keep using old-enough python-mysqlclient ?

We are using 1.3.12 for py2 and 2.0.1 for py3, but the latest is 1.4.6 for py2 and 2.2.4 for py3.

[Jérome Perrin]

I don't remember a reason for using old versions like that. Do you want to try to push a commit in for_testrunner_1 branch to see if it breaks ?

[Jérome Perrin]

I pushed ee19f449 to do the conversion in SQL and amended the "WIP: Why ?" commit to remove this part.

[Arnaud Fontaine]

Great, thanks!