From 30f4be4889968e89ce4c81e454b7de1758c03491 Mon Sep 17 00:00:00 2001 From: Thibault Francois Date: Fri, 29 Jan 2021 15:06:31 +0000 Subject: [PATCH] [FIX] (google|microsoft)_calendar: make sync more resilient Issue ----- - Currently the cron can take too much time and timeout. In case of timeout process data are rollback. The sync per user also crash and stop the process for all user and make the transaction rollback In both case we cannot ensure that all user will end up synchronized - The real time sync can end up in deadlock: If you create a holiday, it trigger a write on the user and the creation of an event that will be synchronize with google The synchronization may ask for a new token if this request fail a new cursor is create to empty on the user the refresh token. 2 cursor that are trying to write on the same record => Deadlock - Bad refresh token management When the refresh token is not anymore valid it's erased but the calendar_token and validity are kept. Is odoo consider the token valid it will try to sync an event with google|microsoft despite the refresh token is not valid anymore. Of course this lead to an error and the impossibility to create an event anymore without manually wiping the calendar_token and calendar_token_validity - Error 401 are handled in context manager (google|microsoft)_calendar_token, but get_*_calendar_token never raise http error only user error. Solution --------- - Improve cron resilience - Commit between each user and rollback in case of issue for a user - Solve the issue when stop < start - Solve issue with rrule not being the first element in the list of recurrence - Improve perf when searching for organizer - In order to limit the volume during the first synchronization, we only synchronized event from x days in the past to x days in the future with x = 365 by default - Limit the number of event to synchronize to google each time to 200, since this operation is time consuming (1 request is sent per event) - Avoid deadlock A new cursor is not needed, we only need to ensure that write on the user is done with a clean cursor and will be commited despite the error raise. So rollabck before the write and commit explicitly afterwards - Erase properly all the token and the token validity if the refresh token is not valid anymore - For google calendar, erase also the token in case of 401 error: Wrong client id, this mean the database parameter have been change and all the user should reconfigure the synchronization - Directly handle error 401 with error 400 in refresh_token method closes odoo/odoo#66250 X-original-commit: 486f69abad6226e5ce016ad89a0eeb103d709a3f Signed-off-by: Yannick Tivisse (yti) --- .../google_account/models/google_service.py | 2 +- addons/google_calendar/models/calendar.py | 15 +++++++++++++- addons/google_calendar/models/google_sync.py | 20 +++++++------------ addons/google_calendar/models/res_users.py | 13 +++++++----- .../tests/test_sync_odoo2google.py | 2 ++ .../google_calendar/utils/google_calendar.py | 11 +++++++++- addons/google_calendar/utils/google_event.py | 10 +++++++--- .../models/microsoft_sync.py | 14 ++----------- addons/microsoft_calendar/models/res_users.py | 17 +++++++++++----- 9 files changed, 63 insertions(+), 41 deletions(-) diff --git a/addons/google_account/models/google_service.py b/addons/google_account/models/google_service.py index 5a69f35bd2f..7905f4b074a 100644 --- a/addons/google_account/models/google_service.py +++ b/addons/google_account/models/google_service.py @@ -133,7 +133,7 @@ class GoogleService(models.AbstractModel): if headers is None: headers = {} - _logger.debug("Uri: %s - Type : %s - Headers: %s - Params : %s !", (uri, method, headers, params)) + _logger.debug("Uri: %s - Type : %s - Headers: %s - Params : %s !", uri, method, headers, params) ask_time = fields.Datetime.now() try: diff --git a/addons/google_calendar/models/calendar.py b/addons/google_calendar/models/calendar.py index 255500f7463..8f4c6405e6b 100644 --- a/addons/google_calendar/models/calendar.py +++ b/addons/google_calendar/models/calendar.py @@ -53,7 +53,16 @@ class Meeting(models.Model): return super().write(values) def _get_sync_domain(self): - return [('partner_ids.user_ids', 'in', self.env.user.id)] + # in case of full sync, limit to a range of 1y in past and 1y in the future by default + ICP = self.env['ir.config_parameter'].sudo() + day_range = int(ICP.get_param('google_calendar.sync.range_days', default=365)) + lower_bound = fields.Datetime.subtract(fields.Datetime.now(), days=day_range) + upper_bound = fields.Datetime.add(fields.Datetime.now(), days=day_range) + return [ + ('partner_ids.user_ids', 'in', self.env.user.id), + ('stop', '>', lower_bound), + ('start', '<', upper_bound) + ] @api.model def _odoo_values(self, google_event, default_reminders=()): @@ -84,6 +93,10 @@ class Meeting(models.Model): else: start = parse(google_event.start.get('date')) stop = parse(google_event.end.get('date')) - relativedelta(days=1) + # Stop date should be exclusive as defined here https://developers.google.com/calendar/v3/reference/events#resource + # but it seems that's not always the case for old event + if stop < start: + stop = parse(google_event.end.get('date')) values['allday'] = True values['start'] = start values['stop'] = stop diff --git a/addons/google_calendar/models/google_sync.py b/addons/google_calendar/models/google_sync.py index 0007e5d10c8..e8cc1113ebf 100644 --- a/addons/google_calendar/models/google_sync.py +++ b/addons/google_calendar/models/google_sync.py @@ -46,18 +46,8 @@ def after_commit(func): @contextmanager def google_calendar_token(user): - try: - yield user._get_google_calendar_token() - except requests.HTTPError as e: - if e.response.status_code == 401: # Invalid token. - # The transaction should be rolledback, but the user's tokens - # should be reset. The user will be asked to authenticate again next time. - # Rollback manually first to avoid concurrent access errors/deadlocks. - user.env.cr.rollback() - with user.pool.cursor() as cr: - env = user.env(cr=cr) - user.with_env(env)._set_auth_tokens(False, False, 0) - raise e + yield user._get_google_calendar_token() + class GoogleSync(models.AbstractModel): _name = 'google.calendar.sync' @@ -218,7 +208,11 @@ class GoogleSync(models.AbstractModel): '&', ('google_id', '=', False), is_active_clause, ('need_sync', '=', True), ]]) - return self.with_context(active_test=False).search(domain) + # We want to limit to 200 event sync per transaction, it shouldn't be a problem for the day to day + # but it allows to run the first synchro within an acceptable time without timeout. + # If there is a lot of event to synchronize to google the first time, + # they will be synchronized eventually with the cron running few times a day + return self.with_context(active_test=False).search(domain, limit=200) @api.model def _odoo_values(self, google_event: GoogleEvent, default_reminders=()): diff --git a/addons/google_calendar/models/res_users.py b/addons/google_calendar/models/res_users.py index 7159cd3a265..cc859e81d35 100644 --- a/addons/google_calendar/models/res_users.py +++ b/addons/google_calendar/models/res_users.py @@ -51,12 +51,12 @@ class User(models.Model): def _get_google_calendar_token(self): self.ensure_one() - if self._is_google_calendar_valid(): + if self.google_calendar_rtoken and not self._is_google_calendar_valid(): self._refresh_google_calendar_token() return self.google_calendar_token def _is_google_calendar_valid(self): - return self.google_calendar_token_validity and self.google_calendar_token_validity < (fields.Datetime.now() + timedelta(minutes=1)) + return self.google_calendar_token_validity and self.google_calendar_token_validity >= (fields.Datetime.now() + timedelta(minutes=1)) def _refresh_google_calendar_token(self): # LUL TODO similar code exists in google_drive. Should be factorized in google_account @@ -84,10 +84,11 @@ class User(models.Model): 'google_calendar_token_validity': fields.Datetime.now() + timedelta(seconds=ttl), }) except requests.HTTPError as error: - if error.response.status_code == 400: # invalid grant + if error.response.status_code in (400, 401): # invalid grant or invalid client # Delete refresh token and make sure it's commited - with self.pool.cursor() as cr: - self.env.user.with_env(self.env(cr=cr)).write({'google_calendar_rtoken': False}) + self.env.cr.rollback() + self._set_auth_tokens(False, False, 0) + self.env.cr.commit() error_key = error.response.json().get("error", "nc") error_msg = _("Something went wrong during your token generation. Maybe your Authorization Code is invalid or already expired [%s]", error_key) raise UserError(error_msg) @@ -129,8 +130,10 @@ class User(models.Model): _logger.info("Calendar Synchro - Starting synchronization for %s", user) try: user.with_user(user).sudo()._sync_google_calendar(google) + self.env.cr.commit() except Exception as e: _logger.exception("[%s] Calendar Synchro - Exception : %s !", user, exception_to_unicode(e)) + self.env.cr.rollback() def stop_google_synchronization(self): self.ensure_one() diff --git a/addons/google_calendar/tests/test_sync_odoo2google.py b/addons/google_calendar/tests/test_sync_odoo2google.py index b6a42fd14c2..7f74a5bb223 100644 --- a/addons/google_calendar/tests/test_sync_odoo2google.py +++ b/addons/google_calendar/tests/test_sync_odoo2google.py @@ -27,6 +27,8 @@ class TestSyncOdoo2Google(TransactionCase): def setUp(self): super().setUp() self.google_service = GoogleCalendarService(self.env['google.service']) + # Make sure this test will work for the next 30 years + self.env['ir.config_parameter'].set_param('google_calendar.sync.range_days', 10000) def assertGoogleEventDeleted(self, google_id): GoogleSync._google_delete.assert_called() diff --git a/addons/google_calendar/utils/google_calendar.py b/addons/google_calendar/utils/google_calendar.py index 20f30d33a7f..b02019cb846 100644 --- a/addons/google_calendar/utils/google_calendar.py +++ b/addons/google_calendar/utils/google_calendar.py @@ -6,7 +6,7 @@ import requests import json import logging -from odoo import api, _ +from odoo import fields from odoo.tools import exception_to_unicode from odoo.addons.google_calendar.utils.google_event import GoogleEvent from odoo.addons.google_account.models.google_service import TIMEOUT @@ -36,6 +36,15 @@ class GoogleCalendarService(): params = {'access_token': token} if sync_token: params['syncToken'] = sync_token + else: + # full sync, limit to a range of 1y in past to 1y in the futur by default + ICP = self.google_service.env['ir.config_parameter'].sudo() + day_range = int(ICP.get_param('google_calendar.sync.range_days', default=365)) + _logger.info("Full cal sync, restricting to %s days range", day_range) + lower_bound = fields.Datetime.subtract(fields.Datetime.now(), days=day_range) + upper_bound = fields.Datetime.add(fields.Datetime.now(), days=day_range) + params['timeMin'] = lower_bound.isoformat() + 'Z' # Z = UTC (RFC3339) + params['timeMax'] = upper_bound.isoformat() + 'Z' # Z = UTC (RFC3339) try: status, data, time = self.google_service._do_request(url, params, headers, method='GET', timeout=timeout) except requests.HTTPError as e: diff --git a/addons/google_calendar/utils/google_event.py b/addons/google_calendar/utils/google_event.py index 8c11af3ce41..b4008e5848e 100644 --- a/addons/google_calendar/utils/google_event.py +++ b/addons/google_calendar/utils/google_event.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. from odoo.api import model +from odoo.tools import email_normalize from odoo.tools.sql import existing_tables import pytz from typing import Iterator, Mapping @@ -65,8 +66,10 @@ class GoogleEvent(abc.Set): @property def rrule(self): - if self.recurrence and 'RRULE:' in self.recurrence[0]: # LUL TODO what if there are something else in the list? - return self.recurrence[0][6:] # skip "RRULE:" in the rrule string + if self.recurrence: + # Find the rrule in the list + rrule = next(rr for rr in self.recurrence if 'RRULE:' in rr) + return rrule[6:] # skip "RRULE:" in the rrule string def odoo_id(self, env): self.odoo_ids(env) # load ids @@ -142,7 +145,8 @@ class GoogleEvent(abc.Set): return env.user elif self.organizer and self.organizer.get('email'): # In Google: 1 email = 1 user; but in Odoo several users might have the same email :/ - return env['res.users'].search([('email', '=', self.organizer.get('email'))], limit=1) + org_email = email_normalize(self.organizer.get('email')) + return env['res.users'].search([('email_normalized', '=', org_email)], limit=1) else: return env['res.users'] diff --git a/addons/microsoft_calendar/models/microsoft_sync.py b/addons/microsoft_calendar/models/microsoft_sync.py index ccfde004118..cbec487ff45 100644 --- a/addons/microsoft_calendar/models/microsoft_sync.py +++ b/addons/microsoft_calendar/models/microsoft_sync.py @@ -48,18 +48,8 @@ def after_commit(func): @contextmanager def microsoft_calendar_token(user): - try: - yield user._get_microsoft_calendar_token() - except requests.HTTPError as e: - if e.response.status_code == 401: # Invalid token. - # The transaction should be rolledback, but the user's tokens - # should be reset. The user will be asked to authenticate again next time. - # Rollback manually first to avoid concurrent access errors/deadlocks. - user.env.cr.rollback() - with user.pool.cursor() as cr: - env = user.env(cr=cr) - user.with_env(env)._set_microsoft_auth_tokens(False, False, 0) - raise e + yield user._get_microsoft_calendar_token() + class MicrosoftSync(models.AbstractModel): _name = 'microsoft.calendar.sync' diff --git a/addons/microsoft_calendar/models/res_users.py b/addons/microsoft_calendar/models/res_users.py index 706f003bc9c..4784184cb98 100644 --- a/addons/microsoft_calendar/models/res_users.py +++ b/addons/microsoft_calendar/models/res_users.py @@ -39,12 +39,12 @@ class User(models.Model): def _get_microsoft_calendar_token(self): self.ensure_one() - if self._is_microsoft_calendar_valid(): + if self.microsoft_calendar_rtoken and not self._is_microsoft_calendar_valid(): self._refresh_microsoft_calendar_token() return self.microsoft_calendar_token def _is_microsoft_calendar_valid(self): - return self.microsoft_calendar_token_validity and self.microsoft_calendar_token_validity < (fields.Datetime.now() + timedelta(minutes=1)) + return self.microsoft_calendar_token_validity and self.microsoft_calendar_token_validity >= (fields.Datetime.now() + timedelta(minutes=1)) def _refresh_microsoft_calendar_token(self): self.ensure_one() @@ -72,10 +72,15 @@ class User(models.Model): 'microsoft_calendar_token_validity': fields.Datetime.now() + timedelta(seconds=ttl), }) except requests.HTTPError as error: - if error.response.status_code == 400: # invalid grant + if error.response.status_code in (400, 401): # invalid grant # Delete refresh token and make sure it's commited - with self.pool.cursor() as cr: - self.env.user.with_env(self.env(cr=cr)).write({'microsoft_calendar_rtoken': False}) + self.env.cr.rollback() + self.write({ + 'microsoft_calendar_rtoken': False, + 'microsoft_calendar_token': False, + 'microsoft_calendar_token_validity': False, + }) + self.env.cr.commit() error_key = error.response.json().get("error", "nc") error_msg = _("Something went wrong during your token generation. Maybe your Authorization Code is invalid or already expired [%s]", error_key) raise UserError(error_msg) @@ -117,8 +122,10 @@ class User(models.Model): _logger.info("Calendar Synchro - Starting synchronization for %s", user) try: user.with_user(user).sudo()._sync_microsoft_calendar(microsoft) + self.env.cr.commit() except Exception as e: _logger.exception("[%s] Calendar Synchro - Exception : %s !", user, exception_to_unicode(e)) + self.env.cr.rollback() def stop_microsoft_synchronization(self): self.ensure_one()