[MERGE] mail, test_mail: remove aggregation from `message_format`

Remove aggregation from `message_format `and `_message_read_dict_postprocess`.
There should be no functional change in this PR.

Purpose
=======

The existing code working with "tree" for aggregation and doing explicit "read"
or "search" for related fields does not seem necessary since the ORM of v13.

Considering that it adds complexity to the method, if its original purpose is
not necessary anymore, it would be better to change it.

The notable changes of the ORM that would justify this task are:
 - single cache, shared between current user and sudo (which is called a lot in
   those methods and might explain why the "tree" mechanism was put in place
   originally)
 - m2o and o2m being kept consistent with each other, which allows accessing o2m
   through fields directly (instead of having to use "search" to ensure getting
   consistent data)

Explanation
===========

Doing "read" or "search" could be counter-productive if the data are already in
cache because using those methods will lead to a query no matter what.
The ideal way is to simply access the fields through the records.

As for the "tree" and aggregation in general, the prefetch is automatically
taking care of that when iterating.

task-2180311

closes odoo/odoo#43841

Signed-off-by: Sébastien Theys (seb) <seb@odoo.com>

5 files changed, 209 insertions, 137 deletions

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 9b1dd26b0ed..cb5b71d10b8 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -941,92 +941,63 @@ class Message(models.Model):
     # MESSAGE READ / FETCH / FAILURE API
     # ------------------------------------------------------
 
-    @api.model
-    def _message_read_dict_postprocess(self, messages, message_tree):
-        """ Post-processing on values given by message_read. This method will
-            handle partners in batch to avoid doing numerous queries.
-
-            :param list messages: list of message, as get_dict result
-            :param dict message_tree: {[msg.id]: msg browse record as super user}
-        """
-        # 1. Aggregate partners (author_id and partner_ids), attachments and tracking values
-        partners = self.env['res.partner'].sudo()
-        attachments = self.env['ir.attachment']
-        message_ids = list(message_tree.keys())
-        email_notification_tree = {}
-        for message in message_tree.values():
-            if message.author_id:
-                partners |= message.author_id
-            # find all notified partners
-            email_notification_tree[message.id] = message.notification_ids.filtered(
-                lambda n: n.notification_type == 'email' and n.res_partner_id.active and
-                (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share))
-            if message.attachment_ids:
-                attachments |= message.attachment_ids
-        partners |= self.env['mail.notification'].concat(*email_notification_tree.values()).mapped('res_partner_id')
-        # Read partners as SUPERUSER -> message being browsed as SUPERUSER it is already the case
-        partners_names = partners.name_get()
-        partner_tree = dict((partner[0], partner) for partner in partners_names)
-
-        # 2. Attachments as SUPERUSER, because could receive msg and attachments for doc uid cannot see
-        attachments_data = attachments.sudo().read(['id', 'name', 'mimetype'])
+    def _message_format(self, fnames):
+        """Reads values from messages and formats them for the web client."""
+        self.check_access_rule('read')
+        vals_list = self._read_format(fnames)
         safari = request and request.httprequest.user_agent.browser == 'safari'
-        attachments_tree = dict((attachment['id'], {
-            'id': attachment['id'],
-            'filename': attachment['name'],
-            'name': attachment['name'],
-            'mimetype': 'application/octet-stream' if safari and attachment['mimetype'] and 'video' in attachment['mimetype'] else attachment['mimetype'],
-        }) for attachment in attachments_data)
-
-        # 3. Tracking values
-        tracking_values = self.env['mail.tracking.value'].sudo().search([('mail_message_id', 'in', message_ids)])
-        message_to_tracking = dict()
-        tracking_tree = dict.fromkeys(tracking_values.ids, False)
-        for tracking in tracking_values:
-            groups = tracking.field_groups
-            if not groups or self.env.is_superuser() or self.user_has_groups(groups):
-                message_to_tracking.setdefault(tracking.mail_message_id.id, list()).append(tracking.id)
-                tracking_tree[tracking.id] = {
-                    'id': tracking.id,
-                    'changed_field': tracking.field_desc,
-                    'old_value': tracking.get_old_display_value()[0],
-                    'new_value': tracking.get_new_display_value()[0],
-                    'field_type': tracking.field_type,
-                }
+        for vals in vals_list:
+            message_sudo = self.browse(vals['id']).sudo().with_prefetch(self.ids)
 
-        # 4. Update message dictionaries
-        for message_dict in messages:
-            message_id = message_dict.get('id')
-            message = message_tree[message_id]
-            if message.author_id:
-                author = partner_tree[message.author_id.id]
+            # Author
+            if message_sudo.author_id:
+                author = (message_sudo.author_id.id, message_sudo.author_id.display_name)
             else:
-                author = (0, message.email_from)
+                author = (0, message_sudo.email_from)
+
+            # Notifications
             customer_email_status = (
-                (all(n.notification_status == 'sent' for n in message.notification_ids if n.notification_type == 'email') and 'sent') or
-                (any(n.notification_status == 'exception' for n in message.notification_ids if n.notification_type == 'email') and 'exception') or
-                (any(n.notification_status == 'bounce' for n in message.notification_ids if n.notification_type == 'email') and 'bounce') or
+                (all(n.notification_status == 'sent' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'sent') or
+                (any(n.notification_status == 'exception' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'exception') or
+                (any(n.notification_status == 'bounce' for n in message_sudo.notification_ids if n.notification_type == 'email') and 'bounce') or
                 'ready'
             )
             customer_email_data = []
-            for notification in email_notification_tree[message.id]:
-                customer_email_data.append((partner_tree[notification.res_partner_id.id][0], partner_tree[notification.res_partner_id.id][1], notification.notification_status))
+            filtered_notifications = message_sudo.notification_ids.filtered(lambda n:
+                n.notification_type == 'email' and n.res_partner_id.active and
+                (n.notification_status in ('bounce', 'exception', 'canceled') or n.res_partner_id.partner_share)
+            )
+            for notification in filtered_notifications:
+                customer_email_data.append((notification.res_partner_id.id, notification.res_partner_id.display_name, notification.notification_status))
 
-            has_access_to_model = message.model and self.env[message.model].check_access_rights('read', raise_exception=False)
-            main_attachment = None
-            if message.attachment_ids and message.res_id and issubclass(self.pool[message.model], self.pool['mail.thread']) and has_access_to_model:
-                main_attachment =  self.env[message.model].browse(message.res_id).message_main_attachment_id
+            # Attachments
+            main_attachment = self.env['ir.attachment']
+            if message_sudo.attachment_ids and message_sudo.res_id and issubclass(self.pool[message_sudo.model], self.pool['mail.thread']):
+                main_attachment = self.env[message_sudo.model].sudo().browse(message_sudo.res_id).message_main_attachment_id
             attachment_ids = []
-            for attachment in message.attachment_ids:
-                if attachment.id in attachments_tree:
-                    attachments_tree[attachment.id]['is_main'] = main_attachment == attachment
-                    attachment_ids.append(attachments_tree[attachment.id])
+            for attachment in message_sudo.attachment_ids:
+                attachment_ids.append({
+                    'id': attachment.id,
+                    'filename': attachment.name,
+                    'name': attachment.name,
+                    'mimetype': 'application/octet-stream' if safari and attachment.mimetype and 'video' in attachment.mimetype else attachment.mimetype,
+                    'is_main': main_attachment == attachment
+                })
+
+            # Tracking values
             tracking_value_ids = []
-            for tracking_value_id in message_to_tracking.get(message_id, list()):
-                if tracking_value_id in tracking_tree:
-                    tracking_value_ids.append(tracking_tree[tracking_value_id])
-
-            message_dict.update({
+            for tracking in message_sudo.tracking_value_ids:
+                groups = tracking.field_groups
+                if not groups or self.env.is_superuser() or self.user_has_groups(groups):
+                    tracking_value_ids.append({
+                        'id': tracking.id,
+                        'changed_field': tracking.field_desc,
+                        'old_value': tracking.get_old_display_value()[0],
+                        'new_value': tracking.get_new_display_value()[0],
+                        'field_type': tracking.field_type,
+                    })
+
+            vals.update({
                 'author_id': author,
                 'customer_email_status': customer_email_status,
                 'customer_email_data': customer_email_data,
@@ -1034,7 +1005,7 @@ class Message(models.Model):
                 'tracking_value_ids': tracking_value_ids,
             })
 
-        return True
+        return vals_list
 
     def message_fetch_failed(self):
         messages = self.search([
@@ -1114,44 +1085,25 @@ class Message(models.Model):
                     'moderation_status': 'pending_moderation'
                 }
         """
-        message_values = self.read(self._get_message_format_fields())
-        message_tree = dict((m.id, m) for m in self.sudo())
-        self._message_read_dict_postprocess(message_values, message_tree)
-
-        # add subtype data (is_note flag, is_discussion flag , subtype_description). Do it as sudo
-        # because portal / public may have to look for internal subtypes
-        subtype_ids = [msg['subtype_id'][0] for msg in message_values if msg['subtype_id']]
-        subtypes = self.env['mail.message.subtype'].sudo().browse(subtype_ids).read(['internal', 'description','id'])
-        subtypes_dict = dict((subtype['id'], subtype) for subtype in subtypes)
+        vals_list = self._message_format(self._get_message_format_fields())
 
         com_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment')
         note_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note')
 
-        # fetch notification status
-
-        notif_dict = defaultdict(lambda: defaultdict(list))
-        notifs = self.env['mail.notification'].sudo().search([('mail_message_id', 'in', list(mid for mid in message_tree)), ('res_partner_id', '!=', False)])
-
-        for notif in notifs:
-            mid = notif.mail_message_id.id
-            if notif.is_read:
-                notif_dict[mid]['history_partner_ids'].append(notif.res_partner_id.id)
-            else:
-                notif_dict[mid]['needaction_partner_ids'].append(notif.res_partner_id.id)
-
-        for message in message_values:
-            message.update({
-                'needaction_partner_ids': notif_dict[message['id']]['needaction_partner_ids'],
-                'history_partner_ids': notif_dict[message['id']]['history_partner_ids'],
-                'is_note': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == note_id,
-                'is_discussion': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['id'] == com_id,
-                'subtype_description': message['subtype_id'] and subtypes_dict[message['subtype_id'][0]]['description']
+        for vals in vals_list:
+            message_sudo = self.browse(vals['id']).sudo().with_prefetch(self.ids)
+            notifs = message_sudo.notification_ids.filtered(lambda n: n.res_partner_id)
+            vals.update({
+                'needaction_partner_ids': notifs.filtered(lambda n: not n.is_read).res_partner_id.ids,
+                'history_partner_ids': notifs.filtered(lambda n: n.is_read).res_partner_id.ids,
+                'is_note': message_sudo.subtype_id.id == note_id,
+                'is_discussion': message_sudo.subtype_id.id == com_id,
+                'subtype_description': message_sudo.subtype_id.description,
+                'is_notification': vals['message_type'] == 'user_notification',
             })
-            message['is_notification'] = message['message_type'] == 'user_notification'
-
-            if message['model'] and self.env[message['model']]._original_module:
-                message['module_icon'] = modules.module.get_module_icon(self.env[message['model']]._original_module)
-        return message_values
+            if vals['model'] and self.env[vals['model']]._original_module:
+                vals['module_icon'] = modules.module.get_module_icon(self.env[vals['model']]._original_module)
+        return vals_list
 
     def _get_message_format_fields(self):
         return [
diff --git a/addons/portal/models/mail_message.py b/addons/portal/models/mail_message.py
index cfda6d5eaa0..91d40d7a891 100644
--- a/addons/portal/models/mail_message.py
+++ b/addons/portal/models/mail_message.py
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
-from odoo import api, models
+from odoo import models
 
 
 class MailMessage(models.Model):
@@ -15,12 +15,10 @@ class MailMessage(models.Model):
         ])
 
     def _portal_message_format(self, fields_list):
-        message_values = self.read(fields_list)
-        message_tree = dict((m.id, m) for m in self.sudo())
-        self._message_read_dict_postprocess(message_values, message_tree)
+        vals_list = self._message_format(fields_list)
         IrAttachmentSudo = self.env['ir.attachment'].sudo()
-        for message in message_values:
-            for attachment in message.get('attachment_ids', []):
+        for vals in vals_list:
+            for attachment in vals.get('attachment_ids', []):
                 if not attachment.get('access_token'):
                     attachment['access_token'] = IrAttachmentSudo.browse(attachment['id']).generate_access_token()[0]
-        return message_values
+        return vals_list
diff --git a/addons/portal_rating/models/mail_message.py b/addons/portal_rating/models/mail_message.py
index 9f2be5d1a25..20716960503 100644
--- a/addons/portal_rating/models/mail_message.py
+++ b/addons/portal_rating/models/mail_message.py
@@ -13,16 +13,16 @@ class MailMessage(models.Model):
             field_list += ['rating_value']
         return super(MailMessage, self)._portal_message_format(field_list)
 
-    def _message_read_dict_postprocess(self, message_values, message_tree):
+    def _message_format(self, fnames):
         """ Override the method to add information about a publisher comment
         on each rating messages if requested, and compute a plaintext value of it.
         """
-        res = super(MailMessage, self)._message_read_dict_postprocess(message_values, message_tree)
+        vals_list = super(MailMessage, self)._message_format(fnames)
 
         if self._context.get('rating_include'):
             infos = ["id", "publisher_comment", "publisher_id", "publisher_datetime", "message_id"]
             related_rating = self.env['rating.rating'].sudo().search([('message_id', 'in', self.ids)]).read(infos)
             mid_rating_tree = dict((rating['message_id'][0], rating) for rating in related_rating)
-            for values in message_values:
-                values["rating"] = mid_rating_tree.get(values['id'], {})
-        return res
+            for vals in vals_list:
+                vals["rating"] = mid_rating_tree.get(vals['id'], {})
+        return vals_list
diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py
index 1b1a2ca3915..b952fe73388 100644
--- a/addons/test_mail/tests/test_performance.py
+++ b/addons/test_mail/tests/test_performance.py
@@ -3,7 +3,7 @@
 import base64
 
 from odoo.addons.base.tests.common import TransactionCaseWithUserDemo
-from odoo.tests.common import TransactionCase, users, warmup
+from odoo.tests.common import users, warmup
 from odoo.tests import tagged
 from odoo.tools import mute_logger, formataddr
 
@@ -253,7 +253,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
 
     @users('__system__', 'emp')
     @warmup
-    @mute_logger('odoo.addons.mail.models.mail_mail')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail')
     def test_mail_composer(self):
         test_record = self.env['mail.test.full'].browse(self.test_record_full.id)
         customer_id = self.customer.id
@@ -267,17 +267,17 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 'partner_ids': [(4, customer_id)],
             })
 
-        with self.assertQueryCount(__system__=36, emp=41):
+        with self.assertQueryCount(__system__=31, emp=37):
             composer.send_mail()
 
     @users('__system__', 'emp')
     @warmup
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     def test_mail_composer_w_template(self):
         test_record = self.env['mail.test.full'].browse(self.test_record_full.id)
         test_template = self.env['mail.template'].browse(self.test_template_full.id)
-        # TODO XDO/TDE/MAT FIXME non deterministic between 25 and 29 queries
-        with self.assertQueryCount(__system__=28, emp=29):
+        # TODO FIXME non deterministic, last check 25 feb 2020. Runbot at most +7 compared to local.
+        with self.assertQueryCount(__system__=27, emp=28):
             composer = self.env['mail.compose.message'].with_context({
                 'default_composition_mode': 'comment',
                 'default_model': test_record._name,
@@ -286,13 +286,13 @@ class TestMailAPIPerformance(BaseMailPerformance):
             }).create({})
             composer.onchange_template_id_wrapper()
 
-        with self.assertQueryCount(__system__=44, emp=48):
+        with self.assertQueryCount(__system__=39, emp=44):
             composer.send_mail()
 
         # remove created partner to ensure tests are the same each run
         self.env['res.partner'].sudo().search([('email', '=', 'nopartner.test@example.com')]).unlink()
 
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     @users('__system__', 'emp')
     @warmup
     def test_message_assignation_email(self):
@@ -307,7 +307,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     @warmup
     def test_message_assignation_inbox(self):
         record = self.env['mail.test.track'].create({'name': 'Test'})
-        with self.assertQueryCount(__system__=25, emp=27):
+        with self.assertQueryCount(__system__=19, emp=22):
             record.write({
                 'user_id': self.user_test.id,
             })
@@ -345,7 +345,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
                 message_type='comment',
                 subtype_xmlid='mail.mt_comment')
 
-    @mute_logger('odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
     @users('__system__', 'emp')
     @warmup
     def test_message_post_one_email_notification(self):
@@ -363,7 +363,7 @@ class TestMailAPIPerformance(BaseMailPerformance):
     def test_message_post_one_inbox_notification(self):
         record = self.env['mail.test.simple'].create({'name': 'Test'})
 
-        with self.assertQueryCount(__system__=20, emp=22):
+        with self.assertQueryCount(__system__=15, emp=18):
             record.message_post(
                 body='<p>Test Post Performances with an inbox ping</p>',
                 partner_ids=self.user_test.partner_id.ids,
@@ -696,6 +696,119 @@ class TestMailComplexPerformance(BaseMailPerformance):
         self.assertEqual(rec1.message_ids[2].notified_partner_ids, self.partners | self.user_portal.partner_id)
         self.assertEqual(len(rec1.message_ids), 3)
 
+    @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink')
+    @users('emp')
+    @warmup
+    def test_message_format(self):
+        """Test performance of `_message_format` and of `message_format` with
+        multiple messages with multiple attachments, different authors, various
+        notifications, and different tracking values.
+        Those messages might not make sense functionally but they are crafted to
+        cover as much of the code as possible in regard to number of queries.
+        """
+        name_field = self.env['ir.model.fields']._get(self.umbrella._name, 'name')
+        customer_id_field = self.env['ir.model.fields']._get(self.umbrella._name, 'customer_id')
+
+        messages = self.env['mail.message'].sudo().create([{
+            'subject': 'Test 0',
+            'body': '<p>Test 0</p>',
+            'author_id': self.partners[0].id,
+            'email_from': self.partners[0].email,
+            'model': 'mail.test',
+            'res_id': self.umbrella.id,
+            'subtype_id': self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment'),
+            'attachment_ids': [
+                (0, 0, {
+                    'name': 'test file 0 - %d' % j,
+                    'datas': 'data',
+                }) for j in range(2)
+            ],
+            'notification_ids': [
+                (0, 0, {
+                    'res_partner_id': self.partners[3].id,
+                    'notification_type': 'inbox',
+                }),
+                (0, 0, {
+                    'res_partner_id': self.partners[4].id,
+                    'notification_type': 'email',
+                    'notification_status': 'exception',
+                }),
+                (0, 0, {
+                    'res_partner_id': self.partners[6].id,
+                    'notification_type': 'email',
+                    'notification_status': 'exception',
+                }),
+            ],
+            'tracking_value_ids': [
+                (0, 0, {
+                    'field': name_field.id,
+                    'field_desc': 'Name',
+                    'old_value_char': 'old 0',
+                    'new_value_char': 'new 0',
+                }),
+                (0, 0, {
+                    'field': customer_id_field.id,
+                    'field_desc': 'Customer',
+                    'old_value_integer': self.partners[7].id,
+                    'new_value_integer': self.partners[8].id,
+                }),
+            ]
+        }, {
+            'subject': 'Test 1',
+            'body': '<p>Test 1</p>',
+            'author_id': self.partners[1].id,
+            'email_from': self.partners[1].email,
+            'model': 'mail.test',
+            'res_id': self.umbrella.id,
+            'subtype_id': self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note'),
+            'attachment_ids': [
+                (0, 0, {
+                    'name': 'test file 1 - %d' % j,
+                    'datas': 'data',
+                }) for j in range(2)
+            ],
+            'notification_ids': [
+                (0, 0, {
+                    'res_partner_id': self.partners[5].id,
+                    'notification_type': 'inbox',
+                }),
+                (0, 0, {
+                    'res_partner_id': self.partners[6].id,
+                    'notification_type': 'email',
+                    'notification_status': 'exception',
+                }),
+            ],
+            'tracking_value_ids': [
+                (0, 0, {
+                    'field': name_field.id,
+                    'field_desc': 'Name',
+                    'old_value_char': 'old 1',
+                    'new_value_char': 'new 1',
+                }),
+                (0, 0, {
+                    'field': customer_id_field.id,
+                    'field_desc': 'Customer',
+                    'old_value_integer': self.partners[7].id,
+                    'new_value_integer': self.partners[8].id,
+                }),
+            ]
+        }])
+
+        with self.assertQueryCount(emp=6):
+            res = messages.message_format()
+            self.assertEqual(len(res), 2)
+            for message in res:
+                self.assertEqual(len(message['attachment_ids']), 2)
+
+        messages.flush()
+        messages.invalidate_cache()
+
+        with self.assertQueryCount(emp=18):
+            res = messages.message_format()
+            self.assertEqual(len(res), 2)
+            for message in res:
+                self.assertEqual(len(message['attachment_ids']), 2)
+
 
 @tagged('mail_performance')
 class TestMailHeavyPerformancePost(BaseMailPerformance):
@@ -815,7 +928,7 @@ class TestMailHeavyPerformancePost(BaseMailPerformance):
         ]
         self.attachements = self.env['ir.attachment'].with_user(self.env.user).create(self.vals)
         attachement_ids = self.attachements.ids
-        with self.assertQueryCount(emp=91):
+        with self.assertQueryCount(emp=86):
             self.cr.sql_log = self.warm and self.cr.sql_log_count
             record.with_context({}).message_post(
                 body='<p>Test body <img src="cid:cid1"> <img src="cid:cid2"></p>',
diff --git a/odoo/models.py b/odoo/models.py
index bbec718b5a3..a5d8ddb7900 100644
--- a/odoo/models.py
+++ b/odoo/models.py
@@ -2932,11 +2932,20 @@ Fields:
                         stored_fields.add(f.name)
         self._read(stored_fields)
 
-        # retrieve results from records; this takes values from the cache and
-        # computes remaining fields
+        return self._read_format(fnames=fields, load=load)
+
+    def _read_format(self, fnames, load='_classic_read'):
+        """Returns a list of dictionaries mapping field names to their values,
+        with one dictionary per record that exists.
+
+        The output format is similar to the one expected from the `read` method.
+
+        The current method is different from `read` because it retrieves its
+        values from the cache without doing a query when it is avoidable.
+        """
         data = [(record, {'id': record._ids[0]}) for record in self]
         use_name_get = (load == '_classic_read')
-        for name in fields:
+        for name in fnames:
             convert = self._fields[name].convert_to_read
             for record, vals in data:
                 # missing records have their vals empty