Repository: dmwm/wmcore
Issue: [py2py3] Fix potential bug in WMException

**Impact of the bug**

This bug could affect WMException and all the modules that depend on it that have already been modernized to py2py3, for example `WMConfigCache`.

**Describe the bug**

The line 
https://github.com/dmwm/WMCore/blob/875c3c5a48d0d82df947cdf13466c469c33ca27a/src/python/WMCore/WMException.py#L32
will always return `False`  in python2.

This is because we import ` from builtins import bytes ` and `bytes` actually is `future.types.newbytes.newbytes` in python2. While `isinstance(message, bytes)` correctly returns `True` when `message` is a python2 `str` (remember that a py2 `str` is a sequence of bytes), if we use `type(message) == bytes` this will try to match `str` with `future.types.newbytes.newbytes` and will always return `False`.

Effect: when formatting an exception message containing non-ascii characters from a modernized module that imports `str()` from future, WMException fails with 

```plaintext
Traceback (most recent call last):
  File "/home/dmwm/wmcore_unittest/WMCore/test/python/WMCore_t/WMException_t.py", line 62, in testExceptionUnicodeAsBytesSequence
    self.logger.debug("String version of exception: " + str(exception))
  File "/home/dmwm/unittestdeploy/wmagent/1.4.0-comp/sw/slc7_amd64_gcc630/external/py2-future/0.18.2/lib/python2.7/site-packages/future/types/newstr.py", line 104, in __new__
    return super(newstr, cls).__new__(cls, value)
  File "/home/dmwm/wmcore_unittest/WMCore/src/python/WMCore/WMException.py", line 149, in __str__
    strg += "Message: %s\\n" % self._message
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 74: ordinal not in range(128)
```

**How to reproduce it**

Add `from builtins import str` at the beginning of `test/python/WMCore_t/WMException_t.py` and run those unit tests.

**Additional context and error message**

`from builtins import str` is not at the beginning of `test/python/WMCore_t/WMException_t.py` yet because I modernized the unit test in #9868 with the goal "make WMException work", while the proper goal should have been "make WMException work, while calling it from a properly modernized unit test as if it were a generic WMCore module". This would have forced me to update `str()` in
https://github.com/dmwm/WMCore/blob/875c3c5a48d0d82df947cdf13466c469c33ca27a/test/python/WMCore_t/WMException_t.py#L67
to the modernized `str()` provided from future.

The possibility that this bug could happen emerged when investigating failing unit tests in #10172 

**Update** after opening #10187 : the correct purpose should have been "make WMException work, while calling it from a properly modernized unit test **and** from a non-modernized unit test". Pursuing the incorrect purpose caused #10174 to be inadequate to solve this issue and #10187 was necessary

**Expected behavior**

We should properly handle strings in WMException, and update WMException_t accordingly.



Changed files:
src/python/WMCore/WMException.py, test/python/WMCore_t/WMException_t.py

diff --git a/src/python/WMCore/WMException.py b/src/python/WMCore/WMException.py
index bdf3a9fb8..1a96cf8ef 100644
--- a/src/python/WMCore/WMException.py
+++ b/src/python/WMCore/WMException.py
@@ -6,7 +6,7 @@ General Exception class for WM modules
 
 """
 
-from builtins import bytes
+from builtins import str, bytes
 from future.utils import viewitems
 
 import inspect
@@ -29,9 +29,11 @@ class WMException(Exception):
 
     def __init__(self, message, errorNo=None, **data):
         self.name = str(self.__class__.__name__)
-        if type(message) == bytes:
+        if isinstance(message, bytes):
             # Fix for the unicode encoding issue, see #8056 and #8403
             # interprets this string using utf-8 codec and ignoring any errors
+            # unicode sandwich: convert sequence of bytes strings to
+            # unicode codepoints strings as soon as possible
             message = message.decode('utf-8', 'ignore')
 
         Exception.__init__(self, self.name, message)
@@ -52,7 +54,7 @@ class WMException(Exception):
             self.data.setdefault("ErrorNr", errorNo)
 
         self._message = message
-        self.data.update(data)
+        self.addInfo(**data)
 
         #  //
         # // Automatically determine the module name
@@ -62,7 +64,7 @@ class WMException(Exception):
                 frame = inspect.currentframe()
                 lastframe = inspect.getouterframes(frame)[1][0]
                 excepModule = inspect.getmodule(lastframe)
-                if excepModule != None:
+                if excepModule is not None:
                     modName = excepModule.__name__
                     self.data['ModuleName'] = modName
             finally:
@@ -83,7 +85,7 @@ class WMException(Exception):
         # // ClassName if ClassInstance is passed
         # //
         try:
-            if self.data['ClassInstance'] != None:
+            if self.data['ClassInstance'] is not None:
                 self.data['ClassName'] = self.data['ClassInstance'].__class__.__name__
         except Exception:
             pass
@@ -104,6 +106,12 @@ class WMException(Exception):
         """
         make exception look like a dictionary
         """
+        # unicode sandwich: convert sequence of bytes strings to
+        # unicode codepoints strings as soon as possible
+        if isinstance(key, bytes):
+            key = key.decode("utf-8", "ignore")
+        if isinstance(value, bytes):
+            value = value.decode("utf-8", "ignore")
         self.data[key] = value
 
     def addInfo(self, **data):
@@ -114,6 +122,13 @@ class WMException(Exception):
         exception instance
         """
         for key, value in viewitems(data):
+            # unicode sandwich: convert sequence of bytes strings to
+            # unicode codepoints strings as soon as possible
+            # assumption: value is not iterable (list, dict, tuple, ...)
+            if isinstance(key, bytes):
+                key = key.decode("utf-8", "ignore")
+            if isinstance(value, bytes):
+                value = value.decode("utf-8", "ignore")
             self[key] = value
         return
 
@@ -153,7 +168,7 @@ class WMException(Exception):
         strg += self.traceback
         strg += '\\n'
         strg += WMEXCEPTION_END_STR
-        if type(strg) == bytes:
+        if isinstance(strg, bytes):
             # Fix for the unicode encoding issue, #8043
             strg = strg.decode('utf-8', 'ignore')
         return strg
diff --git a/test/python/WMCore_t/WMException_t.py b/test/python/WMCore_t/WMException_t.py
index 08e5918ca..e94506117 100644
--- a/test/python/WMCore_t/WMException_t.py
+++ b/test/python/WMCore_t/WMException_t.py
@@ -7,6 +7,8 @@ General test for WMException
 
 """
 from __future__ import print_function, division
+from builtins import str, bytes
+
 import logging
 import unittest
 from WMCore.WMException import WMException
@@ -21,13 +23,29 @@ class WMExceptionTest(unittest.TestCase):
         setup log file output.
         """
         logging.basicConfig(level=logging.NOTSET,
-            format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s',
-            datefmt='%m-%d %H:%M',
-            filename='%s.log' % __file__,
-            filemode='w')
+                            format='%(asctime)s %(name)-12s %(levelname)-8s %(message)s',
+                            datefmt='%m-%d %H:%M',
+                            filename='%s.log' % __file__,
+                            filemode='w')
 
         self.logger = logging.getLogger('WMExceptionTest')
 
+        self.test_data = {
+            'key-ascii': 'value-1',       # unicode (ascii): unicode (ascii)
+            'key-ascii-b': '√@ʟυℯ-1',     # unicode (ascii): unicode (non-ascii)
+            'key-ascii-c': u'√@ʟυℯ-1',    # unicode (ascii): unicode (non-ascii)
+            'key-ascii-d': bytes('√@ʟυℯ-1', 'utf-8'),    # unicode (ascii): bytes (of non-ascii)
+            'ḱℯƴ-unicode-a': 'ṽ@łυ℮-2',   # unicode (non-ascii): unicode (non-ascii)
+            u'ḱℯƴ-unicode-b': 'ṽ@łυ℮-2',  # unicode (non-ascii): unicode (non-ascii)
+            bytes('ḱℯƴ-unicode-c', 'utf-8'): 'ṽ@łυ℮-2',  # bytes (of non-ascii): unicode (non-ascii)
+            'ḱℯƴ-unicode-d': 'value-\\x95',  # unicode (of non-ascii): unicode (invalid byte)
+            'key-\\x95': 'ṽ@łυ℮-2',  # unicode (invalid byte): unicode (non-ascii)
+            'key3': 3.14159,
+            # This would break WMException, but should not happen
+            # 'key4': {
+            #     b'ḱℯƴ-unicode-c': 'ṽ@łυ℮-2',  # bytes (of unicode): unicode
+            # }
+            }
 
     def tearDown(self):
         """
@@ -38,33 +56,37 @@ class WMExceptionTest(unittest.TestCase):
 
     def testException(self):
         """
-        create an exception and do some tests.
+        create an exception and do some tests (only ascii chars)
         """
 
         exception = WMException("an exception message with nr. 100", 100)
-        self.logger.debug("String version of exception: " + str(exception))
-        self.logger.debug("XML version of exception: " + exception.xml())
+        self.logger.debug("String version of exception: %s", str(exception))
+        self.logger.debug("XML version of exception: %s", exception.xml())
         self.logger.debug("Adding data")
         data = {}
         data['key1'] = 'value1'
-        data['key2'] = 'data2'
+        data['key2'] = 3.14159
         exception.addInfo(**data)
-        self.logger.debug("String version of exception: " + str(exception))
+        self.logger.debug("String version of exception: %s", str(exception))
 
-    def testExceptionUnicode(self):
+    def testExceptionUnicode0(self):
         """
-        create an exception with non-ascii characters and do some tests.
+        create an exception with non-ascii chars in message and test WMException.addInfo().
         """
 
         exception = WMException("an exception message with nr. 100 and some non-ascii characters: ₩♏ℭ☺яε", 100)
-        self.logger.debug("String version of exception: " + str(exception))
-        self.logger.debug("XML version of exception: " + exception.xml())
-        self.logger.debug("Adding data")
-        data = {}
-        data['key1'] = 'value1'
-        data['key2'] = 'data2'
-        exception.addInfo(**data)
-        self.logger.debug("String version of exception: " + str(exception))
+        exception.addInfo(**self.test_data)
+        self.logger.debug("XML version of exception: %s", exception.xml())
+        self.logger.debug("String version of exception: %s", str(exception))
+
+    def testExceptionUnicode1(self):
+        """
+        create an exception with non-ascii chars in message and test WMException constructor
+        """
+
+        exception = WMException("an exception message with nr. 100 and some non-ascii characters: ₩♏ℭ☺яε", 100, **self.test_data)
+        self.logger.debug("XML version of exception: %s", exception.xml())
+        self.logger.debug("String version of exception: %s", str(exception))
 
 
 if __name__ == "__main__":