[IMP] tools, website_{*}: increment multiple fields at once

* = blog, forum, slides

For performance reasons, allow to increment multiple fields of the same record
within the same query.

With python tests.

Task-2663320
Part of odoo/odoo#79615

addons/website_blog/controllers/main.py

@@ -295,7 +295,7 @@ class WebsiteBlog(http.Controller):
         response = request.render("website_blog.blog_post_complete", values)
 
         if blog_post.id not in request.session.get('posts_viewed', []):
-            if sql.increment_field_skiplock(blog_post, 'visits'):
+            if sql.increment_fields_skiplock(blog_post, 'visits'):
                 if not request.session.get('posts_viewed'):
                     request.session['posts_viewed'] = []
                 request.session['posts_viewed'].append(blog_post.id)

addons/website_forum/models/forum.py

@@ -904,7 +904,7 @@ class Post(models.Model):
 
     def _set_viewed(self):
         self.ensure_one()
-        return sql.increment_field_skiplock(self, 'views')
+        return sql.increment_fields_skiplock(self, 'views')
 
     def _get_access_action(self, access_uid=None, force_website=False):
         """ Instead of the classic form view, redirect to the post on the website directly """

addons/website_slides/controllers/main.py

@@ -67,9 +67,7 @@ class WebsiteSlides(WebsiteProfile):
         if not slide.channel_id.is_member:
             viewed_slides = request.session.setdefault('viewed_slides', set())
             if slide.id not in viewed_slides:
-                if tools.sql.increment_field_skiplock(slide, 'public_views'):
-                    # Increment total views immediately as compute is not triggered
-                    tools.sql.increment_field_skiplock(slide, 'total_views')
+                if tools.sql.increment_fields_skiplock(slide, 'public_views', 'total_views'):
                     viewed_slides.add(slide.id)
                     request.session.touch()
         else:

addons/website_slides/models/slide_slide.py

@@ -882,7 +882,7 @@ class Slide(models.Model):
             ('partner_id', '=', target_partner.id)
         ])
         if quiz_attempts_inc and existing_sudo:
-            sql.increment_field_skiplock(existing_sudo, 'quiz_attempts_count')
+            sql.increment_fields_skiplock(existing_sudo, 'quiz_attempts_count')
             existing_sudo.invalidate_recordset(['quiz_attempts_count'])
 
         new_slides = self_sudo - existing_sudo.mapped('slide_id')

odoo/addons/test_performance/models/models.py

@@ -67,3 +67,16 @@ class Eggs(models.Model):
     _description = 'Test Performance Eggs'
 
     name = fields.Char()
+
+
+class Mozzarella(models.Model):
+    _name = 'test_performance.mozzarella'
+    _description = 'Test Performance Mozzarella'
+
+    value = fields.Integer(default=0, required=True)
+    value_plus_one = fields.Integer(compute="_value_plus_one", required=True, store=True)
+
+    @api.depends('value')
+    def _value_plus_one(self):
+        for record in self:
+            record.value_plus_one = record.value + 1

odoo/addons/test_performance/security/ir.model.access.csv

@@ -4,3 +4,4 @@ access_test_performance_line,access_test_performance_line,model_test_performance
 access_test_performance_tag,access_test_performance_tag,model_test_performance_tag,,1,1,1,1
 access_test_performance_bacon,access_test_performance_bacon,model_test_performance_bacon,,1,1,1,1
 access_test_performance_eggs,access_test_performance_eggs,model_test_performance_eggs,,1,1,1,1
+access_test_performance_mozzarella,access_test_performance_mozzarella,model_test_performance_mozzarella,,1,1,1,1

odoo/addons/test_performance/tests/test_performance.py

@@ -3,12 +3,15 @@
 
 from collections import defaultdict
 import json
+import logging
 
 from odoo.addons.base.tests.common import SavepointCaseWithUserDemo
 from odoo.tests.common import TransactionCase, users, warmup, tagged
-from odoo.tools import mute_logger, json_default
+from odoo.tools import mute_logger, json_default, sql
 from odoo import Command
 
+_logger = logging.getLogger(__name__)
+
 
 class TestPerformance(SavepointCaseWithUserDemo):
 
@@ -610,3 +613,75 @@ class TestMapped(TransactionCase):
         with self.assertQueryCount(3):
             for rec in recs:
                 rec.line_ids.mapped('value')
+
+
+@tagged('increment_perf')
+class TestIncrementFieldsSkipLock(TransactionCase):
+    """ Test the behavior of the function `increment_fields_skiplock`.
+
+    Note that, per-design, the function will not always update the requested
+    records in case of a (table/row-level) lock. This is reflected in these
+    tests as we make sure to check the integrity of the results whether
+    any record was updated or not.
+    """
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+
+        cls.record = cls.env['test_performance.mozzarella'].create([{
+            'value': 1,
+            'value_plus_one': 2,
+        }])
+        cls.other_record = cls.env['test_performance.mozzarella'].create([{
+            'value': 10,
+            'value_plus_one': 11,
+        }])
+
+    def test_increment_fields_skiplock_one_field(self):
+        """ Test that we can increment the value of a single field of a record
+        with `increment_fields_skiplock` and that it doesn't trigger _compute
+        methods of fields depending on it.
+
+        If the test fails because of changes making that _compute **are**
+        triggered, be sure to also check `increment_fields_skiplock` uses on the
+        codebase when updating this test class.
+        """
+        with self.assertQueryCount(1):
+            did_update = sql.increment_fields_skiplock(self.record, 'value')
+            _logger.info('increment_fields_skiplock did %supdate the field', '' if did_update else 'not ')
+
+        # increment_fields_skiplock does not invalidate the cache
+        self.record.invalidate_recordset()
+
+        with self.assertQueryCount(1):  # Read
+            if did_update:
+                self.assertEqual(self.record.value, 2, "according to increment_fields_skiplock's output, this number should have been incremented.")
+            else:
+                # Making sure the test random locks do not break the test; but updated and the values must be consistent
+                self.assertEqual(self.record.value, 1, "according to increment_fields_skiplock output's, this number should NOT have been incremented.")
+
+            self.assertEqual(self.record.value_plus_one, 2, "This value should not have been incremented, irrespective of the presence of a lock or not.")
+
+        self.assertEqual(self.other_record.value, 10, "other_record should not have been updated.")
+        self.assertEqual(self.other_record.value_plus_one, 11, "other_record should not have been updated.")
+
+    def test_increment_fields_skiplock_multiple_fields(self):
+        """ Test that we can update several fields on the same rows with one request.
+        """
+        with self.assertQueryCount(1):
+            did_update = sql.increment_fields_skiplock(self.record, 'value', 'value_plus_one')
+            _logger.info('increment_fields_skiplock did %supdate the fields', '' if did_update else 'not ')
+
+        # increment_fields_skiplock does not invalidate the cache
+        self.record.invalidate_recordset()
+
+        with self.assertQueryCount(1):  # Read
+            if did_update:
+                self.assertEqual(self.record.value, 2, "according to increment_fields_skiplock's output, this number should have been incremented.")
+                self.assertEqual(self.record.value_plus_one, 3, "according to increment_fields_skiplock's output, this number should have been incremented.")
+            else:  # Making sure the test random locks do not break the test; but updated and the values must be consistent
+                self.assertEqual(self.record.value, 1, "according to increment_fields_skiplock output's, this number should NOT have been incremented.")
+                self.assertEqual(self.record.value_plus_one, 2, "according to increment_fields_skiplock's output, this number should NOT have been incremented.")
+
+        self.assertEqual(self.other_record.value, 10, "other_record should not have been updated.")
+        self.assertEqual(self.other_record.value_plus_one, 11, "other_record should not have been updated.")

odoo/tools/sql.py

@@ -5,6 +5,8 @@
 
 import logging
 import psycopg2
+from psycopg2.sql import SQL, Identifier
+
 import odoo.sql_db
 from collections import defaultdict
 from contextlib import closing
@@ -313,25 +315,35 @@ def reverse_order(order):
     return ', '.join(items)
 
 
-def increment_field_skiplock(record, field):
+def increment_fields_skiplock(records, *fields):
     """
-        Increment 'friendly' the [field] of the current [record](s)
+        Increment 'friendly' the given `fields` of the current `records`.
         If record is locked, we just skip the update.
         It doesn't invalidate the cache since the update is not critical.
 
-        :rtype: bool - if field has been incremented or not
+        :param records: recordset to update
+        :param fields: integer fields to increment
+        :returns: whether the specified fields were incremented on any record.
+        :rtype: bool
     """
-    if not record:
+    if not records:
         return False
 
-    assert record._fields[field].type == 'integer'
+    for field in fields:
+        assert records._fields[field].type == 'integer'
 
-    cr = record._cr
-    query = """
-        UPDATE {table} SET {field} = {field} + 1 WHERE id IN (
-            SELECT id from {table} WHERE id in %(ids)s FOR UPDATE SKIP LOCKED
-        ) RETURNING id
-    """.format(table=record._table, field=field)
-    cr.execute(query, {'ids': tuple(record.ids)})
+    query = SQL("""
+        UPDATE {table}
+           SET {sets}
+         WHERE id IN (SELECT id FROM {table} WHERE id = ANY(%(ids)s) FOR UPDATE SKIP LOCKED)
+    """).format(
+        table=Identifier(records._table),
+        sets=SQL(', ').join(map(
+            SQL('{0} = {0} + 1').format,
+            map(Identifier, fields)
+        ))
+    )
 
-    return bool(cr.fetchone())
+    cr = records._cr
+    cr.execute(query, {'ids': records.ids})
+    return bool(cr.rowcount)