-
Notifications
You must be signed in to change notification settings - Fork 203
fix(outlook): dispatch Contacts by item type and harden folder/field assumptions #4147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
fff7d54
7d92b86
a9fb212
c976747
0e99183
09a7de2
cf5231a
3cd23fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||
| iso_utc, | ||||
| ) | ||||
| from exchangelib.errors import ErrorNonExistentMailbox | ||||
| from exchangelib.items import DistributionList | ||||
|
|
||||
| from connectors.access_control import ACCESS_CONTROL, es_access_control_query | ||||
| from connectors.sources.outlook.client import OutlookClient, _extract_ldap_mail | ||||
|
|
@@ -34,6 +35,11 @@ | |||
| ) | ||||
| from connectors.utils import html_to_text | ||||
|
|
||||
| # Per-item errors: a single malformed item is skipped, not fatal. Connection-wide | ||||
| # failures (transport, SSL, rate limit, missing mailbox) are excluded on purpose | ||||
| # so they still abort the sync instead of silently emptying the index. | ||||
| ITEM_SHAPE_ERRORS = (AttributeError, KeyError, ValueError, TypeError) | ||||
|
|
||||
|
|
||||
| class OutlookDocFormatter: | ||||
| """Format Outlook object documents to Elasticsearch document""" | ||||
|
|
@@ -90,11 +96,13 @@ def calendar_doc_formatter(self, calendar, child_calendar, timezone): | |||
| } | ||||
|
|
||||
| if child_calendar in ["Folder (Birthdays)", "Birthdays (Birthdays)"]: | ||||
| # calendar.start may be missing; guard against splitting a None. | ||||
| birthday = ews_format_to_datetime( | ||||
| source_datetime=calendar.start, timezone=timezone | ||||
| ) | ||||
| document.update( | ||||
| { | ||||
| "date": ews_format_to_datetime( | ||||
| source_datetime=calendar.start, timezone=timezone | ||||
| ).split("T", 1)[0], | ||||
| "date": birthday.split("T", 1)[0] if birthday else None, | ||||
| } | ||||
| ) | ||||
| else: | ||||
|
|
@@ -168,6 +176,23 @@ def contact_doc_formatter(self, contact, timezone): | |||
| ), | ||||
| } | ||||
|
|
||||
| def distribution_list_doc_formatter(self, distribution_list, timezone): | ||||
| # A contact group has members, not per-contact fields, so it needs its own shape. | ||||
| return { | ||||
| "_id": distribution_list.id, | ||||
| "type": "Distribution List", | ||||
| "_timestamp": ews_format_to_datetime( | ||||
| source_datetime=distribution_list.last_modified_time, | ||||
| timezone=timezone, | ||||
| ), | ||||
| "name": distribution_list.display_name, | ||||
| "email_addresses": [ | ||||
| member.mailbox.email_address | ||||
| for member in (distribution_list.members or []) | ||||
| if member and member.mailbox and member.mailbox.email_address | ||||
| ], | ||||
| } | ||||
|
|
||||
| def attachment_doc_formatter(self, attachment, attachment_type, timezone): | ||||
| attachment_id = ( | ||||
| attachment.attachment_id.id if attachment.attachment_id else None | ||||
|
|
@@ -488,6 +513,31 @@ async def download_func(self, content): | |||
| """ | ||||
| yield content | ||||
|
|
||||
| def _format_item(self, formatter, item, account): | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually has tendency to actually produce syncs that are successful but return 0 items. I'm not really sure we should do this. We've tried an approach with this class:
Error Monitor swallows errors up to a certain threshold and then raises an exception if too many errors happen. It also has its downsides.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing the train of thought from above: I feel this method attempt to help not raise when formatting objects of unexpected types. E.g. you have an API returning types So instead each formatter method can do type checks and warn or error if type is unexpected. Warn or error? Not sure. It's all about expectation setting to me. Warns are good because the sync does not crash Because of that Error Monitor will not actually help, but will just hide the problem. All in all I think it's best to do type checks in formatter methods and warn on unexpected types. I am open to a discussion here, though
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. Fixed. Now we're raising instead of silently skipping. |
||||
| """Run a document formatter for a single item, isolating item-shape | ||||
| errors so one malformed Exchange object is skipped (with a warning) | ||||
| instead of aborting the whole sync. | ||||
|
|
||||
| Args: | ||||
| formatter (callable): Zero-argument callable that formats the item | ||||
| (typically a functools.partial around a doc formatter). | ||||
| item: The Exchange item being formatted, used for logging context. | ||||
| account: The account the item belongs to, used for logging context. | ||||
|
|
||||
| Returns: | ||||
| The formatted document, or None if the item was malformed and | ||||
| skipped. Connection-wide errors are not caught here and propagate. | ||||
| """ | ||||
| try: | ||||
| return formatter() | ||||
| except ITEM_SHAPE_ERRORS as error: | ||||
| self._logger.warning( | ||||
| f"Skipping malformed {type(item).__name__} item " | ||||
| f"{getattr(item, 'id', 'unknown')} for " | ||||
| f"{account.primary_smtp_address}: {error}" | ||||
| ) | ||||
| return None | ||||
|
|
||||
| async def _fetch_attachments( | ||||
| self, attachment_type, outlook_object, timezone, account | ||||
| ): | ||||
|
|
@@ -497,11 +547,18 @@ async def _fetch_attachments( | |||
| f"Skipping attachment without an ID on item {outlook_object.id}" | ||||
| ) | ||||
| continue | ||||
| document = self.doc_formatter.attachment_doc_formatter( | ||||
| attachment=attachment, | ||||
| attachment_type=attachment_type, | ||||
| timezone=timezone, | ||||
| document = self._format_item( | ||||
| partial( | ||||
| self.doc_formatter.attachment_doc_formatter, | ||||
| attachment=attachment, | ||||
| attachment_type=attachment_type, | ||||
| timezone=timezone, | ||||
| ), | ||||
| item=attachment, | ||||
| account=account, | ||||
| ) | ||||
| if document is None: | ||||
| continue | ||||
| yield ( | ||||
| self._decorate_with_access_control( | ||||
| document, [account.primary_smtp_address] | ||||
|
|
@@ -513,11 +570,18 @@ async def _fetch_attachments( | |||
|
|
||||
| async def _fetch_mails(self, account, timezone): | ||||
| async for mail, mail_type in self.client.get_mails(account=account): | ||||
| document = self.doc_formatter.mails_doc_formatter( | ||||
| mail=mail, | ||||
| mail_type=mail_type, | ||||
| timezone=timezone, | ||||
| document = self._format_item( | ||||
| partial( | ||||
| self.doc_formatter.mails_doc_formatter, | ||||
| mail=mail, | ||||
| mail_type=mail_type, | ||||
| timezone=timezone, | ||||
| ), | ||||
| item=mail, | ||||
| account=account, | ||||
| ) | ||||
| if document is None: | ||||
| continue | ||||
| yield ( | ||||
| self._decorate_with_access_control( | ||||
| document, [account.primary_smtp_address] | ||||
|
|
@@ -537,10 +601,22 @@ async def _fetch_mails(self, account, timezone): | |||
| async def _fetch_contacts(self, account, timezone): | ||||
| self._logger.debug(f"Fetching contacts for {account.primary_smtp_address}") | ||||
| async for contact in self.client.get_contacts(account=account): | ||||
| document = self.doc_formatter.contact_doc_formatter( | ||||
| contact=contact, | ||||
| timezone=timezone, | ||||
| ) | ||||
| # Contacts folder holds both contacts and groups; each has its own shape. | ||||
| if isinstance(contact, DistributionList): | ||||
| formatter = partial( | ||||
| self.doc_formatter.distribution_list_doc_formatter, | ||||
| distribution_list=contact, | ||||
| timezone=timezone, | ||||
| ) | ||||
| else: | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally like doing explicit type checks and warn/raise on the case when the type is unexpected.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||
| formatter = partial( | ||||
| self.doc_formatter.contact_doc_formatter, | ||||
| contact=contact, | ||||
| timezone=timezone, | ||||
| ) | ||||
| document = self._format_item(formatter, item=contact, account=account) | ||||
| if document is None: | ||||
| continue | ||||
| yield ( | ||||
| self._decorate_with_access_control( | ||||
| document, [account.primary_smtp_address] | ||||
|
|
@@ -551,9 +627,17 @@ async def _fetch_contacts(self, account, timezone): | |||
| async def _fetch_tasks(self, account, timezone): | ||||
| self._logger.debug(f"Fetching tasks for {account.primary_smtp_address}") | ||||
| async for task in self.client.get_tasks(account=account): | ||||
| document = self.doc_formatter.task_doc_formatter( | ||||
| task=task, timezone=timezone | ||||
| document = self._format_item( | ||||
| partial( | ||||
| self.doc_formatter.task_doc_formatter, | ||||
| task=task, | ||||
| timezone=timezone, | ||||
| ), | ||||
| item=task, | ||||
| account=account, | ||||
| ) | ||||
| if document is None: | ||||
| continue | ||||
| yield ( | ||||
| self._decorate_with_access_control( | ||||
| document, [account.primary_smtp_address] | ||||
|
|
@@ -597,11 +681,18 @@ async def _fetch_child_calendars(self, account, timezone): | |||
| yield doc | ||||
|
|
||||
| async def _enqueue_calendars(self, calendar, child_calendar, timezone, account): | ||||
| document = self.doc_formatter.calendar_doc_formatter( | ||||
| calendar=calendar, | ||||
| child_calendar=str(child_calendar), | ||||
| timezone=timezone, | ||||
| document = self._format_item( | ||||
| partial( | ||||
| self.doc_formatter.calendar_doc_formatter, | ||||
| calendar=calendar, | ||||
| child_calendar=str(child_calendar), | ||||
| timezone=timezone, | ||||
| ), | ||||
| item=calendar, | ||||
| account=account, | ||||
| ) | ||||
| if document is None: | ||||
| return | ||||
| yield ( | ||||
| self._decorate_with_access_control( | ||||
| document, [account.primary_smtp_address] | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanity check - this does not produce network call,
folder.all()does, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually it's the other way around:
account.calendarmakes the network call. It is the same asself.root.get_default_folder(Calendar), which callsGetFolderand raisesErrorFolderNotFound.folder.all()on the other hand is a lazyQuerySet. No network call until it gets iterated, and noErrorFolderNotFound. So we should be good here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then there is a problem here.
exchangelibis not async, and the call toaccount.calendaris gonna block the whole process until it resolves. Then we'd need to wrap it intoasyncio.to_threador something similarThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This then will have to be done to the methods that do network calls below unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. You're right. Done.