From 26d110c4727d3c423672044164a51c02671d2797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 5 Apr 2018 11:29:21 +0200 Subject: [PATCH 1/3] [IMP] mail: remove cache invalidation when notifying a message with a parent When notifying partners or channels of a message, a cache invalidation is currently done if the message has a parent. This invalidation has been done when migrating the mail module at the new API at 4b122ad41d873febe288ab1f8c6cdc078ca2a479. In that time notifying people of a message lead to the creation of notifications of the parent message, if any. It was due to the chatter being threaded and therefore displaying message with their header message. Adding notifications for the parent was necessary to avoid access rights issues when fetching parent message data. Indeed as being notified is one of the rule to see a message record adding notifications was done. A cache invalidation has therefore been added to clean the message cache and ensure everything was fine. Commit 88b8cd058713bfad2942f7e7434c34b6c1a6e7da changed the way notifications are modeled in Odoo. Notification on parent message was removed and access rights changed. Threaded mode for Chatter has also been removed. However cache invalidation has been kept probably by fear of removing it. It does not seem to have any viable reason to invalidate cache when a message has a parent. Posting a message does not push other messages in users's Inbox meaning there should not be any issue with the cache preventing to see messages. Removing this cache invalidation allow to gain queries in performance tests. It has an impact on each process involving message creation which is quite common in Odoo. --- addons/mail/models/mail_message.py | 5 ----- addons/test_mail/tests/test_performance.py | 22 +++++++++++----------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 482afcbe4fe..e087d745861 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -856,9 +856,4 @@ class Message(models.Model): channels_sudo._notify(self) - # Discard cache, because child / parent allow reading and therefore - # change access rights. - if self.parent_id: - self.parent_id.invalidate_cache() - return True diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 4802af632f2..cfc78926bad 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -167,7 +167,7 @@ class TestAdvMailPerformance(TransactionCase): 'activity_type_id': self.env.ref('mail.mail_activity_data_todo').id, }) - with self.assertQueryCount(margin=1, admin=56, emp=85): # test_mail only: 56 - 85 + with self.assertQueryCount(margin=1, admin=53, emp=77): # test_mail only: 53 - 77 activity.action_feedback(feedback='Zizisse Done !') @users('admin', 'emp') @@ -181,7 +181,7 @@ class TestAdvMailPerformance(TransactionCase): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(margin=1, admin=56, emp=86): # test_mail only: 56 - 85 + with self.assertQueryCount(margin=1, admin=53, emp=78): # test_mail only: 53 - 78 record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -193,7 +193,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=82, emp=107): # test_mail only: 80 - 105 + with self.assertQueryCount(margin=1, admin=81, emp=105): # test_mail only: 79 - 103 record.write({ 'user_id': self.user_test.id, }) @@ -247,7 +247,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=75, emp=101): # test_mail only: 73 - 99 + with self.assertQueryCount(margin=1, admin=74, emp=99): # com runbot: 72 - 97 // test_mail only: 72 - 97 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -382,7 +382,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=116, emp=147): # com runbot 114 - 145 // test_mail only: 112 - 143 + with self.assertQueryCount(admin=115, emp=144): # com runbot 113 - 142 // test_mail only: 111 - 140 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -399,7 +399,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=138, emp=183): # com runbot 136 - 181 // test_mail only: 134 - 179 + with self.assertQueryCount(admin=137, emp=179): # com runbot 135 - 177 // test_mail only: 133 - 175 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -469,7 +469,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=84, emp=111): # test_mail only: 82 - 109 + with self.assertQueryCount(admin=82, emp=108): # test_mail only: 80 - 106 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -492,7 +492,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(admin=237, emp=286): # test_mail only: 230 - 279 + with self.assertQueryCount(admin=235, emp=282): # test_mail only: 228 - 275 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -521,7 +521,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=149, emp=173): # test_mail only: 144 - 168 + with self.assertQueryCount(admin=148, emp=171): # test_mail only: 143 - 166 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -559,7 +559,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=155, emp=183): # test_mail only: 150 - 178 + with self.assertQueryCount(admin=152, emp=178): # test_mail only: 147 - 173 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, @@ -593,7 +593,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) - with self.assertQueryCount(admin=61, emp=84): # test_mail only: 59 - 82 + with self.assertQueryCount(admin=60, emp=83): # test_mail only: 58 - 81 rec.write({ 'name': 'Test2', 'customer_id': customer_id, From bcc6884737a0e172fb436dfcc199e5e1155fdc60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 5 Apr 2018 12:51:21 +0200 Subject: [PATCH 2/3] [IMP] mail: limit cache invalidation when subscribing followers Cache invalidation when subscribing people has been added at e6f038a8216c249a3ce7d9f490010d9f666f3375. Indeed subscribing partners to a record may lead to an access right update as some of them are based on followers. This is why a cache invalidation is necessary to avoid access rights issues. However subscribing people to a record should change their rights only on the records involved in the subscription mechanism. We can therefore give ids to the cache invalidation to limit to updated records. Cache invalidation is also limited when writing on followers if writing on model, res_id or partner_id fields. Indeed changing subtypes or channel of a subscription should have no impact on cache and access rights. Cache invalidation done manually in _message_subscribe is not necessary as subscription create or update mail.followers records since f9c210923d89aeabfc358fdc7b8bd76df8c23ce8. Create and write of mail.followers records already ask for cache invalidation. It is therefore not necessary to invalidate cache twice. This commit allows to save a few queries on some tests, notably about activities that deal with subscription and messages. As cache is now kept it is not necessary to refetch some data, leading to a few query gain. We gain about 1K queries on com runbot. A side effect of limiting cache invalidation is that some unit tests require a manual cache invalidation to have up to date results. Indeed record not being up to date in cache was hidden by the invalidation we just removed. --- .../tests/test_account_customer_invoice.py | 1 + .../hr_holidays/tests/test_holidays_flow.py | 1 + addons/mail/models/mail_followers.py | 3 ++- addons/mail/models/mail_thread.py | 1 - addons/test_mail/tests/test_mail_activity.py | 1 + addons/test_mail/tests/test_performance.py | 24 +++++++++---------- 6 files changed, 17 insertions(+), 14 deletions(-) diff --git a/addons/account/tests/test_account_customer_invoice.py b/addons/account/tests/test_account_customer_invoice.py index b1743aa982f..0a4a58cbfb4 100644 --- a/addons/account/tests/test_account_customer_invoice.py +++ b/addons/account/tests/test_account_customer_invoice.py @@ -90,6 +90,7 @@ class TestAccountCustomerInvoice(AccountTestUsers): # I verify that invoice is now in Paid state assert (self.account_invoice_customer0.state == 'paid'), "Invoice is not in Paid state" + self.partner3.invalidate_cache(ids=self.partner3.ids) total_after_confirm = self.partner3.total_invoiced self.assertEquals(total_after_confirm - total_before_confirm, self.account_invoice_customer0.amount_untaxed_signed) diff --git a/addons/hr_holidays/tests/test_holidays_flow.py b/addons/hr_holidays/tests/test_holidays_flow.py index abd9b809863..ff5f3a80307 100644 --- a/addons/hr_holidays/tests/test_holidays_flow.py +++ b/addons/hr_holidays/tests/test_holidays_flow.py @@ -190,6 +190,7 @@ class TestHolidaysFlow(TestHrHolidaysBase): }) hol2_user_group = hol2.sudo(self.user_hruser_id) # Check left days: - 1 virtual remaining day + hol_status_2_employee_group.invalidate_cache() _check_holidays_status(hol_status_2_employee_group, 2.0, 0.0, 2.0, 1.0) # HrManager validates the first step diff --git a/addons/mail/models/mail_followers.py b/addons/mail/models/mail_followers.py index fc07e4e3ae7..4040de72276 100644 --- a/addons/mail/models/mail_followers.py +++ b/addons/mail/models/mail_followers.py @@ -57,7 +57,8 @@ class Followers(models.Model): if 'res_model' in vals or 'res_id' in vals: self._invalidate_documents() res = super(Followers, self).write(vals) - self._invalidate_documents() + if any(x in vals for x in ['res_model', 'res_id', 'partner_id']): + self._invalidate_documents() return res @api.multi diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 82a917482e3..5cdc223cfad 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2084,7 +2084,6 @@ class MailThread(models.AbstractModel): channel_ids, dict((cid, subtype_ids) for cid in channel_ids), customer_ids=customer_ids, check_existing=True, existing_policy='force') - self.invalidate_cache() return True @api.multi diff --git a/addons/test_mail/tests/test_mail_activity.py b/addons/test_mail/tests/test_mail_activity.py index 5a4121f2c6c..64c87a43cc7 100644 --- a/addons/test_mail/tests/test_mail_activity.py +++ b/addons/test_mail/tests/test_mail_activity.py @@ -99,6 +99,7 @@ class TestMailActivity(BaseFunctionalTest): self.assertEqual(self.test_record.activity_state, 'overdue') self.assertEqual(self.test_record.activity_user_id, self.user_employee) + self.test_record.invalidate_cache(ids=self.test_record.ids) self.assertEqual(self.test_record.activity_ids, act1 | act2 | act3) # Perform todo activities for admin diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index cfc78926bad..91c6017ef4b 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -160,14 +160,14 @@ class TestAdvMailPerformance(TransactionCase): 'default_res_model': 'mail.test.activity', }) - with self.assertQueryCount(admin=11, emp=15): # test_mail only: 11 - 15 + with self.assertQueryCount(admin=9, emp=13): # test_mail only: 9 - 13 activity = MailActivity.create({ 'summary': 'Test Activity', 'res_id': record.id, 'activity_type_id': self.env.ref('mail.mail_activity_data_todo').id, }) - with self.assertQueryCount(margin=1, admin=53, emp=77): # test_mail only: 53 - 77 + with self.assertQueryCount(margin=1, admin=49, emp=73): # test_mail only: 49 - 73 activity.action_feedback(feedback='Zizisse Done !') @users('admin', 'emp') @@ -176,12 +176,12 @@ class TestAdvMailPerformance(TransactionCase): def test_adv_activity_mixin(self): record = self.env['mail.test.activity'].create({'name': 'Test'}) - with self.assertQueryCount(admin=11, emp=15): # test_mail only: 11 - 15 + with self.assertQueryCount(admin=9, emp=13): # test_mail only: 9 - 13 record.action_start('Test Start') record.write({'name': 'Dupe write'}) - with self.assertQueryCount(margin=1, admin=53, emp=78): # test_mail only: 53 - 78 + with self.assertQueryCount(margin=1, admin=51, emp=75): # test_mail only: 51 - 75 record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -275,7 +275,7 @@ class TestAdvMailPerformance(TransactionCase): with self.assertQueryCount(admin=6, emp=6): # test_mail only: 6 - 6 record.message_subscribe(partner_ids=self.user_test.partner_id.ids) - with self.assertQueryCount(admin=3, emp=3): # test_mail only: 3 - 3 + with self.assertQueryCount(admin=2, emp=2): # test_mail only: 2 - 2 record.message_subscribe(partner_ids=self.user_test.partner_id.ids) @mute_logger('odoo.models.unlink') @@ -288,7 +288,7 @@ class TestAdvMailPerformance(TransactionCase): with self.assertQueryCount(admin=5, emp=5): # test_mail only: 5 - 5 record.message_subscribe(partner_ids=self.user_test.partner_id.ids, subtype_ids=subtype_ids) - with self.assertQueryCount(admin=14, emp=14): # test_mail only: 14 - 14 + with self.assertQueryCount(admin=12, emp=12): # test_mail only: 12 - 12 record.message_subscribe(partner_ids=self.user_test.partner_id.ids, subtype_ids=subtype_ids) @@ -382,7 +382,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=115, emp=144): # com runbot 113 - 142 // test_mail only: 111 - 140 + with self.assertQueryCount(admin=114, emp=143): # com runbot 112 - 141 // test_mail only: 110 - 139 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -399,7 +399,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=137, emp=179): # com runbot 135 - 177 // test_mail only: 133 - 175 + with self.assertQueryCount(admin=136, emp=178): # com runbot 134 - 176 // test_mail only: 132 - 174 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -446,7 +446,7 @@ class TestHeavyMailPerformance(TransactionCase): self.assertEqual(rec.message_channel_ids, self.channel) # subscribe existing and new followers with force=True, meaning all will have the same subtypes - with self.assertQueryCount(admin=42, emp=43): # test_mail only: 42 - 43 + with self.assertQueryCount(admin=42, emp=42): # test_mail only: 42 - 42 rec.message_subscribe( partner_ids=pids, channel_ids=cids, @@ -492,7 +492,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(admin=235, emp=282): # test_mail only: 228 - 275 + with self.assertQueryCount(margin=1, admin=235, emp=284): # test_mail only: 228 - 277 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -521,7 +521,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=148, emp=171): # test_mail only: 143 - 166 + with self.assertQueryCount(admin=147, emp=172): # test_mail only: 143 - 167 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -559,7 +559,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=152, emp=178): # test_mail only: 147 - 173 + with self.assertQueryCount(admin=152, emp=180): # test_mail only: 147 - 175 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, From 9f3889eba39fd39497208ec0a31fe8bc010a979a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Thu, 5 Apr 2018 14:48:19 +0200 Subject: [PATCH 3/3] [IMP] mail: limit cache invalidation when managing messages This commit proposes to limit cache invalidation at some cases that will trigger some behavior change when dealing with mail messages : * creating messages linked to a document; * update model or res_id of a message; * updating notifications, as notified people could change some computed fields on the record; This commit also invalidates only mail-related fields as updating messages should not invalidate other things than some computed fields linked to mail. --- addons/mail/models/mail_message.py | 16 ++++++++++++---- addons/test_mail/tests/test_performance.py | 10 +++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index e087d745861..740425bb75e 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -717,8 +717,14 @@ class Message(models.Model): def _invalidate_documents(self): """ Invalidate the cache of the documents followed by ``self``. """ for record in self: - if record.model and record.res_id: - self.env[record.model].invalidate_cache(ids=[record.res_id]) + if record.model and record.res_id and 'message_ids' in self.env[record.model]: + self.env[record.model].invalidate_cache(fnames=[ + 'message_ids', + 'message_unread', + 'message_unread_counter', + 'message_needaction', + 'message_needaction_counter', + ], ids=[record.res_id]) @api.model def create(self, values): @@ -764,7 +770,8 @@ class Message(models.Model): if tracking_values_cmd: message.sudo().write({'tracking_value_ids': tracking_values_cmd}) - message._invalidate_documents() + if values.get('model') and values.get('res_id'): + message._invalidate_documents() return message @@ -780,7 +787,8 @@ class Message(models.Model): if 'model' in vals or 'res_id' in vals: self._invalidate_documents() res = super(Message, self).write(vals) - self._invalidate_documents() + if 'notification_ids' in vals or 'model' in vals or 'res_id' in vals: + self._invalidate_documents() return res @api.multi diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 91c6017ef4b..08871500892 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -77,7 +77,7 @@ class TestMailPerformance(TransactionCase): 'partner_id': self.env.ref('base.res_partner_12').id, }) - with self.assertQueryCount(admin=7, demo=7): # test_mail only: 7 - 7 + with self.assertQueryCount(admin=6, demo=6): # test_mail only: 6 - 6 record.track = 'X' @users('admin', 'demo') @@ -93,7 +93,7 @@ class TestMailPerformance(TransactionCase): @warmup def test_create_mail_with_tracking(self): """ Create records inheriting from 'mail.thread' (with field tracking). """ - with self.assertQueryCount(admin=15, demo=15): # test_mail only: 15 - 15 + with self.assertQueryCount(admin=14, demo=14): # test_mail only: 14 - 14 self.env['test_performance.mail'].create({'name': 'X'}) @users('admin', 'emp') @@ -399,7 +399,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=136, emp=178): # com runbot 134 - 176 // test_mail only: 132 - 174 + with self.assertQueryCount(admin=133, emp=174): # com runbot 131 - 172 // test_mail only: 129 - 170 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -521,7 +521,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(admin=147, emp=172): # test_mail only: 143 - 167 + with self.assertQueryCount(admin=147, emp=172): # test_mail only: 142 - 167 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -593,7 +593,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) - with self.assertQueryCount(admin=60, emp=83): # test_mail only: 58 - 81 + with self.assertQueryCount(admin=57, emp=78): # test_mail only: 55 - 76 rec.write({ 'name': 'Test2', 'customer_id': customer_id,