[FIX] account: reduce locking on moves

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) <oco@odoo.com>
Signed-off-by: William André (wan) <wan@odoo.com>

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):

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)

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'])

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: