From f085a67b745b07572b2a371bca028c227b9369a4 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 19 May 2017 13:43:49 +0200 Subject: [PATCH 1/9] [FIX] P3: columns initialisation order is unspecified With P3's hash randomisation, assuming one column is initialised before the other in the same model basically never works. --- addons/project_timesheet_holidays/models/res_company.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/addons/project_timesheet_holidays/models/res_company.py b/addons/project_timesheet_holidays/models/res_company.py index 3d21974573c..8dfe3ff580c 100644 --- a/addons/project_timesheet_holidays/models/res_company.py +++ b/addons/project_timesheet_holidays/models/res_company.py @@ -14,11 +14,8 @@ class Company(models.Model): 'project.task', string="Leave Task", domain="[('project_id', '=', leave_timesheet_project_id)]") - def _init_column(self, name): - super(Company, self)._init_column(name) - - if name == 'leave_timesheet_project_id': - self.search([('leave_timesheet_project_id', '=', False)])._create_leave_project_task() + def init(self): + self.search([('leave_timesheet_project_id', '=', False)])._create_leave_project_task() @api.model def create(self, values): From 132841695965307a3777728415a61e2beb002762 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 17 May 2017 11:04:10 +0200 Subject: [PATCH 2/9] [FIX] P3: hash randomisation in XML/HTML stuff * Have the cleaned up style attributes be dumped in the same order they were originally so that we can easily validate against text content. * While lxml's attributes are stored in an ordereddict, namespaces are not and thus when comparing text serialisation of namespaced documents the ordering of namespaces can differ between runs. Fix by comparing the LXML trees directly. --- odoo/addons/base/tests/test_mail_examples.py | 2 +- odoo/addons/base/tests/test_qweb.py | 135 +++++++++---------- odoo/addons/base/tests/test_views.py | 18 --- odoo/tests/common.py | 24 +++- odoo/tools/mail.py | 3 +- 5 files changed, 93 insertions(+), 89 deletions(-) diff --git a/odoo/addons/base/tests/test_mail_examples.py b/odoo/addons/base/tests/test_mail_examples.py index a83e668d940..dfecfe7c0d4 100644 --- a/odoo/addons/base/tests/test_mail_examples.py +++ b/odoo/addons/base/tests/test_mail_examples.py @@ -510,7 +510,7 @@ REMOVE_CLASS = """ """ REMOVE_CLASS_IN = [ - '
', + '
', 'An error occured in a modal and I will send you back the html to try opening one on your end'] REMOVE_CLASS_OUT = [ '
', diff --git a/odoo/addons/base/tests/test_qweb.py b/odoo/addons/base/tests/test_qweb.py index c331f426748..7a8f921af55 100644 --- a/odoo/addons/base/tests/test_qweb.py +++ b/odoo/addons/base/tests/test_qweb.py @@ -14,10 +14,6 @@ from odoo.addons.base.ir.ir_qweb import QWebException from odoo.tools import pycompat, misc, ustr -def dedent_and_strip(string): - return ''.join([line.strip() for line in string.splitlines()]) - - class TestQWebTField(TransactionCase): def setUp(self): super(TestQWebTField, self).setUp() @@ -30,13 +26,13 @@ class TestQWebTField(TransactionCase): result = self.engine.render(field, {'company': company}) self.assertEqual( - result, - '%s' % ( + etree.fromstring(result), + etree.fromstring(u'%s' % ( company.id, - "My Test Company", - ), + u"My Test Company", + )), ) def test_i18n(self): @@ -46,13 +42,13 @@ class TestQWebTField(TransactionCase): result = self.engine.render(field, {'company': company}) self.assertEqual( - ustr(result), - '%s' % ( + etree.fromstring(result), + etree.fromstring(u'%s' % ( company.id, misc.html_escape(s), - ), + )), ) def test_reject_crummy_tags(self): @@ -72,7 +68,7 @@ class TestQWebNS(TransactionCase): def test_render_static_xml_with_namespace(self): """ Test the rendering on a namespaced view with no static content. The resulting string should be untouched. """ - expected_result = """ + expected_result = u""" @@ -89,17 +85,17 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" %s """ % expected_result }) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), etree.fromstring(expected_result)) def test_render_static_xml_with_namespace_2(self): """ Test the rendering on a namespaced view with no static content. The resulting string should be untouched. """ - expected_result = """ + expected_result = u""" Book Review @@ -127,12 +123,12 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" %s """ % expected_result }) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), etree.fromstring(expected_result)) def test_render_static_xml_with_useless_distributed_namespace(self): """ Test that redundant namespaces are stripped upon rendering. @@ -140,7 +136,7 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -154,7 +150,7 @@ class TestQWebNS(TransactionCase): """ }) - expected_result = """ + expected_result = etree.fromstring(u""" @@ -163,24 +159,24 @@ class TestQWebNS(TransactionCase): - """ + """) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_static_xml_with_namespace_3(self): - expected_result = """ + expected_result = u""" """ view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" %s """ % expected_result }) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), etree.fromstring(expected_result)) def test_render_dynamic_xml_with_namespace_t_esc(self): """ Test that rendering a template containing a node having both an ns declaration and a t-esc attribute correctly @@ -189,16 +185,16 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" """ }) - expected_result = """test""" + expected_result = etree.fromstring(u"""test""") - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_dynamic_xml_with_namespace_t_esc_with_useless_distributed_namespace(self): """ Test that rendering a template containing a node having both an ns declaration and a t-esc attribute correctly @@ -207,7 +203,7 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" blabla @@ -216,13 +212,13 @@ class TestQWebNS(TransactionCase): """ }) - expected_result = """ + expected_result = etree.fromstring(u""" blabla - """ + """) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_dynamic_xml_with_namespace_t_attf(self): """ Test that rendering a template containing a node having both an ns declaration and a t-attf attribute correctly @@ -231,7 +227,7 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -248,7 +244,7 @@ class TestQWebNS(TransactionCase): """ }) - expected_result = """ + expected_result = etree.fromstring(u""" @@ -260,9 +256,9 @@ class TestQWebNS(TransactionCase): 80 - """ + """) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_dynamic_xml_with_namespace_t_attf_with_useless_distributed_namespace(self): """ Test that rendering a template containing a node having both an ns declaration and a t-attf attribute correctly @@ -271,7 +267,7 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -289,7 +285,7 @@ class TestQWebNS(TransactionCase): """ }) - expected_result = """ + expected_result = etree.fromstring(u""" @@ -302,15 +298,15 @@ class TestQWebNS(TransactionCase): - """ + """) - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_dynamic_xml_with_namespace_2(self): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -322,7 +318,7 @@ class TestQWebNS(TransactionCase): """ }) - expected_result = """ + expected_result = etree.fromstring(u""" 1.0 @@ -335,45 +331,45 @@ class TestQWebNS(TransactionCase): Oasis 4 - """ + """) - self.assertEquals(dedent_and_strip(view1.render(dict(version_id=1.0))), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render({'version_id': 1.0})), expected_result) def test_render_static_xml_with_namespaced_attributes(self): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" abc """ }) - expected_result = """abc""" + expected_result = etree.fromstring(u"""abc""") - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_dynamic_xml_with_namespaced_attributes(self): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" """ }) - expected_result = """abc""" + expected_result = etree.fromstring("""abc""") - self.assertEquals(dedent_and_strip(view1.render()), dedent_and_strip(expected_result)) + self.assertEqual(etree.fromstring(view1.render()), expected_result) def test_render_static_xml_with_t_call(self): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -386,13 +382,13 @@ class TestQWebNS(TransactionCase): """ }) self.env.cr.execute("INSERT INTO ir_model_data(name, model, res_id, module)" - "VALUES ('dummy', 'ir.ui.view', %s, 'base')" % view1.id) + "VALUES ('dummy', 'ir.ui.view', %s, 'base')", [view1.id]) # view2 will t-call view1 view2 = self.env['ir.ui.view'].create({ 'name': "dummy2", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -406,19 +402,19 @@ class TestQWebNS(TransactionCase): # check that the root tag has all its xmlns expected_ns = { - None: 'urn:oasis:names:specification:ubl:schema:xsd:Invoice-2', - 'cac': 'urn:oasis:names:specification:ubl:schema:xsd:CommonAggregateComponents-2', - 'cbc': 'urn:oasis:names:specification:ubl:schema:xsd:CommonBasicComponents-2' + (None, 'urn:oasis:names:specification:ubl:schema:xsd:Invoice-2'), + ('cac', 'urn:oasis:names:specification:ubl:schema:xsd:CommonAggregateComponents-2'), + ('cbc', 'urn:oasis:names:specification:ubl:schema:xsd:CommonBasicComponents-2'), } - self.assertSetEqual(set(pycompat.items(expected_ns)) - set(pycompat.items(result_etree.nsmap)), set()) + self.assertEqual(set(pycompat.items(result_etree.nsmap)), expected_ns) # check that the t-call did its work cac_lines = result_etree.findall('.//cac:line', namespaces={'cac': 'urn:oasis:names:specification:ubl:schema:xsd:CommonAggregateComponents-2'}) - self.assertEquals(len(cac_lines), 2) - self.assertEquals(result.count('Appel'), 2) + self.assertEqual(len(cac_lines), 2) + self.assertEqual(result.count(b'Appel'), 2) # check that the t-call dit not output again the xmlns declaration - self.assertEquals(result.count('xmlns:cbc="urn:oasis:names:specification:ubl:schema:xsd:CommonBasicComponents-2"'), 1) + self.assertEqual(result.count(b'xmlns:cbc="urn:oasis:names:specification:ubl:schema:xsd:CommonBasicComponents-2"'), 1) def test_render_static_xml_with_extension(self): """ Test the extension of a view by an xpath expression on a ns prefixed element. @@ -427,7 +423,7 @@ class TestQWebNS(TransactionCase): view1 = self.env['ir.ui.view'].create({ 'name': "dummy", 'type': 'qweb', - 'arch': """ + 'arch': u""" @@ -445,14 +441,14 @@ class TestQWebNS(TransactionCase): 'name': "dummy_ext", 'type': 'qweb', 'inherit_id': view1.id, - 'arch': """ + 'arch': u""" Oranges """ }) - expected_result = """ + expected_result = etree.fromstring(u""" @@ -462,9 +458,12 @@ class TestQWebNS(TransactionCase): - """ + """) - self.assertEquals(dedent_and_strip(view1.with_context(check_view_ids=[view1.id, view2.id]).render()), dedent_and_strip(expected_result)) + self.assertEqual( + etree.fromstring(view1.with_context(check_view_ids=[view1.id, view2.id]).render()), + expected_result + ) from copy import deepcopy diff --git a/odoo/addons/base/tests/test_views.py b/odoo/addons/base/tests/test_views.py index b9b8cd0fe39..3f007b5f71b 100644 --- a/odoo/addons/base/tests/test_views.py +++ b/odoo/addons/base/tests/test_views.py @@ -2,10 +2,6 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from functools import partial -try: - from itertools import zip_longest as izip_longuest -except ImportError: - from itertools import izip_longest from lxml import etree from lxml.builder import E @@ -28,22 +24,8 @@ class ViewXMLID(common.TransactionCase): class ViewCase(common.TransactionCase): def setUp(self): super(ViewCase, self).setUp() - self.addTypeEqualityFunc(etree._Element, self.assertTreesEqual) self.View = self.env['ir.ui.view'] - def assertTreesEqual(self, n1, n2, msg=None): - self.assertEqual(n1.tag, n2.tag, msg) - self.assertEqual((n1.text or '').strip(), (n2.text or '').strip(), msg) - self.assertEqual((n1.tail or '').strip(), (n2.tail or '').strip(), msg) - - # Because lxml uses ordereddicts in which order is important to - # equality (!?!?!?!) - self.assertEqual(dict(n1.attrib), dict(n2.attrib), msg) - - for c1, c2 in izip_longest(n1, n2): - self.assertEqual(c1, c2, msg) - - class TestNodeLocator(common.TransactionCase): """ The node locator returns None when it can not find a node, and the first diff --git a/odoo/tests/common.py b/odoo/tests/common.py index 823a56c6190..b0f601f284a 100644 --- a/odoo/tests/common.py +++ b/odoo/tests/common.py @@ -18,10 +18,15 @@ import itertools import unittest from contextlib import contextmanager from datetime import datetime, timedelta +from lxml import etree from pprint import pformat import requests +try: + from itertools import zip_longest as izip_longest +except ImportError: + from itertools import izip_longest try: from xmlrpc import client as xmlrpclib except ImportError: @@ -82,7 +87,24 @@ def post_install(flag): return obj return decorator -class BaseCase(unittest.TestCase): +class TreeCase(unittest.TestCase): + def __init__(self, methodName='runTest'): + super(TreeCase, self).__init__(methodName) + self.addTypeEqualityFunc(etree._Element, self.assertTreesEqual) + + def assertTreesEqual(self, n1, n2, msg=None): + self.assertEqual(n1.tag, n2.tag, msg) + # Because lxml.attrib is an ordereddict for which order is important + # to equality, even though *we* don't care + self.assertEqual(dict(n1.attrib), dict(n2.attrib), msg) + + self.assertEqual((n1.text or u'').strip(), (n2.text or u'').strip(), msg) + self.assertEqual((n1.tail or u'').strip(), (n2.tail or u'').strip(), msg) + + for c1, c2 in izip_longest(n1, n2): + self.assertEqual(c1, c2, msg) + +class BaseCase(TreeCase): """ Subclass of TestCase for common OpenERP-specific code. diff --git a/odoo/tools/mail.py b/odoo/tools/mail.py index 7fd6ca833bb..530f3e63862 100644 --- a/odoo/tools/mail.py +++ b/odoo/tools/mail.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. +import collections import logging from lxml.html import clean import random @@ -147,7 +148,7 @@ class _Cleaner(clean.Cleaner): attributes = el.attrib styling = attributes.get('style') if styling: - valid_styles = {} + valid_styles = collections.OrderedDict() styles = self._style_re.findall(styling) for style in styles: if style[0].lower() in self._style_whitelist: From 1d579221d7e5a33add17e1ef7c973c6c4d8db850 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 17 May 2017 11:06:11 +0200 Subject: [PATCH 3/9] [FIX] P3: bypass hash randomisation in modifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compare Python dict to Python dict, no reason to dump the generated modifiers to JSON before comparing them to JSON strings… --- odoo/osv/orm.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/odoo/osv/orm.py b/odoo/osv/orm.py index 368e60cd255..e4be2706c7f 100644 --- a/odoo/osv/orm.py +++ b/odoo/osv/orm.py @@ -129,32 +129,30 @@ def test_modifiers(what, expected): node = etree.fromstring(what) transfer_node_to_modifiers(node, modifiers) simplify_modifiers(modifiers) - dump = json.dumps(modifiers) - assert dump == expected, "%s != %s" % (dump, expected) + assert modifiers == expected, "%s != %s" % (modifiers, expected) elif isinstance(what, dict): transfer_field_to_modifiers(what, modifiers) simplify_modifiers(modifiers) - dump = json.dumps(modifiers) - assert dump == expected, "%s != %s" % (dump, expected) + assert modifiers == expected, "%s != %s" % (modifiers, expected) # To use this test: # import odoo # odoo.osv.orm.modifiers_tests() def modifiers_tests(): - test_modifiers('', '{}') - test_modifiers('', '{"invisible": true}') - test_modifiers('', '{"readonly": true}') - test_modifiers('', '{"required": true}') - test_modifiers('', '{}') - test_modifiers('', '{}') - test_modifiers('', '{}') - test_modifiers('', '{"invisible": true, "required": true}') # TODO order is not guaranteed - test_modifiers('', '{"invisible": true}') - test_modifiers('', '{"required": true}') - test_modifiers("""""", '{"invisible": [["b", "=", "c"]]}') + test_modifiers('', {}) + test_modifiers('', {"invisible": True}) + test_modifiers('', {"readonly": True}) + test_modifiers('', {"required": True}) + test_modifiers('', {}) + test_modifiers('', {}) + test_modifiers('', {}) + test_modifiers('', {"invisible": True, "required": True}) # TODO order is not guaranteed + test_modifiers('', {"invisible": True}) + test_modifiers('', {"required": True}) + test_modifiers("""""", {"invisible": [["b", "=", "c"]]}) # The dictionary is supposed to be the result of fields_get(). - test_modifiers({}, '{}') - test_modifiers({"invisible": True}, '{"invisible": true}') - test_modifiers({"invisible": False}, '{}') + test_modifiers({}, {}) + test_modifiers({"invisible": True}, {"invisible": True}) + test_modifiers({"invisible": False}, {}) From b62ba7ee1adc8e3c41040cafde30c2c93154ce69 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 19 May 2017 17:01:42 +0200 Subject: [PATCH 4/9] [FIX] P3: initialisation order of columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Python 3's hash randomisation means columns can be iterated — and thus initialised — in any order (within a given model/override anyway), which means there can be an implicit dependency on column initialisation order e.g. (res.company).currency_id has a default which fetches the currency_id of the company_id of the administrator, and res.company has a _order=sequence,name, if the currency_id field is created before the sequence the SELECT query will blow up as the sequence column will not exist yet. Attempt to always manipulate (iterate & initialize) fields based on their order of definition using a global sequence number. --- odoo/addons/base/res/res_company.py | 2 +- odoo/fields.py | 6 +++++- odoo/models.py | 12 ++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/odoo/addons/base/res/res_company.py b/odoo/addons/base/res/res_company.py index 9dfdba96cba..98eff189f6e 100644 --- a/odoo/addons/base/res/res_company.py +++ b/odoo/addons/base/res/res_company.py @@ -27,6 +27,7 @@ class Company(models.Model): return currency_id or self._get_euro() name = fields.Char(related='partner_id.name', string='Company Name', required=True, store=True) + sequence = fields.Integer(help='Used to order Companies in the company switcher', default=10) parent_id = fields.Many2one('res.company', string='Parent Company', index=True) child_ids = fields.One2many('res.company', 'parent_id', string='Child Companies') partner_id = fields.Many2one('res.partner', string='Partner', required=True) @@ -52,7 +53,6 @@ class Company(models.Model): website = fields.Char(related='partner_id.website') vat = fields.Char(related='partner_id.vat', string="TIN") company_registry = fields.Char() - sequence = fields.Integer(help='Used to order Companies in the company switcher', default=10) paperformat_id = fields.Many2one('report.paperformat', 'Paper format', default=lambda self: self.env.ref('report.paperformat_euro', raise_if_not_found=False)) external_report_layout = fields.Selection([ ('background', 'Background'), diff --git a/odoo/fields.py b/odoo/fields.py index fec4666e925..37c57dcd5c4 100644 --- a/odoo/fields.py +++ b/odoo/fields.py @@ -7,7 +7,9 @@ from collections import OrderedDict, defaultdict from datetime import date, datetime from functools import partial from operator import attrgetter +import itertools import logging + import pytz try: from xmlrpc.client import MAXINT @@ -119,7 +121,7 @@ class MetaField(type): elif attr.startswith('_description_'): cls.description_attrs.append((attr[13:], attr)) - +_global_seq = iter(itertools.count()) class Field(MetaField('DummyField', (object,), {})): """ The field descriptor contains the field definition, and manages accesses and assignments of the corresponding field on records. The following @@ -296,6 +298,7 @@ class Field(MetaField('DummyField', (object,), {})): '_attrs': EMPTY_DICT, # the field's non-slot attributes '_module': None, # the field's module name '_setup_done': None, # the field's setup state: None, 'base' or 'full' + '_sequence': None, # absolute ordering of the field 'automatic': False, # whether the field is automatically created ("magic" field) 'inherited': False, # whether the field is inherited (_inherits) @@ -336,6 +339,7 @@ class Field(MetaField('DummyField', (object,), {})): def __init__(self, string=Default, **kwargs): kwargs['string'] = string + self._sequence = kwargs['_sequence'] = next(_global_seq) args = {key: val for key, val in pycompat.items(kwargs) if val is not Default} self.args = args or EMPTY_DICT self._setup_done = None diff --git a/odoo/models.py b/odoo/models.py index 97fc5976572..8a231a756ad 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -425,7 +425,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): '_original_module': cls._module, '_inherit_children': OrderedSet(), # names of children models '_inherits_children': set(), # names of children models - '_fields': {}, # populated in _setup_base() + '_fields': OrderedDict(), # populated in _setup_base() }) check_parent = cls._build_model_check_parent @@ -2272,7 +2272,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): # therefore two registries are never set up at the same time. # remove fields that are not proper to cls - for name in set(cls._fields) - cls0._proper_fields: + for name in OrderedSet(cls._fields) - cls0._proper_fields: delattr(cls, name) cls._fields.pop(name, None) # collect proper fields on cls0, and add them on cls @@ -2283,20 +2283,20 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): self._add_field(name, field) else: self._add_field(name, field.new(**field.args)) - cls._proper_fields = set(cls._fields) + cls._proper_fields = OrderedSet(cls._fields) else: # retrieve fields from parent classes, and duplicate them on cls to # avoid clashes with inheritance between different models for name in cls._fields: delattr(cls, name) - cls._fields = {} - for name, field in getmembers(cls, Field.__instancecheck__): + cls._fields = OrderedDict() + for name, field in sorted(getmembers(cls, Field.__instancecheck__), key=lambda f: f[1]._sequence): # do not retrieve magic, custom and inherited fields if not any(field.args.get(k) for k in ('automatic', 'manual', 'inherited')): self._add_field(name, field.new()) self._add_magic_fields() - cls._proper_fields = set(cls._fields) + cls._proper_fields = OrderedSet(cls._fields) cls.pool.model_cache[cls._model_cache_key] = cls From a5283f90ce2cc8145ce536ab29984e1ea02a085c Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 26 May 2017 17:25:04 +0200 Subject: [PATCH 5/9] [FIX] P3: QueryURL relying on kw iteration order --- addons/website/controllers/main.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/addons/website/controllers/main.py b/addons/website/controllers/main.py index 2f97218ce72..e0eb3c34c3a 100644 --- a/addons/website/controllers/main.py +++ b/addons/website/controllers/main.py @@ -20,7 +20,7 @@ from odoo.http import request from odoo.addons.website.models.website import slug from odoo.addons.web.controllers.main import WebClient, Binary, Home -from odoo.tools import pycompat +from odoo.tools import pycompat, OrderedSet logger = logging.getLogger(__name__) @@ -34,27 +34,29 @@ class QueryURL(object): def __init__(self, path='', path_args=None, **args): self.path = path self.args = args - self.path_args = set(path_args or []) + self.path_args = OrderedSet(path_args or []) def __call__(self, path=None, path_args=None, **kw): path = path or self.path for key, value in pycompat.items(self.args): kw.setdefault(key, value) - path_args = set(path_args or []).union(self.path_args) - paths, fragments = [], [] + path_args = OrderedSet(path_args or []) | self.path_args + paths, fragments = {}, [] for key, value in pycompat.items(kw): if value and key in path_args: if isinstance(value, models.BaseModel): - paths.append((key, slug(value))) + paths[key] = slug(value) else: - paths.append((key, value)) + paths[key] = u"%s" % value elif value: if isinstance(value, list) or isinstance(value, set): fragments.append(werkzeug.url_encode([(key, item) for item in value])) else: fragments.append(werkzeug.url_encode([(key, value)])) - for key, value in paths: - path += '/' + key + '/%s' % value + for key in path_args: + value = paths.get(key) + if value is not None: + path += '/' + key + '/' + value if fragments: path += '?' + '&'.join(fragments) return path From ba1b88db34333d65e0a6abdbc7904a03baaa361b Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 29 May 2017 16:51:44 +0200 Subject: [PATCH 6/9] [FIX] P3: sales tests relying on dict iteration order test_00_sale_stock_invoice would blow up in Python 3, because the test(s) relies on the iteration order of the products dict to know where which line is (holy mother of caca) an set the relevant line's data by indexing directly into it. This would fail (with very odd results) in Python 2 and 3 as it could set information on the "wrong" order line aka the order line for a different product (and invoicing mode/whatever) than intended. Fix the issue by hard-coding the products iteration order (to the iteration order I get in Python 2.7.10 on that dict literal) using an OrderedDict. --- addons/sale/tests/test_sale_common.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/addons/sale/tests/test_sale_common.py b/addons/sale/tests/test_sale_common.py index 85ecdeac108..ebea8f85c5f 100644 --- a/addons/sale/tests/test_sale_common.py +++ b/addons/sale/tests/test_sale_common.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. - +from collections import OrderedDict from odoo.addons.account.tests.account_test_classes import AccountingTestCase @@ -27,11 +27,11 @@ class TestSale(AccountingTestCase): 'groups_id': [(6, 0, [group_user.id])] }) # create quotation with differend kinds of products (all possible combinations) - self.products = { - 'prod_order': self.env.ref('product.product_order_01'), - 'prod_del': self.env.ref('product.product_delivery_01'), - 'serv_order': self.env.ref('product.service_order_01'), - 'serv_del': self.env.ref('product.service_delivery'), - } + self.products = OrderedDict([ + ('prod_order', self.env.ref('product.product_order_01')), + ('serv_del', self.env.ref('product.service_delivery')), + ('serv_order', self.env.ref('product.service_order_01')), + ('prod_del', self.env.ref('product.product_delivery_01')), + ]) self.partner = self.env.ref('base.res_partner_1') From ea21c7a8ee5a62bd22d965eb861844ec4e884f75 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 2 Jun 2017 16:25:08 +0200 Subject: [PATCH 7/9] [FIX] P3: try to write fields in source order When adding a Python constraint on a (relational) field which uses an other (relational) field, the order in which the fields are written is also the order in which they are checked, and if a constraint depends on a sibling field this might blow up in P3, here as in as in test_new_api.message.Message (_check_author constraint depends on parent discussion's participants): test_11_stored would commonly fail when creating a discussion with two participants and 3 messages (because the message's authors would be checked before the participants had been associated with the discussion). --- odoo/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/odoo/models.py b/odoo/models.py index 8a231a756ad..71040650173 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -3423,7 +3423,7 @@ class BaseModel(MetaModel('DummyModel', (object,), {'_register': False})): if not key.startswith('default_')} # call the 'write' method of fields which are not columns - for name in upd_todo: + for name in sorted(upd_todo, key=lambda name: self._fields[name]._sequence): field = self._fields[name] field.write(self.with_context(rel_context), vals[name]) From 4957296f1d0113aa074015ccd14f7e96918ba3cb Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 2 Jun 2017 16:31:58 +0200 Subject: [PATCH 8/9] [FIX] P3: incorrect dependencies website_sale_digital overrides a method defined on website_portal_sale, but only depends on website_portal and extends the base controller from website_portal. Depending on the (relative) order in which the system decides to load website_sale_digital and website_portal_sale we can end up with the former's template but the latter's methods (without the proper context data). Which ends up commonly crashing in P35. Fix dependencies and controller inheritance. --- addons/website_sale_digital/__manifest__.py | 2 +- addons/website_sale_digital/controllers/main.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/addons/website_sale_digital/__manifest__.py b/addons/website_sale_digital/__manifest__.py index 973a3bd1817..84018a35c61 100644 --- a/addons/website_sale_digital/__manifest__.py +++ b/addons/website_sale_digital/__manifest__.py @@ -8,7 +8,7 @@ Sell digital product using attachments to virtual products """, 'depends': [ 'document', - 'website_sale', + 'website_portal_sale', ], 'installable': True, 'data': [ diff --git a/addons/website_sale_digital/controllers/main.py b/addons/website_sale_digital/controllers/main.py index 162daa5c0c8..2df86980bc6 100644 --- a/addons/website_sale_digital/controllers/main.py +++ b/addons/website_sale_digital/controllers/main.py @@ -7,7 +7,7 @@ from werkzeug.utils import redirect from odoo import http from odoo.http import request -from odoo.addons.website_portal.controllers.main import website_account +from odoo.addons.website_portal_sale.controllers.main import website_account from odoo.addons.website_sale.controllers.main import WebsiteSale class WebsiteSaleDigitalConfirmation(WebsiteSale): From 5654a1ab95a8d526c6e7c15905962e91f16fa863 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 12 Jun 2017 10:06:32 +0200 Subject: [PATCH 9/9] [ADD] P3: hash randomisation docs --- doc/python3.rst | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/doc/python3.rst b/doc/python3.rst index ea14b6e4515..0b358ac9e3e 100644 --- a/doc/python3.rst +++ b/doc/python3.rst @@ -61,6 +61,35 @@ features whereas: generator delegation, pathlib, ...), you must *not *use them in Odoo until Python 2 support is dropped +Semantics changes +================= + +Dict & set iteration order ("Hash Randomisation") +------------------------------------------------- + +In Python 2, the iteration order depends on the value's hash (modulo the +collection's capacity and conflict resolution), which provides a +spec-undefined but implementation-defined order. While that's not supposed to +happen, it turns out code may depend on the specific order of iteration over +a hash collection (``dict`` or ``set``). + +Python 3.3 enables `hash randomisation`_ by default (this can be optionally +enabled on previous versions including Python 2 by providing the ``-R`` +command-line parameter), which means *the order of iteration changes from one +run to the next*. + +When discovered, this can be fixed by one of: + +* making iteration steps properly independent (removing the dependency of + order of iteration) +* using different checking method (e.g. when serialising sets or dictionaries + and checking against the specific serialised value) +* fixing dependencies +* using a ``collections.OrderedDict`` or ``odoo.tools.misc.OrderedSet`` instead + of a regular one, they guarantee order of iteration is order of insertion +* sorting the collection's items before iterating over them (this may require + adding some sort of iteration key to the items) + Moved and removed ================= @@ -340,6 +369,8 @@ Minor syntax changes error, octal literals now follow the hexadecimal convention with a ``0o`` prefix. +.. _hash randomisation: http://bugs.python.org/issue13703 + .. _requests: http://docs.python-requests.org/ .. _werkzeug: http://werkzeug.pocoo.org/docs/urls/