Code review comment for lp:~openerp-dev/openobject-addons/trunk-bug-921442-nco

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

The fix with the missing ustr() call is fine, but it was already correct in the branch from kjo, so I'll merge that in the 6.1 branch separately.

The other fix from Joel is unfortunately not correct for multiple reasons:

1. There is still no proper way to reproduce it that does not involve injecting the error directly in the OpenERP code. We need a way to reproduce this by passing an email message to the parse_message() method, anything else is flawed: when you directly patch the code to produce an error you can do anything that is impossible to trigger in real cases.

2. This is not a proper way to avoid unicode errors. When you call unicode(txt, errors='ignore') you *explicitly* drop anything that is not ASCII within `txt` in the process, because anything else is considered invalid in the default ASCII encoding! e.g.:
  >>> unicode('abcdé', errors='ignore')
  u'abcd'
A cleaner way to do it would be to use errors='replace' and pass the original encoding so only invalid bytes *in the specified encoding* will be replaced. i.e.
  >>> unicode('abcdé' + chr(255), 'utf8', errors='replace')
  u'abcd\xe9\ufffd''

3. This patch is attempting to spot-patch just one out of the dozens of many conversions that are performed in the parse_message() method, which makes little sense. If the code is supposed to be tolerant to malformed email parts, it should not do it for only of the possible places that can be malformed.

review: Disapprove

« Back to merge proposal