From 0f71c64237b313fad0a261f4dd652bdc9459d995 Mon Sep 17 00:00:00 2001 From: william Date: Wed, 29 Jun 2022 10:45:00 +0000 Subject: [PATCH] [FIX] account: reduce locking on moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By trying to avoid concurrency issues while assigning the sequence [1], we are locking too many operations even when not in the process of assigning a new number. * when computing `highest_name`, we don't need to lock as this is mostly an information on the form that can possibly change anyway while the form view is open (without any lock anymore) * when computing the accounting date, we don't need to lock, the only information we want is the format of the sequence (yearly, monthly, no reset), which is not likely to change on a same journal. * when changing the `journal_id`, it is only a helper for encoding, if there is a concurrency error it will be shown during the post anyways. Also same argument as for `highest_name` [1] https://github.com/odoo/odoo/commit/ba6ee0b75d66d6f22a592b1b6a7c5158461bec41 closes odoo/odoo#95480 X-original-commit: 809557cdd02fe6581ed573b58158a927aabe0694 Related: odoo/enterprise#29212 Signed-off-by: Olivier Colson (oco) Signed-off-by: William André (wan) --- addons/account/models/account_move.py | 8 +- addons/account/models/sequence_mixin.py | 24 +-- addons/account/tests/test_sequence_mixin.py | 170 +++++++++++++------- addons/l10n_ar/models/account_move.py | 1 - 4 files changed, 126 insertions(+), 77 deletions(-) diff --git a/addons/account/models/account_move.py b/addons/account/models/account_move.py index 763e6aaf28a..31042561142 100644 --- a/addons/account/models/account_move.py +++ b/addons/account/models/account_move.py @@ -470,7 +470,7 @@ class AccountMove(models.Model): """ lock_dates = self._get_violated_lock_dates(invoice_date, has_tax) today = fields.Date.today() - highest_name = self.highest_name or self._get_last_sequence(relaxed=True) + highest_name = self.highest_name or self._get_last_sequence(relaxed=True, lock=False) number_reset = self._deduce_sequence_number_reset(highest_name) if lock_dates: invoice_date = lock_dates[-1][0] + timedelta(days=1) @@ -546,7 +546,7 @@ class AccountMove(models.Model): if new_currency != self.currency_id: self.currency_id = new_currency self._onchange_currency() - if self.state == 'draft' and self.name and self.name != '/' and not self.quick_edit_mode and self._get_last_sequence(): + if self.state == 'draft' and self.name and self.name != '/' and not self.quick_edit_mode and self._get_last_sequence(lock=False): self.name = '/' @api.onchange('partner_id') @@ -1318,7 +1318,7 @@ class AccountMove(models.Model): ) ) self = self.sorted(lambda m: (m.date, m.ref or '', m.id)) - highest_name = self[0]._get_last_sequence() if self else False + highest_name = self[0]._get_last_sequence(lock=False) if self else False # Group the moves by journal and month for move in self: @@ -1381,7 +1381,7 @@ class AccountMove(models.Model): @api.depends('journal_id', 'date') def _compute_highest_name(self): for record in self: - record.highest_name = record._get_last_sequence() + record.highest_name = record._get_last_sequence(lock=False) @api.depends('journal_id.type', 'company_id.quick_edit_mode', 'quick_edit_total_amount') def _compute_quick_edit_mode(self): diff --git a/addons/account/models/sequence_mixin.py b/addons/account/models/sequence_mixin.py index 0425f34c6a0..7adf8491cb0 100644 --- a/addons/account/models/sequence_mixin.py +++ b/addons/account/models/sequence_mixin.py @@ -133,7 +133,7 @@ class SequenceMixin(models.AbstractModel): self.ensure_one() return "00000000" - def _get_last_sequence(self, relaxed=False, with_prefix=None): + def _get_last_sequence(self, relaxed=False, with_prefix=None, lock=True): """Retrieve the previous sequence. This is done by taking the number with the greatest alphabetical value within @@ -165,20 +165,22 @@ class SequenceMixin(models.AbstractModel): where_string += " AND sequence_prefix = %(with_prefix)s " param['with_prefix'] = with_prefix - query = """ - UPDATE {table} SET write_date = write_date WHERE id = ( - SELECT id FROM {table} + query = f""" + SELECT {{field}} FROM {self._table} {where_string} - AND sequence_prefix = (SELECT sequence_prefix FROM {table} {where_string} ORDER BY id DESC LIMIT 1) + AND sequence_prefix = (SELECT sequence_prefix FROM {self._table} {where_string} ORDER BY id DESC LIMIT 1) ORDER BY sequence_number DESC LIMIT 1 + """ + if lock: + query = f""" + UPDATE {self._table} SET write_date = write_date WHERE id = ( + {query.format(field='id')} ) - RETURNING {field}; - """.format( - table=self._table, - where_string=where_string, - field=self._sequence_field, - ) + RETURNING {self._sequence_field}; + """ + else: + query = query.format(field=self._sequence_field) self.flush_model([self._sequence_field, 'sequence_number', 'sequence_prefix']) self.env.cr.execute(query, param) diff --git a/addons/account/tests/test_sequence_mixin.py b/addons/account/tests/test_sequence_mixin.py index 714706e47ff..7aa582551b4 100644 --- a/addons/account/tests/test_sequence_mixin.py +++ b/addons/account/tests/test_sequence_mixin.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- from odoo.addons.account.tests.common import AccountTestInvoicingCommon from odoo.tests import tagged -from odoo.tests.common import Form +from odoo.tests.common import Form, TransactionCase from odoo import fields, api, SUPERUSER_ID from odoo.exceptions import ValidationError, UserError from odoo.tools import mute_logger @@ -349,53 +349,6 @@ class TestSequenceMixin(TestSequenceMixinCommon): reset_never = set_sequence(self.test_move.date + relativedelta(years=2), 'MISC/00100') test_date(reset_never.date, 'MISC/00101') # Keep the new prefix in 2018 - def test_sequence_concurency(self): - """Computing the same name in concurent transactions is not allowed.""" - with self.env.registry.cursor() as cr0,\ - self.env.registry.cursor() as cr1,\ - self.env.registry.cursor() as cr2: - env0 = api.Environment(cr0, SUPERUSER_ID, {}) - env1 = api.Environment(cr1, SUPERUSER_ID, {}) - env2 = api.Environment(cr2, SUPERUSER_ID, {}) - - journal = env0['account.journal'].create({ - 'name': 'concurency_test', - 'code': 'CT', - 'type': 'general', - }) - account = env0['account.account'].create({ - 'code': 'CT', - 'name': 'CT', - 'user_type_id': env0.ref('account.data_account_type_fixed_assets').id, - }) - moves = env0['account.move'].create([{ - 'journal_id': journal.id, - 'date': fields.Date.from_string('2016-01-01'), - 'line_ids': [(0, 0, {'name': 'name', 'account_id': account.id})] - }] * 3) - moves.name = '/' - moves[0].action_post() - self.assertEqual(moves.mapped('name'), ['CT/2016/01/0001', '/', '/']) - env0.cr.commit() - - # start the transactions here on cr2 to simulate concurency with cr1 - env2.cr.execute('SELECT 1') - - move = env1['account.move'].browse(moves[1].id) - move.action_post() - env1.cr.commit() - - move = env2['account.move'].browse(moves[2].id) - with self.assertRaises(psycopg2.OperationalError), env2.cr.savepoint(), mute_logger('odoo.sql_db'): - move.action_post() - - self.assertEqual(moves.mapped('name'), ['CT/2016/01/0001', 'CT/2016/01/0002', '/']) - moves.button_draft() - moves.with_context(force_delete=True).unlink() - journal.unlink() - account.unlink() - env0.cr.commit() - def test_resequence_clash(self): """Resequence doesn't clash when it uses a name set in the same batch but that will be overriden later.""" @@ -415,6 +368,20 @@ class TestSequenceMixin(TestSequenceMixinCommon): 'first_name': '2', }).resequence() + @freeze_time('2021-10-01 00:00:00') + def test_change_journal_on_first_account_move(self): + """Changing the journal on the first move is allowed""" + journal = self.env['account.journal'].create({ + 'name': 'awesome journal', + 'type': 'general', + 'code': 'AJ', + }) + move = self.env['account.move'].create({}) + self.assertEqual(move.name, 'MISC/2021/10/0001') + with Form(move) as move_form: + move_form.journal_id = journal + self.assertEqual(move.name, 'AJ/2021/10/0001') + @tagged('post_install', '-at_install') class TestSequenceMixinDeletion(TestSequenceMixinCommon): @@ -476,16 +443,97 @@ class TestSequenceMixinDeletion(TestSequenceMixinCommon): with self.assertRaises(UserError): all_moves.unlink() - @freeze_time('2021-10-01 00:00:00') - def test_change_journal_on_first_account_move(self): - """Changing the journal on the first move is allowed""" - journal = self.env['account.journal'].create({ - 'name': 'awesome journal', - 'type': 'general', - 'code': 'AJ', - }) - move = self.env['account.move'].create({}) - self.assertEqual(move.name, 'MISC/2021/10/0001') - with Form(move) as move_form: - move_form.journal_id = journal - self.assertEqual(move.name, 'AJ/2021/10/0001') + +@tagged('post_install', '-at_install') +class TestSequenceMixinConcurrency(TransactionCase): + def setUp(self): + super().setUp() + with self.env.registry.cursor() as cr: + env = api.Environment(cr, SUPERUSER_ID, {}) + journal = env['account.journal'].create({ + 'name': 'concurency_test', + 'code': 'CT', + 'type': 'general', + }) + account = env['account.account'].create({ + 'code': 'CT', + 'name': 'CT', + 'user_type_id': env.ref('account.data_account_type_fixed_assets').id, + }) + moves = env['account.move'].create([{ + 'journal_id': journal.id, + 'date': fields.Date.from_string('2016-01-01'), + 'line_ids': [(0, 0, {'name': 'name', 'account_id': account.id})] + }] * 3) + moves.name = '/' + moves[0].action_post() + self.assertEqual(moves.mapped('name'), ['CT/2016/01/0001', '/', '/']) + env.cr.commit() + self.data = { + 'move_ids': moves.ids, + 'account_id': account.id, + 'journal_id': journal.id, + 'envs': [ + api.Environment(self.env.registry.cursor(), SUPERUSER_ID, {}), + api.Environment(self.env.registry.cursor(), SUPERUSER_ID, {}), + api.Environment(self.env.registry.cursor(), SUPERUSER_ID, {}), + ], + } + self.addCleanup(self.cleanUp) + + def cleanUp(self): + with self.env.registry.cursor() as cr: + env = api.Environment(cr, SUPERUSER_ID, {}) + moves = env['account.move'].browse(self.data['move_ids']) + moves.button_draft() + moves.posted_before = False + moves.unlink() + journal = env['account.journal'].browse(self.data['journal_id']) + journal.unlink() + account = env['account.account'].browse(self.data['account_id']) + account.unlink() + env.cr.commit() + for env in self.data['envs']: + env.cr.close() + + def test_sequence_concurency(self): + """Computing the same name in concurent transactions is not allowed.""" + env0, env1, env2 = self.data['envs'] + + # start the transactions here on cr1 to simulate concurency with cr2 + env1.cr.execute('SELECT 1') + + # post in cr2 + move = env2['account.move'].browse(self.data['move_ids'][1]) + move.action_post() + env2.cr.commit() + + # try to post in cr1, should fail because this transaction started before the post in cr2 + move = env1['account.move'].browse(self.data['move_ids'][2]) + with self.assertRaises(psycopg2.OperationalError), mute_logger('odoo.sql_db'): + move.action_post() + + # check the values + moves = env0['account.move'].browse(self.data['move_ids']) + self.assertEqual(moves.mapped('name'), ['CT/2016/01/0001', 'CT/2016/01/0002', '/']) + + def test_sequence_concurency_no_useless_lock(self): + """Do not lock needlessly when the sequence is not computed""" + env0, env1, env2 = self.data['envs'] + + # start the transactions here on cr1 to simulate concurency with cr2 + env1.cr.execute('SELECT 1') + + # get the last sequence in cr1 (for instance opening a form view) + move = env2['account.move'].browse(self.data['move_ids'][1]) + move.highest_name + env2.cr.commit() + + # post in cr1, should work even though cr2 read values + move = env1['account.move'].browse(self.data['move_ids'][2]) + move.action_post() + env1.cr.commit() + + # check the values + moves = env0['account.move'].browse(self.data['move_ids']) + self.assertEqual(moves.mapped('name'), ['CT/2016/01/0001', '/', 'CT/2016/01/0002']) diff --git a/addons/l10n_ar/models/account_move.py b/addons/l10n_ar/models/account_move.py index ff748f505d3..ffc5cd45f02 100644 --- a/addons/l10n_ar/models/account_move.py +++ b/addons/l10n_ar/models/account_move.py @@ -240,7 +240,6 @@ class AccountMove(models.Model): return self._get_formatted_sequence() return super()._get_starting_sequence() - def _get_last_sequence_domain(self, relaxed=False): where_string, param = super(AccountMove, self)._get_last_sequence_domain(relaxed) if self.company_id.account_fiscal_country_id.code == "AR" and self.l10n_latam_use_documents: