[FIX] web,onboarding: fix multiple progress records

and better handle progress requests.

In previous versions, it sometimes occurred that multiple
onboarding_progress records were created despite our measures
against that.

Two problems were identified and fixed here, together with a simple
mean to handle this error on the client-side.

1. `_sql_constraints` entry was not adequate to prevent multiple
  records from being created for onboardings which are not to be
  completed per-company but per database because in our version(s)
  of PostgreSQL `NULL` values cannot be considered `NOT DISTINCT`,
  i.e., all NULL values are different from one another so `UNIQUE`
  is respected when we wouldn't want it.
  Therefore, we implement here a `UNIQUE` index via the model's init,
  allowing to catch unique violations when company_id is not set.

2. While we search if a record exists before creating a new one, it
  could happen that another worker created a record between these
  steps. As the database is configured (`REPEATABLE_READ`, which
  applies until the end of the transaction, when we leave the
  controller), it is very difficult to retrieve the record created by
  the other process. See additional details in the testing part below.
  Therefore, we chose to ask the client to perform a new request if
  desirable.

Existing extra records are removed with the related UPG PR script.

### Tests

Tests are included to make sure it is not possible to create multiple
onboarding_progress records for the same onboarding and company or
without any. This includes a non-standard (runbot will not run it)
and "database breaking" (we try to clean up our mess but we cannot
give any guarantee) concurrent test that reproduce the following
scenario:

The `/onboarding/<string:route_name>` is called twice in two different
workers for a same `route_name`. The corresponding onboarding exists
and has no corresponding `onboarding.progress` record yet. Python side,
the two workers are oblivious of what's happening in the transaction of
the other worker thus any python-side-only unicity constraint would
fail to detect any progress created by other workers. This would end up
with two progress records created for a single onboarding which is
illegal.

Task-3101666

closes odoo/odoo#109221

Related: odoo/upgrade#4142
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
Co-authored-by: Julien Castiaux <juc@odoo.com>

addons/onboarding/controllers/onboarding.py

@@ -1,6 +1,8 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
+from psycopg2 import IntegrityError
+
 from odoo import http
 from odoo.http import request
 
@@ -12,11 +14,17 @@ class OnboardingController(http.Controller):
             return {}
 
         onboarding = request.env['onboarding.onboarding'].search([('route_name', '=', route_name)])
-        if onboarding and not onboarding._search_or_create_progress().is_onboarding_closed:
-            # JS implementation of the onboarding panel expects this data structure
-            return {
-                'html': request.env['ir.qweb']._render(
-                    'onboarding.onboarding_panel', onboarding._prepare_rendering_values())
-            }
+        if onboarding:
+            try:
+                progress = onboarding._search_or_create_progress()
+            except IntegrityError:  # Another worker created the record at the same time
+                return {'code': 503}  # Temporarily unavailable - Invites client to try again
+
+            if not progress.is_onboarding_closed:
+                # JS implementation of the onboarding panel expects this data structure
+                return {
+                    'html': request.env['ir.qweb']._render(
+                        'onboarding.onboarding_panel', onboarding._prepare_rendering_values())
+                }
 
         return {}

addons/onboarding/models/onboarding_progress.py

@@ -23,10 +23,13 @@ class OnboardingProgress(models.Model):
     onboarding_id = fields.Many2one(
         'onboarding.onboarding', 'Related onboarding tracked', required=True, ondelete='cascade')
     progress_step_ids = fields.One2many('onboarding.progress.step', 'progress_id', 'Progress Steps Trackers')
-    _sql_constraints = [
-        ('onboarding_company_uniq', 'unique (onboarding_id,company_id)',
-         'There cannot be multiple records of the same onboarding completion for the same company.'),
-    ]
+
+    def init(self):
+        # not in _sql_constraint because COALESCE is not supported for PostgreSQL constraint
+        self.env.cr.execute("""
+            CREATE UNIQUE INDEX IF NOT EXISTS onboarding_progress_onboarding_company_uniq
+            ON onboarding_progress (onboarding_id, COALESCE(company_id, 0))
+        """)
 
     @api.depends('onboarding_id.step_ids', 'progress_step_ids', 'progress_step_ids.step_state')
     def _compute_onboarding_state(self):

addons/onboarding/tests/__init__.py

@@ -2,3 +2,4 @@
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
 from . import test_onboarding
+from . import test_onboarding_concurrency

addons/onboarding/tests/test_onboarding.py

@@ -1,7 +1,10 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
+from psycopg2 import IntegrityError
+
 from odoo.addons.onboarding.tests.common import TestOnboardingCommon
+from odoo.tools import mute_logger
 
 
 class TestOnboarding(TestOnboardingCommon):
@@ -131,36 +134,34 @@ class TestOnboarding(TestOnboardingCommon):
 
         self.assert_onboarding_is_not_done(self.onboarding_1)
 
-    def test_no_crash_on_multiple_progress_records(self):
-        existing_progress = self.env['onboarding.progress'].search([
-            ('onboarding_id', '=', self.onboarding_1.id), ('company_id', '=', False)
-        ])
-        self.assertEqual(len(existing_progress), 1)
+    @mute_logger('odoo.sql_db')
+    def test_progress_no_company_uniqueness(self):
+        """Check that there cannot be two progress records created for
+        the same onboarding when it is configured to be completed only
+        once for the whole db and not per-company (is_per_company=False).
+        NB: Postgresql UNIQUE constraint failures raise IntegrityErrors.
+        """
+        self.assertFalse(self.onboarding_1.current_progress_id.company_id)
+        with self.assertRaises(IntegrityError):
+            self.env['onboarding.progress'].create({
+                'onboarding_id': self.onboarding_1.id,
+                'company_id': False
+            })
 
-        extra_progress = self.env['onboarding.progress'].create({
-            'onboarding_id': self.onboarding_1.id,
-            'company_id': False
-        })
+    @mute_logger('odoo.sql_db')
+    def test_progress_per_company_uniqueness(self):
+        """Check that there cannot be two progress records created for
+        the same company and the same onboarding when the onboarding is
+        configured to be completed per-company.
+        See also ``test_progress_no_company_uniqueness``
+        """
+        # Updating onboarding to per-company
+        self.onboarding_1.is_per_company = True
+        # Required after progress reset (simulate role of controller)
+        self.onboarding_1._search_or_create_progress()
 
-        self.env['onboarding.progress.step'].create([{
-            'step_id': self.onboarding_1_step_1.id,
-            'progress_id': progress.id
-            } for progress in (existing_progress, extra_progress)
-        ])
-
-        nb_progress = self.env['onboarding.progress'].search([
-            ('onboarding_id', '=', self.onboarding_1.id), ('company_id', '=', False)], count=True)
-        nb_progress_steps = self.env['onboarding.progress.step'].search([
-            ('step_id', '=', self.onboarding_1_step_1.id)], count=True)
-
-        # Even though multiple onboarding progress (& steps) records exist
-        self.assertEqual(nb_progress, 2)
-        self.assertEqual(nb_progress_steps, 2)
-
-        # no error is raised, and so we can interact
-        _ = self.onboarding_1_step_1.current_progress_step_id
-        self.onboarding_1_step_1.action_set_just_done()
-
-        # Same with onboarding progress
-        _ = self.onboarding_1.current_progress_id
-        self.onboarding_1.action_close()
+        with self.assertRaises(IntegrityError):
+            self.env['onboarding.progress'].create({
+                'onboarding_id': self.onboarding_1.id,
+                'company_id': self.env.company.id
+            })

addons/onboarding/tests/test_onboarding_concurrency.py

@@ -0,0 +1,86 @@
+# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+import threading
+from concurrent.futures import ThreadPoolExecutor
+
+from psycopg2 import IntegrityError
+
+import odoo
+from odoo.tests.common import get_db_name, tagged, BaseCase
+from odoo.tools import mute_logger
+
+
+@tagged('-standard', '-at_install', 'post_install', 'database_breaking')
+class TestOnboardingConcurrency(BaseCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.registry = odoo.registry(get_db_name())
+        cls.addClassCleanup(cls.cleanUpClass)
+
+        with cls.registry.cursor() as cr:
+            env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {})
+            cls.onboarding_id = env['onboarding.onboarding'].create([
+                {
+                    'name': 'Test Onboarding Concurrent',
+                    'is_per_company': False,
+                    'route_name': 'onboarding_concurrent'
+                }
+            ]).id
+
+    @classmethod
+    def cleanUpClass(cls):
+        with cls.registry.cursor() as cr:
+            env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {})
+            env['onboarding.onboarding'].browse(cls.onboarding_id).unlink()
+            env['onboarding.progress'].search([
+                ('onboarding_id', '=', cls.onboarding_id)
+            ]).unlink()
+
+    @mute_logger('odoo.sql_db')
+    def test_concurrent_create_progress(self):
+        barrier = threading.Barrier(2)
+
+        def run():
+            raised_unique_violation = False
+
+            with self.registry.cursor() as cr:
+                env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {})
+                onboarding = env['onboarding.onboarding'].search([
+                    ('id', '=', self.onboarding_id)
+                ])
+                # There is no progress record
+                self.assertFalse(env['onboarding.progress'].search([
+                    ('onboarding_id', '=', self.onboarding_id)
+                ]))
+                barrier.wait(timeout=2)
+                try:
+                    onboarding._create_progress()
+                except IntegrityError as e:
+                    if e.pgcode == "23505":  # UniqueViolation
+                        raised_unique_violation = True
+
+            return raised_unique_violation
+
+        with ThreadPoolExecutor(max_workers=2) as executor:
+            future_1 = executor.submit(run)
+            future_2 = executor.submit(run)
+            raised_1 = future_1.result(timeout=3)
+            raised_2 = future_2.result(timeout=3)
+
+        with self.registry.cursor() as cr:
+            env = odoo.api.Environment(cr, odoo.SUPERUSER_ID, {})
+            self.assertEqual(
+                len(env['onboarding.progress'].search([('onboarding_id', '=', self.onboarding_id)])),
+                1,
+                "Exactly one thread should have been able to create a record."
+            )
+
+        self.assertEqual(
+            raised_1 + raised_2,
+            1,
+            "Exactly one thread should have raised a UniqueViolation error even though "
+            "there was no progress record at the start of its transaction."
+        )

addons/web/static/src/views/onboarding_banner.js

@@ -39,7 +39,11 @@ export class OnboardingBanner extends Component {
     }
 
     async loadBanner(bannerRoute) {
-        const response = await this.rpc(bannerRoute, { context: this.user.context });
+        let response = await this.rpc(bannerRoute, { context: this.user.context });
+        if (response.code === 503) {
+            // Sent by Onboarding Controller when rare concurrent `create` transactions occur
+            response = await this.rpc(bannerRoute, { context: this.user.context });
+        }
         if (!response.html) {
             return;
         }