Merge lp:~camptocamp/openerp-connector/7.0-unicode-pickled-1288187 into lp:~openerp-connector-core-editors/openerp-connector/7.0

[Guewen Baconnier @ Camptocamp]

Alternative fix for lp:1288187
(the other one is https://code.launchpad.net/~acsone-openerp/openerp-connector/bug-1288187-sbi-unicode/+merge/209430)

[Stéphane Bidoul (Acsone) (sbi)]

LGTM. Binary fields are cleaner and both approaches require migration of existing data.

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> LGTM. Binary fields are cleaner and both approaches require migration of
> existing data.

Thanks for your opinion. I added a migration script, can you check if it works for you?

[Stéphane Bidoul (Acsone) (sbi)]

Works fine. I also tested the migration script.

Thanks for the fast reaction!

review: Approve (code review and test)

[Leonardo Pistone (lepistone)]

I tried to give a look to the ORM to see if it clearly treats string field contents as unicode, as opposed to utf-8 bytestrings, but that is convoluted to say the least (it uses ustr() from server/openerp/loglevels.py).

Picked data is indeed binary, so I agree, good idea. And indeed it is a good idea to test it

I have a little doubt left: this line from job.py:

    func = _unpickle(str(stored.func)) # openerp stores them as unicode...

Is "str" still necessary?

Moreover, maybe we could add in the test a bytestring parameter with accents.

Thanks!

review: Needs Information (code review)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> I tried to give a look to the ORM to see if it clearly treats string field
> contents as unicode, as opposed to utf-8 bytestrings, but that is convoluted
> to say the least (it uses ustr() from server/openerp/loglevels.py).
>
> Picked data is indeed binary, so I agree, good idea. And indeed it is a good
> idea to test it
>
> I have a little doubt left: this line from job.py:
>
> func = _unpickle(str(stored.func)) # openerp stores them as unicode...
>
> Is "str" still necessary?

Good catch!

>
> Moreover, maybe we could add in the test a bytestring parameter with accents.

You are right, added.

>
> Thanks!

Thanks for your review

[Leonardo Pistone (lepistone)]

thanks!

review: Approve (code review)

[lp:~camptocamp/openerp-connector/7.0-unicode-pickled-1288187]

627. By Guewen Baconnier @ Camptocamp on 2014-03-06

confusing typo

628. By Guewen Baconnier @ Camptocamp on 2014-03-19

func::bytea can fail to convert the string to bytea because it interprets the backshlashes. Use convert_to to LATIN1 instead

=== modified file 'connector/__openerp__.py'
--- connector/__openerp__.py	2014-02-06 10:09:11 +0000
+++ connector/__openerp__.py	2014-03-19 11:14:05 +0000
@@ -20,7 +20,7 @@
 ##############################################################################
 
 {'name': 'Connector',
- 'version': '2.1.1',
+ 'version': '2.2.0',
  'author': 'Openerp Connector Core Editors',
  'website': 'http://openerp-connector.com',
  'license': 'AGPL-3',

=== added directory 'connector/migrations'
=== added directory 'connector/migrations/2.2.0'
=== added file 'connector/migrations/2.2.0/pre-migration.py'
--- connector/migrations/2.2.0/pre-migration.py	1970-01-01 00:00:00 +0000
+++ connector/migrations/2.2.0/pre-migration.py	2014-03-19 11:14:05 +0000
@@ -0,0 +1,9 @@
+# -*- encoding: utf-8 -*-
+
+__name__ = "Convert jobs pickled func to bytea"
+
+def migrate(cr, version):
+    if not version:
+        return
+    cr.execute("ALTER TABLE queue_job ALTER func "
+               "TYPE bytea USING convert_to(func, 'LATIN1') ")

=== modified file 'connector/queue/job.py'
--- connector/queue/job.py	2014-02-04 14:39:23 +0000
+++ connector/queue/job.py	2014-03-19 11:14:05 +0000
@@ -65,7 +65,7 @@
     raised as `NotReadableJobError`).
     """
     try:
-        unpickled = loads(pickled.encode('utf-8'))
+        unpickled = loads(pickled)
     except (StandardError, UnpicklingError):
         raise NotReadableJobError('Could not unpickle.', pickled)
     return unpickled
@@ -229,7 +229,7 @@
                                        self._openerp_id(job_uuid),
                                        context=self.session.context)
 
-        func = _unpickle(str(stored.func))  # openerp stores them as unicode...
+        func = _unpickle(stored.func)
 
         (func_name, args, kwargs) = func
 

=== modified file 'connector/queue/model.py'
--- connector/queue/model.py	2013-11-19 10:14:40 +0000
+++ connector/queue/model.py	2014-03-19 11:14:05 +0000
@@ -53,7 +53,7 @@
         'user_id': fields.many2one('res.users', string='User ID', required=True),
         'name': fields.char('Description', readonly=True),
         'func_string': fields.char('Task', readonly=True),
-        'func': fields.text('Pickled Function', readonly=True, required=True),
+        'func': fields.binary('Pickled Function', readonly=True, required=True),
         'state': fields.selection(STATES,
                                   string='State',
                                   readonly=True,

=== modified file 'connector/tests/test_job.py'
--- connector/tests/test_job.py	2014-01-13 16:07:35 +0000
+++ connector/tests/test_job.py	2014-03-19 11:14:05 +0000
@@ -161,6 +161,44 @@
         self.assertAlmostEqual(job.eta, job_read.eta,
                                delta=delta)
 
+    def test_unicode(self):
+        job = Job(func=dummy_task_args,
+                  model_name='res.users',
+                  args=(u'öô¿‽', u'ñě'),
+                  kwargs={'c': u'ßø'},
+                  priority=15,
+                  description=u"My dé^Wdescription")
+        job.user_id = 1
+        storage = OpenERPJobStorage(self.session)
+        storage.store(job)
+        job_read = storage.load(job.uuid)
+        self.assertEqual(job.args, job_read.args)
+        self.assertEqual(job_read.args, ('res.users', u'öô¿‽', u'ñě'))
+        self.assertEqual(job.kwargs, job_read.kwargs)
+        self.assertEqual(job_read.kwargs, {'c': u'ßø'})
+        self.assertEqual(job.description, job_read.description)
+        self.assertEqual(job_read.description, u"My dé^Wdescription")
+
+    def test_accented_bytestring(self):
+        job = Job(func=dummy_task_args,
+                  model_name='res.users',
+                  args=('öô¿‽', 'ñě'),
+                  kwargs={'c': 'ßø'},
+                  priority=15,
+                  description="My dé^Wdescription")
+        job.user_id = 1
+        storage = OpenERPJobStorage(self.session)
+        storage.store(job)
+        job_read = storage.load(job.uuid)
+        self.assertEqual(job.args, job_read.args)
+        self.assertEqual(job_read.args, ('res.users', 'öô¿‽', 'ñě'))
+        self.assertEqual(job.kwargs, job_read.kwargs)
+        self.assertEqual(job_read.kwargs, {'c': 'ßø'})
+        # the job's description has been created as bytestring but is
+        # decoded to utf8 by the ORM so make them comparable
+        self.assertEqual(job.description, job_read.description.encode('utf8'))
+        self.assertEqual(job_read.description, "My dé^Wdescription".decode('utf8'))
+
     def test_job_delay(self):
         self.cr.execute('delete from queue_job')
         deco_task = job(task_a)